Feature/#10 modal focus trap #17
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
zetareticoli
added
bug
omaxel
reviewed
Oct 16, 2021
lib/modal/modal.html
Outdated
|
|
||
| <!-- Modal --> | ||
| <div data-modal> | ||
| <div id="dialog" data-modal-dialog role="dialog" aria-modal="true" aria-labelledby="modal title" tabindex="-1"> |
There was a problem hiding this comment.
Using aria-labelledby="modal title" could mislead an inexperienced developer putting the modal title as value of the aria-labelledby attribute. It should refer to element(s) that have the text needed for labeling.
I suggest applying an id to the <h3> element and use the same for the aria-labelledby attribute.
As a reference, this is the same approach used by WAI-ARIA.
There was a problem hiding this comment.
You're absolutely right.
|
The documentation you wrote is a nice way to guide the developer to the problem solution. Thank you @zetareticoli. |
zetareticoli
added a commit
that referenced
this pull request
Nov 11, 2021
* Fix: Accordion focus on closed panel child element (#14) * Fix: Accordion focus on closed tab child element * Updated accordion documentation * Added generated file Co-authored-by: Omar Muscatello <7016897+OmarMuscatello@users.noreply.github.com> * Feature/#10 modal focus trap (#17) * Added role and aria-modal attributes, adde x icon to close #10 * Added aria-labelledby #10 * Added tabindex to close button #10 * Added more documentation about Focus Trap and Page Scroll #10 * Change modal template #10 * Changed blockquote style #10 * Better doc #10 * Fixed aria-labelledby attribute #10 * Changed styel * Feature/#7 components docs (#18) * Added more content to Tabs doc #7 * Changed lead color #7 * Added support for :focus event on tooltips #7 * Changed Tooltips docs #7 * Show ring on focused tab panel's label (#20) Co-authored-by: Omar Muscatello <7016897+OmarMuscatello@users.noreply.github.com> * Removed container parent maintaining accessibility (#24) Co-authored-by: Omar Muscatello <7016897+OmarMuscatello@users.noreply.github.com> * Added focus ring on focused accordion item's label (#25) Co-authored-by: Omar Muscatello <7016897+OmarMuscatello@users.noreply.github.com> * Improved tabs accessibility (#23) Co-authored-by: Omar Muscatello <7016897+OmarMuscatello@users.noreply.github.com> * Removed accordion container from HTML preview #24 * Added information to CHANGELOG * Added upcoming release info * Added accessibility info for tabs * Bumped lib version for npm package * Fixed tabs accessibility doc Co-authored-by: Omar Muscatello <7016897+omaxel@users.noreply.github.com> Co-authored-by: Omar Muscatello <7016897+OmarMuscatello@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The focus trap issue couldn't be solved in pure CSS. It needs some Javascript.
So, I provided more info about both Focus Trap and Page Scroll Lock features, linking to highly rated npm packages, if needed.
I added the missing HTML
ariaandroleattributes on the modal container.Check this out and let me know if we need more changes before merge.