fix(a11y): stop blade loading from unmounting the control that has focus - #301
Merged
Conversation
Saving in a blade dropped focus to <body>, so the next Tab restarted from the top of the document (WCAG 2.4.3 Focus Order). The cause is not the Save button disabling itself -- ToolbarBaseButton never sets native `disabled`, only a class and a guard. It is that `loading` is provided blade-wide (vc-blade.vue) and every control swaps itself for a skeleton: the toolbar buttons in BladeToolbar, and every field through vc-input's `bladeLoading` branch. A save raises that flag, so the focused element is unmounted. It takes the field out from under a user who was typing too, which is why Ctrl/Cmd+S loses focus the same way. A skeleton stands in for content that has not rendered yet. Once the blade has shown real content, `loading` means something is in flight, and replacing the controls then is wrong on its own terms -- design systems keep the control mounted and show its pending state in place. So the skeleton is now available exactly once, before the first render, and `loading` after that is reported through aria-busy plus each control's own pending state (ToolbarBaseButton already has one). This needs no module changes: consumers keep passing a single `loading`. Sign-in was a second, unrelated failure. The route watcher in vc-app could never see it: `/login` and the shell are sibling routes, so the watcher is created after that navigation has finished, and the workspace mounts later still -- when the app reports ready -- without a further route change. The workspace element appearing is the signal, so watch that instead. The route watcher stays for route changes within a mounted shell. Measured on a local vendor-portal against a real API, reading document.activeElement: after sign-in <body> -> main.vc-app__workspace after Save <body> -> button.vc-blade-toolbar-base-button (POST 200) Blade open and Maximize/Restore already held focus and still do. Not covered: mobile. `useToolbarRegistration` filters disabled items out of the toolbar on mobile only, so a Save that disables itself is removed from the DOM by a different route than the skeleton. Left alone because showing disabled items on mobile is a product change, not an a11y repair. Relates to VCST-5670
|
📦 Preview published for commit Install the preview with dist-tag: npm install @vc-shell/framework@pr-301Or pin to the exact commit: npm install @vc-shell/framework@2.4.0-pr301.783db97Published packages (dist-tag
|
Completes VCST-5670.
Making the skeleton one-shot fixed the focus loss but went one step too
far: a blade instance that switches to a different entity in place then
rendered the second entity with no loading skeleton at all, because the
latch had already been set by the first.
The latch now resets on a change of entity context, keyed on
`BladeDescriptor.param`. Query state and descriptor identity are
deliberately excluded, so paging, filtering and sorting cannot re-arm it.
The distinction that matters: a save-style loading cycle -- same entity,
`loading` raised again -- must NOT re-arm the skeleton, or this change
would silently restore the very bug the branch fixes. Both directions are
now pinned by tests.
Verified live on a local vendor-portal against a real API:
Save (Ctrl/Cmd+S) 0 skeleton mutations, focus stays on the toolbar
button, fields keep their DOM nodes
Entity switch skeleton re-appears for the newly opened offer
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.
Addresses VCST-5670. Two unrelated failures behind one symptom — focus dropped to
<body>, so the next Tab restarted from the top of the document (WCAG 2.4.3 Focus Order).1. Save
Not the Save button disabling itself.
ToolbarBaseButtonnever sets nativedisabled— only a class and a guard inhandleClick. Verified: after a save the button isdisabledClass: true, nativelyDisabled: false.The real cause:
loadingis provided blade-wide fromvc-blade.vue, and every control swaps itself for a skeleton — toolbar buttons inBladeToolbar.vue:11, and every field throughvc-input.vue:4'sbladeLoadingbranch. A save raises that flag, so the focused element is unmounted. That also takes the field out from under a user who was typing, which is whyCtrl/Cmd+Sloses focus by the same route.A skeleton stands in for content that has not rendered yet. Once the blade has shown real content,
loadingmeans something is in flight, and replacing the controls then is wrong on its own terms — design systems keep the control mounted and show its pending state in place (LoadingButtonin MUI,loadingon Ant Design's Button, spinner-in-button in Bootstrap and Carbon). Skeletons are for initial load.So
useBladeSkeletonmakes the skeleton available exactly once, before the first render of real content. After that,loadingis reported througharia-busyand each control's own pending state —ToolbarBaseButtonalready hasisWaiting.No module changes. Consumers keep passing a single
loadingprop.2. Sign-in
The route watcher in
vc-app.vuecould never catch it:/loginand the shell are sibling routes, so the watcher is created after that navigation has finished, and the workspace mounts later still — when the app reports ready — without a further route change. The workspace element appearing is the signal, so watch that. The route watcher stays for route changes within a mounted shell.Measured, not assumed
Local vendor-portal against the real API, reading
document.activeElementat each transition:<body>main.vc-app__workspace[aria-label="Restore"]<body>button.vc-blade-toolbar-base-buttonMy first Save measurement was void and discarded — the button was still
--disabled, so the click hit the early return and nothing saved. The form has to be dirtied first for the measurement to mean anything.Verification
useBladeSkeleton, 2 for the sign-in watcher (including one that pins it not stealing focus when something already holds it) — all written first and observed failingvitest run ui/components/organisms/vc-app ui/components/organisms/vc-blade— 399 passed / 42 files. TheBladeToolbartests that assert skeleton replacement and mobile removal still pass: they exercise the initial load, which is unchanged.useConnectionStatusanduseNotificationStore, both pre-existing 5s timeouts under parallel load that pass in isolation and fail identically without this change.yarn typecheckexit 0; eslint, prettier,check:locales,check:layerspassNot covered
Mobile.
useToolbarRegistrationfilters disabled items out of the toolbar on mobile only, so a Save that disables itself is removed from the DOM by a route the skeleton latch does not touch. Showing disabled items on mobile is a product change rather than an a11y repair, so it is left alone — flagging it rather than burying it. The mobile measurement itself is also unverified: the mobile toolbar hides items behind an expandable menu and needs a different driver.Update: second commit — re-arm the skeleton when the entity changes
Found while re-verifying the open VCST board. The one-shot latch fixed the focus loss but went one step too far: a blade instance that switches to a different entity in place then rendered the second entity with no loading skeleton at all, because the latch had already been set by the first.
The latch now resets on a change of entity context, keyed on
BladeDescriptor.param. Query state and descriptor identity are deliberately excluded, so paging, filtering and sorting cannot re-arm it.The distinction that matters: a save-style loading cycle — same entity,
loadingraised again — must not re-arm the skeleton, or this change would silently restore the very bug this PR fixes. Both directions are now pinned by tests.Re-verified live on a local vendor-portal against a real API
404 files / 3798 testsgreen on this branch alone.