Skip to content

Commit a1b9cbf

Browse files
gnbmgnbm
committed
fix: honour an open that arrives while the dropbox is still closing
PopoverComponent.show() early-returns for as long as `pop-comp-active` is on the element, and only its own afterHide removes that class. So openDropbox() called during the ~200ms hide transition did all of its own work - beforeOpen, aria-expanded="true", the instance back in openInstances - while popper.show() quietly did nothing. afterShowPopper() never ran, so there was no `focused` class, no focus moved into the list and no afterOpen; the hide that was still pending then completed on top of it. The dropdown ended up shut with its combobox still announcing aria-expanded="true", and focus inside it dropped to <body>. The host's open() was silently lost - it looks like nothing happened, which is the worst shape for a bug to have. Reachable through the public open() API only: toggleDropbox() sees isOpened() === true for the whole transition, so a second click closes again rather than reopening. openDropbox() now queues instead of running when the popover is mid-hide, and afterHidePopper() replays the open once the popover is idle again - immediately before the existing "stand down if the dropdown is open again" guard, so the replay reuses the one path that already knows not to hide an open dropdown rather than adding a second. closeDropbox() clears the queue, so a close arriving after the queued open wins. Both conditions are required to queue. isClosingTransition alone would strand every later open if it were ever left set with no hide actually pending; isShown() alone is true of a dropdown that is simply already open, where re-opening must stay a no-op rather than a queued one. Together they mean exactly "a hide is running and the popover will refuse to show". Nothing reaches into dropboxPopover.popper to cancel its timer - that would couple this component to the plugin's internals - so the cost is one extra fade before the dropdown appears, which is inherent: the popover cannot show while it is hiding. Queueing before any work also keeps the event contract intact. Letting openDropbox() run and re-running it from afterHidePopper() would dispatch beforeOpen twice for one open(), which would make a host that lazy-loads options on beforeOpen fetch twice. test: pin the reopen against the popover's hide transition reopen-during-hide-transition.cy.ts, five cases: the reopen ends up open and on screen, the open actually finishes (focus in the search input, the `focused` class), beforeOpen and afterOpen each fire exactly once, a close following the queued open cancels it, and an ordinary open with no hide running is still applied synchronously rather than deferred. The first three were red against the previous commit. Each waits out the hide and the show that has to follow it before asserting: every assertion also holds during the fade - openDropbox() sets aria-expanded and clears aria-hidden synchronously, and the popover has not yet applied display:none - so a retrying should() would otherwise latch onto that transient and pass against the bug. a11y-aria-hidden-focus.cy.ts's mid-transition case is rewritten. Its premise - that the reopen is lost - no longer holds, so it now pins the accessibility half of the queued path: the queue must hold the pointer gate up for the rest of the fade, so the list cannot take DOM focus back while the hide is still on its way to marking the subtree hidden.
1 parent ab70d57 commit a1b9cbf

3 files changed

Lines changed: 255 additions & 63 deletions

File tree

‎cypress/e2e/a11y-aria-hidden-focus.cy.ts‎

Lines changed: 21 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -421,67 +421,46 @@ describe('A11y: aria-hidden is never applied over the focused element', { testIs
421421
});
422422

423423
/**
424-
* A hide that lost a race with a reopen still lands. A characterisation guard, not a
425-
* red-then-green regression case - it passes on the code that preceded it, and pins two
426-
* things a plausible "fix" in this area would break.
424+
* An open arriving mid-fade is queued behind the running hide (see
425+
* reopen-during-hide-transition.cy.ts for the reopen behaviour itself). This is the a11y half
426+
* of that path: the queue must hold the pointer gate up for the rest of the fade, so the list
427+
* cannot take DOM focus back while the hide is still on its way to marking the subtree hidden.
427428
*
428-
* `open()` during the fade does not cancel the popover's pending hide: its `show()`
429-
* early-returns while `pop-comp-active` is still set, so the popper's hide timer is never
430-
* cleared and afterHidePopper() runs ~200ms later against an instance that is back in
431-
* `openInstances` (see AI-33 in ACTION-ITEMS.md - the reopen is silently lost). Reopening
432-
* also lifts the pointer gate, so the still-rendered list can take DOM focus again in
433-
* between.
434-
*
435-
* 1. No aria-hidden violation on that path. It holds today only because the popover
436-
* applies display:none *before* calling back, so the browser has already moved focus
437-
* out - an ordering this component does not control, which is exactly why it is
438-
* pinned rather than assumed.
439-
* 2. The dropdown stays reopenable. The stale hide has to *complete* rather than stand
440-
* down: skipping it (e.g. gating afterHidePopper on `openInstances`) leaves the
441-
* wrapper without its `closed` class while the popover has already gone invisible, so
442-
* every later toggle sees `isOpened() === true` and closes a dropdown that is not
443-
* there. Confirmed by building that variant - these last assertions are what failed.
429+
* Worth pinning separately because the obvious way to write the queue - run openDropbox()
430+
* eagerly and repair afterwards - lifts `isClosingTransition` and hands the fading list back
431+
* to the pointer, which is exactly how focus used to end up inside an aria-hidden subtree.
444432
*/
445-
it('releases focus when a hide that lost a race with a reopen finally lands', () => {
433+
it('keeps the pointer gate up while an open waits behind the hide transition', () => {
446434
mount();
447435
open();
448436
waitForSearchFocus();
449437

450438
cy.window().then((win) => {
451-
const vs = win.document.getElementById(mountId)!.virtualSelect;
439+
const $ele = win.document.getElementById(mountId)!;
440+
const vs = $ele.virtualSelect;
452441

453442
vs.closeDropbox();
454-
win.document.getElementById(mountId)!.open?.();
443+
expect(vs.isOpened(), 'still mid hide-transition when the reopen arrives').to.equal(true);
444+
445+
$ele.open?.();
455446

456-
/** precondition: the reopen registered, and the pointer gate is back off */
457-
expect((win as any).VirtualSelect.openInstances.has(vs), 'reopened instance tracked').to.equal(true);
458-
expect(vs.isClosingTransition, 'pointer gate lifted by the reopen').to.equal(false);
447+
expect(vs.isClosingTransition, 'pointer gate held while the open waits').to.equal(true);
459448

460449
hoverOptionNow(vs, 3);
461450

462-
/** precondition: the hover genuinely put DOM focus back inside the fading dropbox */
463-
expect(
464-
vs.$dropboxContainer.contains(win.document.activeElement),
465-
'focus back inside after the reopen',
466-
).to.equal(true);
451+
expect(vs.$dropboxContainer.querySelector('.vscomp-option.focused'), 'highlight after hover').to.equal(null);
452+
expect(vs.$dropboxContainer.contains(win.document.activeElement), 'focus after hover').to.equal(false);
467453
});
468454

469455
assertNoViolations();
470456

471-
cy.window().then((win) => {
472-
const vs = win.document.getElementById(mountId)!.virtualSelect;
473-
474-
expect(vs.$dropboxWrapper.contains(win.document.activeElement), 'focus left in the hidden dropbox')
475-
.to.equal(false);
476-
});
477-
478-
/** the stale hide must leave the component closed, so the next open still works */
479-
cy.get(`#${mountId}`).find('.vscomp-wrapper').should('have.class', 'closed');
480-
cy.get(`#${mountId}`).find('.vscomp-toggle-button').click();
457+
/** and the queued open still lands, on a dropbox that was never marked hidden underneath it */
481458
cy.get(`#${mountId}`).find('.vscomp-wrapper').should('not.have.class', 'closed');
482459
cy.get(`#${mountId}`).should(($ele) => {
483-
expect($ele[0].virtualSelect.$dropboxContainer.getBoundingClientRect().height, 'reopened dropbox height')
484-
.to.be.greaterThan(0);
460+
const vs = $ele[0].virtualSelect;
461+
462+
expect(vs.$dropboxWrapper.getAttribute('aria-hidden'), 'aria-hidden on the reopened dropbox').to.not.equal('true');
463+
expect(vs.$dropboxContainer.getBoundingClientRect().height, 'reopened dropbox height').to.be.greaterThan(0);
485464
});
486465
});
487466

‎cypress/e2e/reopen-during-hide-transition.cy.ts‎

Lines changed: 179 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,179 @@
1+
/** cSpell:ignore vscomp popcomp */
2+
3+
/**
4+
* A reopen that arrives while the dropbox is still closing must still open it.
5+
*
6+
* The popover refuses to show while its own hide transition is running: `show()` early-returns
7+
* for as long as `pop-comp-active` is on the element, and only its `afterHide` removes that
8+
* class. So `openDropbox()` during the ~200ms fade did all of its own work - `beforeOpen`,
9+
* `aria-expanded="true"`, the instance back in `openInstances` - while `popper.show()` quietly
10+
* did nothing and `afterShowPopper()` never ran. The pending hide then completed on top of it.
11+
*
12+
* The dropdown ended up shut with its combobox still announcing `aria-expanded="true"`, no
13+
* `afterOpen` ever dispatched, and focus dropped to `<body>` (the popover applies
14+
* `display: none` before the component gets any callback, so nothing can hand it back). The
15+
* host's `open()` was silently lost - it only looks like nothing happened, which is the worst
16+
* shape for a bug to have.
17+
*
18+
* Reachable through the public `open()` API only: `toggleDropbox()` sees `isOpened() === true`
19+
* for the whole transition, so a second click closes again rather than reopening.
20+
*/
21+
22+
import { mountVs, unmountVs, makeOptions } from '../support/mount';
23+
24+
describe('Reopening while the dropbox is still closing', { testIsolation: true }, () => {
25+
const mountId = 'vs-reopen-during-hide';
26+
27+
const mount = (extra: Record<string, unknown> = {}) => {
28+
cy.viewport(1280, 800);
29+
cy.visit('get-started');
30+
cy.window().then((win) => {
31+
mountVs(win, mountId, { options: makeOptions(50), search: true, dropboxWrapper: 'body', ...extra });
32+
});
33+
};
34+
35+
const open = () => {
36+
cy.get(`#${mountId}`).find('.vscomp-toggle-button').click();
37+
cy.get(`#${mountId}`).find('.vscomp-wrapper').should('not.have.class', 'closed');
38+
};
39+
40+
/**
41+
* Close and reopen inside the same tick, so the reopen is guaranteed to land inside the hide
42+
* transition rather than racing a Cypress command hop. `isOpened()` is asserted between the
43+
* two to prove the transition really is still running - otherwise the case could pass by
44+
* testing an ordinary closed-then-open sequence.
45+
*/
46+
const closeThenReopen = () => {
47+
cy.get(`#${mountId}`).then(($ele) => {
48+
const vs = $ele[0].virtualSelect;
49+
50+
vs.closeDropbox();
51+
expect(vs.isOpened(), 'still mid hide-transition when the reopen arrives').to.equal(true);
52+
$ele[0].open?.();
53+
});
54+
};
55+
56+
/**
57+
* Settle first, then assert.
58+
*
59+
* Every assertion below also holds *during* the fade - `openDropbox()` sets aria-expanded and
60+
* clears aria-hidden synchronously, and the popover has not yet applied `display: none` - so a
61+
* retrying `should()` on its own would latch onto that transient and pass against the very bug
62+
* this spec exists for. The wait covers the hide (~200ms) plus the show that has to follow it.
63+
*/
64+
const assertOpenAndVisible = () => {
65+
cy.wait(700);
66+
cy.get(`#${mountId}`).find('.vscomp-wrapper').should('not.have.class', 'closed');
67+
cy.get(`#${mountId}`).should(($ele) => {
68+
const vs = $ele[0].virtualSelect;
69+
70+
expect(vs.isOpened(), 'isOpened()').to.equal(true);
71+
expect(vs.$dropboxContainer.style.display, 'container display').to.not.equal('none');
72+
expect(vs.$dropboxContainer.getBoundingClientRect().height, 'rendered height').to.be.greaterThan(0);
73+
expect(vs.$wrapper.getAttribute('aria-expanded'), 'aria-expanded').to.equal('true');
74+
expect(vs.$dropboxWrapper.getAttribute('aria-hidden'), 'aria-hidden').to.not.equal('true');
75+
expect(vs.$dropboxWrapper.getAttribute('tabindex'), 'tabindex').to.equal('0');
76+
});
77+
};
78+
79+
afterEach(() => {
80+
cy.window().then((win) => unmountVs(win, mountId));
81+
});
82+
83+
it('ends up open and on screen', () => {
84+
mount();
85+
open();
86+
closeThenReopen();
87+
88+
assertOpenAndVisible();
89+
});
90+
91+
/**
92+
* afterShowPopper() is where the dropdown finishes opening - the `focused` class, the scroll
93+
* position, and the focus move onto the search input. A reopen that never reaches it leaves a
94+
* dropdown nobody can type into.
95+
*/
96+
it('finishes the open: focus lands in the search input', () => {
97+
mount();
98+
open();
99+
closeThenReopen();
100+
101+
assertOpenAndVisible();
102+
103+
cy.get(`#${mountId}`).should(($ele) => {
104+
const vs = $ele[0].virtualSelect;
105+
106+
expect($ele[0].ownerDocument.activeElement, 'focus after the reopen').to.equal(vs.$searchInput);
107+
expect(vs.$wrapper.classList.contains('focused'), 'focused class').to.equal(true);
108+
});
109+
});
110+
111+
/**
112+
* One open() call is one open, however the reopen has to be sequenced internally. A host
113+
* loading options on beforeOpen must not be asked twice, and afterOpen has to arrive - it is
114+
* the only signal that the dropdown is actually usable.
115+
*/
116+
it('dispatches beforeOpen once and afterOpen once', () => {
117+
mount();
118+
open();
119+
120+
const events: string[] = [];
121+
122+
cy.get(`#${mountId}`).then(($ele) => {
123+
['beforeOpen', 'afterOpen', 'beforeClose', 'afterClose'].forEach((name) => {
124+
$ele[0].addEventListener(name, () => events.push(name));
125+
});
126+
});
127+
128+
closeThenReopen();
129+
assertOpenAndVisible();
130+
131+
/** let any late duplicate arrive before counting - dispatchEvent defers through setTimeout */
132+
cy.wait(300);
133+
cy.then(() => {
134+
expect(events.filter((name) => name === 'beforeOpen').length, `beforeOpen (${events.join(', ')})`).to.equal(1);
135+
expect(events.filter((name) => name === 'afterOpen').length, `afterOpen (${events.join(', ')})`).to.equal(1);
136+
});
137+
});
138+
139+
/** a close arriving after the queued reopen wins - the dropdown must not spring back open */
140+
it('does not reopen when a close follows the reopen', () => {
141+
mount();
142+
open();
143+
144+
cy.get(`#${mountId}`).then(($ele) => {
145+
const vs = $ele[0].virtualSelect;
146+
147+
vs.closeDropbox();
148+
$ele[0].open?.();
149+
vs.closeDropbox();
150+
});
151+
152+
cy.get(`#${mountId}`).find('.vscomp-wrapper').should('have.class', 'closed');
153+
cy.wait(600);
154+
cy.get(`#${mountId}`).should(($ele) => {
155+
const vs = $ele[0].virtualSelect;
156+
157+
expect(vs.isOpened(), 'isOpened() after the trailing close').to.equal(false);
158+
expect(vs.$dropboxWrapper.getAttribute('aria-hidden'), 'aria-hidden').to.equal('true');
159+
});
160+
});
161+
162+
/** the ordinary path must not be routed through the queue */
163+
it('still opens immediately when no hide is running', () => {
164+
mount();
165+
open();
166+
167+
cy.get(`#${mountId}`).find('.vscomp-toggle-button').click();
168+
cy.get(`#${mountId}`).find('.vscomp-wrapper').should('have.class', 'closed');
169+
170+
cy.get(`#${mountId}`).then(($ele) => {
171+
$ele[0].open?.();
172+
173+
/** synchronous: an open with nothing to wait for must not be deferred */
174+
expect($ele[0].virtualSelect.isOpened(), 'open applied in the same tick').to.equal(true);
175+
});
176+
177+
assertOpenAndVisible();
178+
});
179+
});

‎src/virtual-select.js‎

Lines changed: 55 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -1404,6 +1404,8 @@ export class VirtualSelect {
14041404
this.uniqueId = this.getUniqueId();
14051405
this.shouldFocusWrapperOnClose = true; // Initialize focus management property
14061406
this.isClosing = false;
1407+
/** an open that arrived mid hide-transition and is waiting for it - see openDropbox() */
1408+
this.pendingOpen = false;
14071409
/** true from closeDropbox() until the next openDropbox() - see closeDropbox() */
14081410
this.isSilentServerSearch = false;
14091411
this.ariaSetSize = 0;
@@ -2993,6 +2995,30 @@ export class VirtualSelect {
29932995
openDropbox(isSilent) {
29942996
// Set this instance as the last interacted one immediately
29952997
VirtualSelect.lastInteractedInstance = this;
2998+
2999+
/**
3000+
* Queue the open when the popover is still running its hide transition, and let
3001+
* afterHidePopper() replay it once the popover is idle again.
3002+
*
3003+
* PopoverComponent.show() early-returns for as long as `pop-comp-active` is on the element,
3004+
* and only its own afterHide removes that class - so opening during the ~200ms fade used to
3005+
* do all of this method's work while popper.show() quietly did nothing. afterShowPopper()
3006+
* never ran (no `focused` class, no focus moved into the list, no afterOpen), and the hide
3007+
* that was still pending then completed on top of it: the dropdown ended up shut with its
3008+
* combobox announcing aria-expanded="true". The host's open() was silently lost.
3009+
*
3010+
* Both conditions are required. isClosingTransition alone would strand every later open if
3011+
* it were ever left set with no hide actually pending; isShown() alone is true of a dropdown
3012+
* that is simply already open, where re-opening must stay a no-op rather than a queued one.
3013+
* Together they mean exactly "a hide is running and the popover will refuse to show".
3014+
*/
3015+
if (!isSilent && this.isClosingTransition && this.dropboxPopover && this.dropboxPopover.isShown()) {
3016+
this.pendingOpen = true;
3017+
3018+
return;
3019+
}
3020+
3021+
this.pendingOpen = false;
29963022
let originalTransition = '';
29973023
// Disable transitions for programmatic opening
29983024
if (!isSilent) {
@@ -3097,6 +3123,9 @@ export class VirtualSelect {
30973123
// Remove from open instances
30983124
VirtualSelect.openInstances.delete(this);
30993125

3126+
/** a close supersedes an open still queued behind the running hide - see openDropbox() */
3127+
this.pendingOpen = false;
3128+
31003129
if (this.isOpened() === false) {
31013130
return;
31023131
}
@@ -3224,7 +3253,11 @@ export class VirtualSelect {
32243253

32253254
afterHidePopper() {
32263255
const isSilent = this.isSilentClose;
3256+
/** read before anything can queue a new one, and cleared either way - see openDropbox() */
3257+
const shouldReopen = this.pendingOpen;
3258+
32273259
this.isSilentClose = false;
3260+
this.pendingOpen = false;
32283261
this.isClosingTransition = false;
32293262

32303263
DomUtils.removeClass(this.$allWrappers, 'focused');
@@ -3260,23 +3293,25 @@ export class VirtualSelect {
32603293
this.shouldFocusWrapperOnClose = true;
32613294

32623295
/**
3263-
* Stand down if a focus handler above reopened the dropdown synchronously: openDropbox()
3264-
* has already made the dropbox visible and focusable again, and hiding it now would leave
3265-
* it on screen but absent from the accessibility tree. The next close re-applies these
3266-
* through its own afterHidePopper().
3267-
*
3268-
* Deliberately isOpened() and not `VirtualSelect.openInstances.has(this)`, even though the
3269-
* latter also means "reopened". The two differ for a hide that lost a race with an earlier
3270-
* reopen - `open()` called during the fade, which does not cancel the popover's pending
3271-
* hide because its show() early-returns while `pop-comp-active` is still set. That hide
3272-
* has to *complete*: the popover has already taken the dropbox off the screen by the time
3273-
* it calls back, so standing down would leave the wrapper without its `closed` class,
3274-
* `isOpened()` true against an invisible dropbox, and every later toggle closing a
3275-
* dropdown that is not there - unreopenable. Verified by building it: the openInstances
3276-
* form fails the recovery assertions in a11y-aria-hidden-focus.cy.ts.
3296+
* The hide has finished, so the popover is idle and will accept a show again: replay the
3297+
* open that arrived while it was still running. Placed before the guard below rather than
3298+
* after it, so the reopen goes through the one path that already knows not to hide a
3299+
* dropdown that is open - openDropbox() leaves isOpened() true, and the guard stands the
3300+
* hiding writes down for it exactly as it does for a synchronous focus-handler reopen.
3301+
*/
3302+
if (shouldReopen && !this.isDestroyed) {
3303+
this.openDropbox();
3304+
}
3305+
3306+
/**
3307+
* Stand down if the dropdown is open again by the time these writes would run - either the
3308+
* replay above, or a focus handler that reopened it synchronously from the wrapper refocus.
3309+
* openDropbox() has already made the dropbox visible and focusable in both cases, and
3310+
* hiding it now would leave it on screen but absent from the accessibility tree. The next
3311+
* close re-applies all of this through its own afterHidePopper().
32773312
*
3278-
* isOpened() is false here for that stale case (addClass above set it) and true only for
3279-
* the synchronous reopen this guard is for, which is exactly the split that is wanted.
3313+
* A state read rather than a record of what happened, and evaluated last, so it cannot
3314+
* disagree with the reopen paths the way an "did someone call open()" flag would.
32803315
*/
32813316
if (this.isOpened()) {
32823317
return;
@@ -3507,11 +3542,10 @@ export class VirtualSelect {
35073542
* add - a stale hide should not yank focus out of a dropdown the user is looking at - but a
35083543
* reopen that genuinely put the dropbox back on screen cannot have a hide still pending
35093544
* against it: the popover clears `pop-comp-active` before calling back, so a successful
3510-
* show() means the hide already finished. What such a guard actually suppresses is the one
3511-
* case that needs the release most - the reopen that lost its race with the fade, where the
3512-
* popover has already applied display:none and focus inside the dropbox is about to be lost
3513-
* to <body>. The caller decides whether the dropbox is being hidden; this only makes sure
3514-
* nothing is focused inside it when that happens.
3545+
* show() means the hide already finished, and an open arriving before that is queued rather
3546+
* than applied (see openDropbox). All such a guard could do is suppress the release on a
3547+
* path where the dropbox is being hidden anyway. The caller decides whether the dropbox is
3548+
* being hidden; this only makes sure nothing is focused inside it when that happens.
35153549
*
35163550
* preventScroll because this runs ~200ms after the user's action - if they have scrolled in
35173551
* the meantime, focus restoration must not scroll the combobox back into view (the

0 commit comments

Comments
 (0)