fix(overlays): only close the top overlay on [Esc] #2198
Conversation
gerjanvangeest
requested review from
tlouisse,
daKmoR,
erikkroes,
ruphin,
narzac,
Knorgias,
whenlit,
DannyMoerkerke and
okadurin
as code owners
🦋 Changeset detectedLatest commit: 9329d56 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
| ctrl2.contentNode.dispatchEvent(new KeyboardEvent('keyup', { key: 'Escape' })); | ||
| await aTimeout(0); | ||
| expect(ctrl2.isShown).to.be.false; | ||
| expect(ctrl1.isShown).to.be.true; |
There was a problem hiding this comment.
Does this test fail when _blocksEscKeyHandler is left out of the equation?
I would expect the keyup event to never reach ctrl1.contentNode, as ctrl2.contentNode and ctrl1.contentNode are siblings.
The test should probably create a setup where ctrl2.contentNode is nested inside ctrl1.contentNode
| this.__shownList = this.shownList.filter(ctrl => ctrl !== ctrlToHide); | ||
| setTimeout(() => { |
There was a problem hiding this comment.
Can we explain what this code does and why the timeout is needed?
There was a problem hiding this comment.
It removes the _blocksEscKeyHandler from the next overlay.
If there is no timeout, the unblocking will be done before the Esc will trigger the hide overlay. If you unblock it one tick later, it all works as expected.
| * @param {KeyboardEvent} ev | ||
| * @private | ||
| */ | ||
| __escKeyHandler(ev) { |
There was a problem hiding this comment.
Maybe we can also check this.handleHidesOnOutsideEsc here (instead of overriding the __escKeyHandlerin _handleHidesOnOutsideEsc...)
(Know you didn't write it, but while we're at it :))
There was a problem hiding this comment.
Yes, but we don't want to have the _blocksEscKeyHandler in that function.
Then its better to have a separate escKeyOutisideHandler
| @@ -629,6 +654,30 @@ describe('OverlayController', () => { | |||
| ctrl.contentNode.dispatchEvent(new KeyboardEvent('keyup', { key: 'Escape' })); | |||
| expect(ctrl.isShown).to.be.true; | |||
| }); | |||
|
|
|||
| it('hides all overlays when [escape] is pressed on outside element', async () => { | |||
There was a problem hiding this comment.
I wonder if the whole approach still works if we have two ctrls: ctrl1 with hidesOnOutsideEsc and ctrl2 with hidesOnEsc. And ctrl2 is nested in ctrl1 (its contentNode is nested).
Would ctrl1 still be autonomous and be closed? (I suspect not, as _blocksEscKeyHandler works as a global option (on OverlaysManager level) now).
There was a problem hiding this comment.
Maybe we can solve this in a different way (without _blocksEscKeyHandler)?
You could for instance check whether the esc event came from a child in the dom. If so, and we have closeOnEsc and not closeOnOutsideEsc, we should not close ourselves.
gerjanvangeest
force-pushed
the
fix/nested-overlays-esc
branch
from
69f2571
to
84814fc
Compare
What I did