Ensure live region is appended to role="dialog"
#12
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR updates the logic for appending the live region so that, in addition to native dialog elements, elements with role="dialog" are also considered.
- Added an extra check for elements with the attribute role="dialog"
- Ensured proper fallback to getRootNode() when no dialog is found
Comments suppressed due to low confidence (1)
ariaNotify-polyfill.js:76
- Consider adding unit tests to verify that the live region is correctly appended when an ancestor element with role='dialog' is present, ensuring this new logic behaves as expected across different DOM structures.
this.element.closest("dialog") || this.element.closest("[role='dialog']") || this.element.getRootNode()
There was a problem hiding this comment.
Out of curiosity: Is it necessary to create the <live-region> element inside a non-modal dialog, or only necessary for modal dialogs?
Either way, it’s not harmful to create it inside—and the existing check did not distinguish between modal and non-modal <dialog> elements—so I’m okay approving this PR.
|
Thanks for contributing! This is being released in v2.1.0: https://github.com/github/ariaNotify-polyfill/releases/tag/v2.1.0. |
|
@smockle that is an excellent question! 👀 Looking through dialog testing results from @TylerJDev, it does seem like the problem is with modal dialogs! Note: In the test results doc linked above:
Since the "modal-ness" of I don't think what we have hurts though, and it's probably worth testing again before we scope down! |
|
Semi-related: It’s possible to detect whether a |
|
NICE! |
Fixes: #13
Ensures that live region gets appended to role="dialog" in addition to
<dialog>.It'd be nice to add tests for dialog scenarios we use ariaNotify in covering
<dialog>and[role="dialog"]. It doesn't look like there are any right now - it looks a bit involved, so I will not tackle that in this PR. Filed: #14