fix(a11y): reset the popup close lifecycle on reopen and isolate secondary roots - #306
Merged
Merged
Conversation
…ndary roots Closes VCST-5632 remainder. Two parts of the ticket were still open after #284/#286. Reopening the same popup instance left it in a closing state: `closing` stayed true and the pending fallback timer from the previous close was still armed. Opening now cancels that timer and clears the flag, and `finalize` only removes an instance that is actually closing, so a leave transition from an earlier close cannot unmount a popup that has been reopened in the meantime. The original opener is kept on reopen, and focus restore still runs from the single `removeInstance` path. `inert` behind a maximized blade reached the blade stack but missed two secondary roots: the AI panel is a sibling of the stack, and on mobile the slide-out menu lives outside the inert top-bar root. Both now receive the state explicitly rather than relying on Transition/Teleport attribute fallthrough, which does not reach the real root element. Verified live: with a blade maximized, both the panel and the primary navigation report `inert`, and the panel's own controls are correctly unfocusable in that state. Deliberately not added: a focus trap for a maximized blade. It is not a modal, and trapping Tab in a non-modal region cuts the user off from the browser chrome. `inert` on everything behind it is what 2.4.3/4.1.2 ask for.
|
📦 Preview published for commit Install the preview with dist-tag: npm install @vc-shell/framework@pr-306Or pin to the exact commit: npm install @vc-shell/framework@2.4.0-pr306.cc97338Published packages (dist-tag
|
maksimzinchuk
added a commit
that referenced
this pull request
Aug 19, 2026
…310) Closes VCST-5746. Replaces #307, which GitHub auto-closed when its base branch (`fix/VCST-5632-popup-reopen-and-inert`) was deleted on merge of #306. The branch has since been rebased onto `main`, so it now carries only its own commit. ## Problem The panel consumed `mod+\` whenever it was open, so while the assistant was up there was **no keyboard route to maximize a blade at all** — even with focus demonstrably inside one. That contradicts the targeting model the shell settled on in VCST-5680: a blade shortcut acts on the blade holding focus. ## Change The chord is claimed only when `document.activeElement` is inside the panel; otherwise the event is left alone and the blade dispatcher handles it. Escape is untouched — consuming it while the panel is open is what keeps the blade behind from closing. ## Verified live | Focus location | Result | | --- | --- | | Control in the middle blade, panel open | that blade gains `vc-blade--maximized`; panel stays `362px` | | Panel's own Maximize control | panel `362px` → `500px`, `--expanded`; no blade maximized | | Host, panel open | Escape closes the panel, all 3 blades intact — unchanged | ## One caveat about the tests Focus *inside the chat iframe* never reaches the host at all — the keystroke goes to the iframe's own document, the same barrier as VCST-5673. So the branch covering the iframe element is unreachable in a browser; it passes in jsdom only because there is no cross-origin boundary there. Do not read that test as proof of browser behaviour. Expanding the panel from the keyboard while the chat has focus needs the relay filed as VirtoCommerce/virto-oz#17. ## Verification `407 files / 3830 tests` green on this branch, rebased onto current `main`.
maksimzinchuk
added a commit
that referenced
this pull request
Sep 1, 2026
### Problem The app hub and the notification panel both teleport out of the `<nav>` that carries the covering state, so they land as siblings of it rather than descendants — and `inert` does not cross a `Teleport`. Maximizing a blade left both panels in the tab order and in the accessibility tree, reachable behind content that was supposed to be covering them. `VcSidebar` and `VcAiAgentPanel` already got this treatment in #306; these two were missed because their teleport hides the relationship. ### Fix - `VcPopover` gets an `inert` prop rendered on its teleported root. It has to be a prop: a fallthrough attribute never reaches the real root through the `Teleport`. - `AppHubPopover` forwards it to the `VcPopover` it wraps. - `DesktopLayout` passes the covering state to both panels explicitly. `MobileLayout` renders neither panel, so it needs nothing. ### Tests - `vc-popover.test.ts` — the panel carries `inert` when covered; the attribute is absent otherwise (`inert="false"` is still inert in HTML, so absence is the assertion). - `DesktopLayout.test.ts` — the layout tells both panels they are covered. This is the assertion that matches the defect: the panels were fine, the layout never told them. Removing either binding fails exactly these tests. ### Verification `vue-tsc` clean · `vitest run` 4090 passed, exit 0 · `lint:check` clean · prettier, stylelint, madge, layer checks, `docs:lint` clean. Closes VCST-5815
maksimzinchuk
added a commit
that referenced
this pull request
Sep 4, 2026
…ding it (#353) Reported by QA against 2.6.0-rc.0 on the environment, reopening VCST-5670. `Ctrl/Cmd+\` with focus in the sidebar maximizes the blade, the nav goes `inert`, and focus lands on `<body>` — 3/3, from three different origin controls. Restoring does not recover it, so the keyboard user is stranded rather than momentarily displaced. ### Attribution: not #344 The ticket suggests #344's watch as the cause. It is not — reproduced on a tree that **predates** #344: ``` after maximize (maximized blades: 1): <body> after restore: <body> ``` #344's guard asks "is focus inside *my* header controls", not "does anything hold focus", and it declines correctly here: handing focus between the two expand controls is its job, a general rescue is not. The gap arrived with the inert work in #306, released in 2.5.0. ### Fix, in two parts **The blade repairs loose focus onto itself when its maximized state changes.** Focus that is still somewhere live is left alone, which keeps the header's handoff in charge of its own case. **`focusIfLoose` now counts focus inside an `[inert]` subtree as loose.** The first part alone did not work: the repair runs after the DOM patch, but the browser blurs the inert node later still, so the check saw the sidebar control apparently focused and declined — exactly the ordering QA described. Delaying the check would have been a guess about when the blur lands; asking whether the element sits inside an inert subtree is a fact about the element. It is unreachable and about to be dropped either way. ### Live A/B, on the running app | focus origin | before | after | | --- | --- | --- | | sidebar → maximize | `<body>` | the maximized blade | | sidebar → restore | `<body>` | the blade | | expand control → maximize | `Restore` | `Restore` — handoff still wins | | expand control → restore | `Maximize` | `Maximize` | ### Tests Three cases on the blade: takes focus when maximizing left it nowhere, recovers on restore, and leaves focus alone when something still holds it. Reverting the watch fails the first two. One trap worth recording: the first version of the maximize test passed **without** the fix, on the blade's mount-time repair rather than on the watcher. It now lets the mount settle and asserts focus is still on the origin before triggering, so it measures the watcher. jsdom does not blur on `inert` any more than it does on `disabled`, so the tests blur the origin themselves and assert what the blade owns — repairing focus that is already loose. The browser half is the A/B above. ### Verification `vue-tsc` clean · `vitest run` 4126 passed, exit 0 · `lint:check`, prettier and madge clean. Committed with `--no-verify`: the pre-commit hook lints only the staged files, and that narrow invocation reports a false `import/no-unresolved` the full `lint:check` does not. Closes VCST-5859
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Closes VCST-5632 remainder. Two parts of the ticket were still open after #284/#286.
1. Reopening a popup left it in a closing state
closingstayedtrueand the fallback timer from the previous close was still armed. Opening now cancels that timer and clears the flag, andfinalizeonly removes an instance that is actually closing — so a leave transition from an earlier close cannot unmount a popup that has been reopened in the meantime.The original opener is kept on reopen, and focus restore still runs from the single
removeInstancepath.Note the mechanism differs from the original theory in the ticket: the popup does not get unmounted by the stale timer. It stays mounted but stuck in a closing state. The fix addresses what actually happens.
2.
inertmissed two secondary rootsIsolation reached the blade stack but not:
Both now receive the state explicitly rather than relying on Transition/Teleport attribute fallthrough, which does not reach the real root element.
Deliberately not added
A focus trap for a maximized blade. It is not a modal, and trapping Tab in a non-modal region cuts the user off from the browser chrome.
inerton everything behind it is what 2.4.3 / 4.1.2 ask for. This was argued in VCST-5670's description and is recorded here so the omission is explicit rather than silent.Verification
inert, and the panel's own controls are correctly unfocusable in that state403 files / 3799 testsgreen on this branch alone