Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(Modal): Prevent auto aria-labelledby if aria-label is passed #11012

Merged

Conversation

wise-king-sullyman
Copy link
Contributor

What: Closes #11011

Additional issues:

@patternfly-build
Copy link
Contributor

patternfly-build commented Sep 17, 2024

Comment on lines 79 to 83
if (boxId) {
idRefList.push(boxId);
}
if (ariaLabelledby) {
idRefList.push(ariaLabelledby);
Copy link
Contributor

@thatblindgeye thatblindgeye Sep 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We may want to only allow either boxId or aria-labelledby to be pushed to idRefList, not both. Right now the pf-v6-c-modal-box wrapper element may have extraneous labeling if an aria-labelledby is passed and that value references an element inside the Modal (like the basic modal example is doing).

So right now in that basic example, the accessible name of the dialog is everything in it, followed by its title (which is already announced). Having the boxId act as a fallback if both aria-label and aria-labelledby are missing would work better. Another thing, though, is that the check we're doing in the Modal code (~line 135) isn't really necessary, because if a consumer doesn't pass an ID to the Modal then we're constructing it, so we're always applying an aria-labelledby.

Unless I'm missing a case where that boxId would actually never be a valid value and aria-labelledby wouldn't automatically be applied or anything.

Copy link
Contributor

@tlabaj tlabaj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was this issue found when moving from deprecated modal. We may want to fix in v5 branch for other migrating... thoughts?

@tlabaj tlabaj merged commit 5bb4c1f into patternfly:main Sep 19, 2024
13 checks passed
@patternfly-build
Copy link
Contributor

Your changes have been released in:

Thanks for your contribution! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Test error: Modal being returned by getByLabelText queries when it shouldn't
5 participants