Keep terminal-context detail-dialog buttons focusable while their action is in flight - #574
Conversation
Promote, Save default and Discard and reset passed `busy` to native `disabled`. The browser blurs a disabled element, so activating one moved focus to `<body>`; on the failure path the dialog stays open and focus never returns. The context's Escape and Tab handling both live on the `<section>`'s `onKeyDown` and need a focused descendant to bubble from, so a rejected edit left the `aria-modal` dialog with no Escape and no Tab trap. They now use the `busy` prop introduced in ca86651 for the launch button, which marks the button `aria-disabled` and drops its click while leaving it in the tab order. Two follow-ons from the same mechanism: `disabled:opacity-40` is unconditional again, since `busy` no longer feeds `disabled` and the ternary could only fire on a genuinely disabled button; and the `enabled:hover:*` interaction classes are withheld while busy, because `:enabled` no longer excludes a button that is dropping clicks.
Deploying mouseterm with
|
| Latest commit: |
c2e01f3
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://fefc1fbe.mouseterm.pages.dev |
| Branch Preview URL: | https://fix-context-dialog-focus-bus.mouseterm.pages.dev |
dormouse-bot
left a comment
dormouse-bot
left a comment
There was a problem hiding this comment.
Two of the three changes land clean: busy no longer reaching native disabled, and disabled:opacity-40 going back to unconditional. The hover withdrawal has a problem in both directions, from one root cause — :enabled is now the wrong gate for "not accepting clicks", and withholding the whole SUBTLE_ACTION_INTERACTION_CLASS is too blunt one way and not enough the other.
The focus ring goes with the hover styling. SUBTLE_ACTION_INTERACTION_CLASS in lib/src/components/design.tsx is enabled:hover:bg-current/10 focus-visible:outline focus-visible:outline-focus-ring — only the hover half carries the enabled: gate. Putting !busy && in front of the whole string drops the outline from every in-flight ContextAction, which is exactly the button this PR keeps focused. On main the Opening… launch button #573 introduced keeps its ring for the whole flight; after this it loses it. Concretely: Tab to Save default, press Enter, and the focus ring pops off for the duration of the flight and returns when the host rejects. SUBTLE_ACTION_COLOR_CLASS's enabled:focus-visible:text-link still fires (:enabled is true on an aria-disabled button), so it degrades to colour-only rather than nothing — but stacked on giving up disabled:opacity-40 for these three, an in-flight Save default ends up with no visual state change of any kind, while aria-busy and aria-disabled tell a screen reader it is busy and unavailable. That widens the in-flight-dim question the description leaves open rather than leaving it where it was.
And the hover styling isn't actually withheld. SUBTLE_ACTION_COLOR_CLASS carries enabled:hover:text-link, gated the same way, and it is applied unconditionally through color on the line above. Hovering an in-flight button still tints it link-coloured, so the second follow-on holds for the background but not the text.
Both fall out if the gate moves into the constants and off the call site:
// design.tsx
export const SUBTLE_ACTION_COLOR_CLASS = `${SUBTLE_ACTION_REST_COLOR_CLASS} enabled:not-aria-disabled:hover:text-link enabled:focus-visible:text-link`;
export const SUBTLE_ACTION_INTERACTION_CLASS = 'enabled:not-aria-disabled:hover:bg-current/10 focus-visible:outline focus-visible:outline-focus-ring';with line 40 reverting to ${windowFocused ? SUBTLE_ACTION_INTERACTION_CLASS : ''}. OnOffSwitch is the only other consumer of either constant and never sets aria-disabled, so it is unaffected. If not-aria-disabled: doesn't generate what you expect on Tailwind 4.3, the fallback is exporting the focus-visible: half separately and selecting it when busy — but that fixes the ring only and leaves the colour hover as is.
The previous commit withheld SUBTLE_ACTION_INTERACTION_CLASS entirely while busy. That was wrong in both directions: - The constant's `focus-visible:outline` half carries no `enabled:` gate, so dropping the whole string also dropped the focus ring from every in-flight ContextAction -- including the button this PR exists to keep focused, and the `Opening...` launch button, which keeps its ring on main. - SUBTLE_ACTION_COLOR_CLASS carries `enabled:hover:text-link` and is applied unconditionally through `color`, so hovering an in-flight button still tinted it link-coloured. The hover withdrawal only ever covered the background. Both fall out of moving the gate into the constants: `:enabled` is the wrong test for "not accepting clicks" now that busy means `aria-disabled`, so the two hover rules become `enabled:not-aria-disabled:hover:*` and the call site goes back to keying only on window focus. Tailwind 4.3.3 compiles the variant to `:enabled:not([aria-disabled="true"]):hover`. OnOffSwitch is the only other consumer of either constant and never sets aria-disabled, so it is unaffected. A regression test pins both halves: every hover class on an in-flight button carries the aria-disabled gate, and the focus-ring classes survive the flight. Reverting either half turns it red. Also fixes the vacuous Tab assertion in the previous commit's test, which asserted the dialog contains document.activeElement while the line above had already pinned activeElement to a button inside it -- a no-op Tab handler passed. It now asserts focus moved off the button and stayed inside. It does not name the landing element: jsdom returns a comma-separated selector list grouped by selector rather than in document order, so the trap's wrap target under test differs from a real browser's.
|
Both halves confirmed and fixed as suggested in 77f635e — the gate moved into the constants and line 40 went back to keying only on window focus. export const SUBTLE_ACTION_COLOR_CLASS = `${SUBTLE_ACTION_REST_COLOR_CLASS} enabled:not-aria-disabled:hover:text-link enabled:focus-visible:text-link`;
export const SUBTLE_ACTION_INTERACTION_CLASS = 'enabled:not-aria-disabled:hover:bg-current/10 focus-visible:outline focus-visible:outline-focus-ring';
A regression test pins both directions: every hover class on an in-flight button carries the On the widened dim question: the PR description now states the resulting in-flight state rather than leaving it implicit — focus ring plus |
dormouse-bot
left a comment
dormouse-bot
left a comment
There was a problem hiding this comment.
Both prior findings hold on 77f635e, checked against the tools rather than the description: enabled:not-aria-disabled:hover:bg-current/10 compiles under tailwindcss 4.3.3 to :enabled:not([aria-disabled="true"]):hover (inside the usual @media (hover: hover)), and jsdom 29.1.1 really does return querySelectorAll('button:not(:disabled),input,select') grouped by selector — [Close details, Reset helper…, Save default, input] for this dialog's markup — so the Close details suggestion would have gone red. OnOffSwitch is the only other consumer of either constant and sets no aria-disabled.
One new thing. docs/specs/layout.md now understates the rule this PR changed:
Must suppress context action hover and focus highlights while the window is unfocused, including after opening a native explorer or system browser.
That rule owns context-action hover suppression, and there is now a second suppression condition it doesn't state: hover is withheld from any action that is dropping clicks, window focus aside. The neighbouring Must show "Opening…" rule carries preserving width and keyboard focus while blocking repeat clicks for the directory-explorer button alone, so nothing in the spec tells a reader the three detail-dialog buttons are held to the same thing, and nothing there stops an editor putting disabled={busy} back and breaking Escape from controls closes the innermost disclosure, then context. three rules up. Per AGENTS.md: "When code covered by a spec changes, change the spec." The two code comments cover the two modules; this one is for the spec's reader.
Appending to the unfocused-hover rule keeps it in the section that owns it:
**Must suppress context action hover and focus highlights while the window is unfocused**, including after opening a native explorer or system browser. **Must also withhold hover from an in-flight action, which stays focusable and `aria-disabled` rather than `disabled`** so the innermost disclosure keeps a focused descendant for Escape and Tab.I applied that locally to check the budget: layout.md goes 7763 → 7791 words against its 7800, and node scripts/spec-lint.mjs stays green, so no --ratchet needed. (Reverted; nothing pushed.)
The rest reads clean. disabled:opacity-40 going unconditional restores the dim on the one button that could be disabled and busy at once, every Prototypes/Terminal context story resolves its handlers inline so Chromatic has nothing in flight to snapshot, and the in-flight dim is a maintainer call as you framed it.
The unfocused-hover rule in docs/specs/layout.md now also covers an in-flight action, which stays focusable and aria-disabled rather than disabled. The regression test's hover loop passed vacuously on an empty filter, so deleting the hover classes outright stayed green; it now asserts the filter is non-empty.
|
Spec finding taken as written —
The inline comment is answered on its own thread; both changes rode the one push. |
2 participants
In the terminal context's detail dialogs,
Promote,Save defaultandDiscard and resetpassedbusystraight to nativedisabled. A browser blurs an element the moment it becomes disabled, so activating one of them moved focus to<body>; on the failure pathsubmitskipssetDetail(null), so the dialog stays open and focus never comes back. Escape and the Tab trap both live on the<section>'sonKeyDownand need a focused descendant to bubble from, so a rejected edit left thearia-modaldialog with neither. They now use thebusyprop #573 added for the launch button, which marks the buttonaria-disabledand drops its click while keeping it in the tab order.Verified with a regression test that holds
onModifypending, rejects it, and asserts focus stays onSave defaultacross the whole flight, that Tab still moves focus and keeps it inside the dialog, and that Escape closes the dialog rather than the context. A second test pins the styling contract. Reverting any half of the fix turns one of them red.libtypecheck and all 2413 tests pass on77f635e34a0210e516f32e9e885448d82da58518.Route, and two follow-ons folded in
Open Modify, paste a multi-line command, Tab to
Save default, press Enter. The host rejects it (docs/specs/terminal-context.mdassigns command validation to the host and requires the rejected edit to show its error in Modify), the error renders in the dialog, and from there Escape no longer closes it and Tab walks into the page behind it.Promotehas the same shape viaCould not split the source pane, whichlath.store.addLeafreturns for a pane too small to split — recoverable by mouse, but the keyboard path dead-ends on the error banner.Both other sites keyed on the same
busy/disabledconflation, so they are corrected here too:disabled:opacity-40was behind abusy ? '' :ternary. Now thatbusynever feedsdisabled, the ternary could only fire when both were true — rendering a genuinely disabled button undimmed. It is unconditional again.enabled:variants, and:enabledno longer excludes a button that is dropping clicks, so anaria-disabledbutton took hover styling. The gate now lives in the constants themselves asenabled:not-aria-disabled:hover:*, rather than at the call site. Withholding the wholeSUBTLE_ACTION_INTERACTION_CLASSwhile busy — the first shape of this fix — was wrong in both directions: it also dropped the ungatedfocus-visible:outlinehalf from the very button this PR keeps focused, and it missedSUBTLE_ACTION_COLOR_CLASS'senabled:hover:text-link, which is applied unconditionally throughcolor. Tailwind 4.3.3 compiles the variant to:enabled:not([aria-disabled="true"]):hover, andOnOffSwitch— the only other consumer of either constant — never setsaria-disabled.No Chromatic movement expected: every
Prototypes/Terminal contextstory resolves its handlers immediately, so no snapshot holds a button in flight, and the hover change is pointer-only.This landed on
mainin #573 — I raised it on that thread about a minute before the merge, so it likely went unseen rather than being waved through.The one open choice is the in-flight dim. Passing
busygives updisabled:opacity-40on these three, which this PR accepts — an in-flightSave defaultkeeps its focus ring and itsaria-busy/aria-disabledannouncement, but no dim. Restoring it needs anaria-disabled:opacity-40variant onContextAction, which would also dim theOpening…button and move the approved visual baseline — say the word and I'll push that shape instead.