Fix incorrect handling of nested dialogs #399
Conversation
KittyGiraudel
changed the title
| @@ -13,15 +13,15 @@ describe('Nested dialogs', () => { | |||
| }) | |||
|
|
|||
| it('should close only the top most dialog when pressing ESC', () => { | |||
| cy.get('body').trigger('keydown', { keyCode: 27, which: 27 }) | |||
| cy.get('html').trigger('keydown', { key: 'Escape', keyCode: 27, which: 27 }) | |||
There was a problem hiding this comment.
First , the keydown event is bound to the document, and not the body element (although I’m pretty sure it doesn’t matter in that case). Secondly, the event listener looks at the key property of the event since this commit, so this was just not working.
| cy.get('[data-a11y-dialog="dialog-2"]').then(shouldBeVisible) | ||
| cy.get('[data-a11y-dialog="dialog-3"]').then(shouldBeHidden) | ||
| }) | ||
|
|
||
| it('should close only the top most dialog when clicking the backdrop', () => { | ||
| cy.get('body').trigger('keydown', { keyCode: 27, which: 27 }) |
There was a problem hiding this comment.
I have no idea why we performed an additional ESC here, but this is wrong.
| cy.get('[data-a11y-dialog="dialog-2"]') | ||
| .find('.dialog-overlay') | ||
| .first() |
There was a problem hiding this comment.
Because a dialog container now contains multiple overlay (since the DOM trees are now properly nested), we need to find the first one.
KittyGiraudel
force-pushed
the
nested-dialogs-v7
branch
from
e828e85
to
f23080f
Compare
| @@ -236,7 +249,8 @@ A11yDialog.prototype._fire = function (type, event) { | |||
| A11yDialog.prototype._bindKeypress = function (event) { | |||
| // This is an escape hatch in case there are nested dialogs, so the keypresses | |||
| // are only reacted to for the most recent one | |||
| if (!this.$el.contains(document.activeElement)) return | |||
There was a problem hiding this comment.
Now that the dialog are properly nested, this escape hatch no longer works since the active element is always within all the “higher” dialogs.
We need to flip that logic and check whether the active element is directly within the current dialog.
It turns out that the demo for nested dialogs was not working correctly with VoiceOver on Safari. Nothing gets announced passed the first open dialog.
That’s because the dialogs’ DOM tree were laid out flat (as an implementation artifact from the old system from version 6). As a result, “lower“ dialogs ended up outside of the
aria-modal="true"container of the “higher” dialogs. Because of this, VoiceOver on Safari would be unable to pick up their content.This problem can be solved by properly nesting the nested dialogs’ DOM tree so that “lower” dialogs live within the
aria-modal="true"container of their “higher” dialog. This way, an open dialog is never outside of an element witharia-modal="true".Unfortunately, that makes the code to retrieve the elements that can close the dialog a bit more cumbersome since it needs to account for potentially nested dialogs. This code is not quite bulletproof, but it should do the trick in most scenarios, especially when coupled with better documentation. It will no longer be an issue in version 8 thanks to event delegation.
Kind thanks to @NanneWD for reporting that issue.