amadan
fix(chrome): four defects an independent review found in the phone pass
Reviewed by GPT-5.1-Codex against the branch. All four are real; the first was a hard functional break. FILL AND TEXT COLOUR WERE UNREACHABLE ON A PHONE. The "..." carried name="rst-tools", the same exclusive group as the two colour menus — which sit INSIDE the panel it reveals. Opening Fill colour closed the disclosure, which hid the panel, which took the colour menu down with it. Same shape from the other side: rst-dropdown put the control in rastrillo.js's MENUS, whose click-away closes any open menu that does not CONTAIN the click, and every tap on a swatch or an alignment button is outside a control whose panel is its SIBLING — so the panel also shut under the finger on every single use, and Escape from inside it left focus on an element no longer rendered. One root cause: the panel has to be a sibling (a closed <details> hides its own content, and the wide layout cannot be recovered from inside one), and every containment check in the framework asks the wrong question about a control shaped like that. So it is a bare <details> now, invisible to rastrillo.js on purpose, with click-away and Escape in static/toolbar.js that test containment against the whole strip. THE ARROW KEYS WALKED HIDDEN CONTROLS. commands.js's roving tabindex took every [data-tool] with no visibility filter, so right-arrow from Italic parked the strip's only tab stop on the hidden Fill colour summary and called focus() on it, which does nothing — after which Tab skipped the toolbar entirely. It now walks what is on screen, and normalises the stop instead of trusting that whatever set one demoted the rest. THE CLIPPED BRAND WAS AN INVISIBLE FOCUS STOP. Below 800px the app name was hidden with the 1px clip [rst-skip] uses. [rst-skip] can do that because it un-clips on :focus-visible; this did not, so a keyboard user tabbing the bar landed on a control one pixel square with nothing to reveal it (SC 2.4.7). It is display: none now — the route it carried is in the collapsed tail as "Your sheets", and page.go redirects / to /sheets for anyone signed in, so nothing is lost by it leaving the tab order. And the toolbar work moved to static/toolbar.js: commands.js reached 378 lines against the 300-line cap (guardstest/jscap_test.go). It is the seam the Docs app already draws — commands.js decides what a control does, this decides which one the keyboard is on and which are on screen. Verified in Chromium against a real instance at 390px: the overflow and a colour menu open together with the swatches visible and on screen; a tap inside the panel leaves it open and a tap on a menu bar title closes it; exactly one tab stop, visible, panel open and closed; Escape closes and lands focus on the visible "More formatting" summary. The strip is 36px at 320, 390, 481, 560, 660, 900, 1000 and 1280 with no horizontal scroll at any of them. Full Go suite and guardstest pass.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014jpLiN9xwBrZ5dJw92aYmF
4 files changed,
+185
−47
internal/sheets/static/commands.js+0 −25internal/sheets/static/sheets.css+19 −16internal/sheets/static/toolbar.js+132 −0internal/sheets/templates/sheet.html+34 −6
CI passed — run details
Intent
diff --git a/internal/sheets/static/commands.js b/internal/sheets/static/commands.js| index 6dfb90a..48c701c 100644 |
| --- a/internal/sheets/static/commands.js |
| +++ b/internal/sheets/static/commands.js |
| @@ -264,31 +264,6 @@ |
| el.setAttribute('data-set', ''); |
| } |
| - /* ---- the toolbar is one tab stop --------------------------------- |
| - * |
| - * Fourteen buttons, fourteen tab stops, is a strip a keyboard user |
| - * has to walk through to reach the grid. Roving tabindex with arrow |
| - * keys is the convention for a toolbar, and matches what the menu |
| - * bar beside it does. */ |
| - var strip = document.querySelector('[data-toolbar]'); |
| - if (!strip) return; |
| - var stops = Array.prototype.slice.call(strip.querySelectorAll('[data-tool]')); |
| - if (!stops.length) return; |
| - stops.forEach(function (el, i) { el.tabIndex = i === 0 ? 0 : -1; }); |
| - |
| - strip.addEventListener('keydown', function (e) { |
| - if (e.key !== 'ArrowLeft' && e.key !== 'ArrowRight') return; |
| - var i = stops.indexOf(e.target.closest('[data-tool]')); |
| - if (i < 0) return; |
| - e.preventDefault(); |
| - /* Jspreadsheet listens for arrows on document and would walk the |
| - * cell cursor as well. */ |
| - e.stopPropagation(); |
| - var next = stops[(i + (e.key === 'ArrowRight' ? 1 : -1) + stops.length) % stops.length]; |
| - stops.forEach(function (el) { el.tabIndex = -1; }); |
| - next.tabIndex = 0; |
| - next.focus(); |
| - }); |
| } |
| if (window.SheetsGrid) ready(); |
diff --git a/internal/sheets/static/sheets.css b/internal/sheets/static/sheets.css| index d403b1e..f15a9bb 100644 |
| --- a/internal/sheets/static/sheets.css |
| +++ b/internal/sheets/static/sheets.css |
| @@ -89,10 +89,23 @@ |
| * condition — "there is already a title here" — rather than naming the |
| * page that happens to satisfy it today. |
| * |
| - * Clipped, not removed. [rst-shell-brand] is still the link home, and a |
| - * link with no accessible name is worse than one with a name nobody can |
| - * see. This is .rst-sr-only's own technique, written out because it has |
| - * to be undone again below. |
| + * display: none, and not the 1px clip this started as. The clip was |
| + * argued from "a link with no accessible name is worse than one nobody |
| + * can see", which is true of a link that is still the only route |
| + * somewhere — and this one is not. Below 800px "Your sheets" is in the |
| + * collapsed tail with the same destination (page.go redirects / to |
| + * /sheets for anyone signed in), so the clip bought a second |
| + * announcement of a route already in the menu, and cost a focus stop one |
| + * pixel square with nothing to reveal it: a keyboard user tabbing the bar |
| + * landed on a control they could not see or locate. rastrillo's |
| + * [rst-skip] gets away with the same technique because it un-clips on |
| + * :focus-visible and is a real destination; this is neither, so it |
| + * leaves the tab order (SC 2.4.7). |
| + * |
| + * `display: revert` and not a literal on the way back: tokens.css |
| + * declares no display for this element, so the value to return to is the |
| + * user agent's, and naming one here would freeze an answer that is not |
| + * this file's to give. |
| * |
| * Written mobile-first and reverted inside rastrillo's own |
| * `min-width: 800px` — the width its topbar collapses the tail at — so |
| @@ -100,22 +113,12 @@ |
| * rule would leave the fractional viewports that browser zoom really |
| * does produce uncovered by either. */ |
| [rst-shell-bar]:has([data-doc-title]) [rst-shell-brand] { |
| - position: absolute; |
| - inline-size: 1px; |
| - block-size: 1px; |
| - overflow: hidden; |
| - clip-path: inset(50%); |
| - white-space: nowrap; |
| + display: none; |
| } |
| @media (min-width: 800px) { |
| [rst-shell-bar]:has([data-doc-title]) [rst-shell-brand] { |
| - position: static; |
| - inline-size: auto; |
| - block-size: auto; |
| - overflow: visible; |
| - clip-path: none; |
| - white-space: normal; |
| + display: revert; |
| } |
| } |
diff --git a/internal/sheets/static/toolbar.js b/internal/sheets/static/toolbar.js| new file mode 100644 |
| index 0000000..b33d4bb |
| --- /dev/null |
| +++ b/internal/sheets/static/toolbar.js |
| @@ -0,0 +1,132 @@ |
| +/* The formatting toolbar's keyboard and overflow behaviour. |
| + * |
| + * Split out of commands.js, which was over the 300-line module cap |
| + * (AGENTS.md Frontend, and internal/guardstest/jscap_test.go holds it). |
| + * It is a real seam rather than a place the file happened to be cut: |
| + * commands.js decides what every control DOES, and this decides which |
| + * ones the keyboard is on and which ones are on screen. Nothing here |
| + * touches the command table or the grid. |
| + * |
| + * The Docs app draws the same line in the same place |
| + * (fixtures/toolbar-roving.mjs and toolbar-overflow.mjs), deliberately, |
| + * so the two apps' toolbars stay one idea in two codebases. |
| + */ |
| +(function () { |
| + 'use strict'; |
| + |
| + var surface = document.querySelector('[data-sheet-surface]'); |
| + if (!surface) return; |
| + |
| + function ready() { |
| + /* ---- the toolbar is one tab stop --------------------------------- |
| + * |
| + * Fourteen buttons, fourteen tab stops, is a strip a keyboard user |
| + * has to walk through to reach the grid. Roving tabindex with arrow |
| + * keys is the convention for a toolbar, and matches what the menu |
| + * bar beside it does. */ |
| + var strip = document.querySelector('[data-toolbar]'); |
| + if (!strip) return; |
| + var all = Array.prototype.slice.call(strip.querySelectorAll('[data-tool]')); |
| + if (!all.length) return; |
| + |
| + /* Only what a person can actually reach. |
| + * |
| + * Until the narrow-screen overflow landed, every control in this |
| + * strip was on screen and this list was the whole of it. Below 30rem |
| + * the colour menus and the three alignments are inside a panel that |
| + * is display: none while the "..." is closed, and a hidden element |
| + * cannot take focus — but it can still be handed tabIndex 0, which |
| + * is what the arrow keys did: right-arrow from Italic assigned the |
| + * stop to the hidden Fill colour summary and called .focus() on it, |
| + * which silently did nothing. The stop was then parked somewhere |
| + * unfocusable, so Tab skipped the toolbar altogether and further |
| + * right-arrows retried the same hidden control. |
| + * |
| + * checkVisibility() is the direct question; offsetParent is the |
| + * fallback and reads null for a display: none ancestor, which is the |
| + * only way anything in this strip is hidden. */ |
| + function visible(el) { |
| + return el.checkVisibility ? el.checkVisibility() : el.offsetParent !== null; |
| + } |
| + function reachable() { return all.filter(visible); } |
| + |
| + /* Recomputed on every use rather than captured once: which controls |
| + * are visible changes when the "..." opens and when the viewport |
| + * crosses the breakpoint, and a list captured at load is a list of |
| + * where the controls were, not where they are. */ |
| + function setStop(target) { |
| + all.forEach(function (el) { |
| + var want = el === target ? 0 : -1; |
| + if (el.tabIndex !== want) el.tabIndex = want; |
| + }); |
| + } |
| + function ensureReachable() { |
| + var list = reachable(); |
| + if (!list.length) return; |
| + var current = null; |
| + list.forEach(function (el) { if (el.tabIndex === 0) current = el; }); |
| + setStop(current || list[0]); |
| + } |
| + ensureReachable(); |
| + |
| + strip.addEventListener('keydown', function (e) { |
| + if (e.key !== 'ArrowLeft' && e.key !== 'ArrowRight') return; |
| + var stops = reachable(); |
| + var i = stops.indexOf(e.target.closest('[data-tool]')); |
| + if (i < 0) return; |
| + e.preventDefault(); |
| + /* Jspreadsheet listens for arrows on document and would walk the |
| + * cell cursor as well. */ |
| + e.stopPropagation(); |
| + var next = stops[(i + (e.key === 'ArrowRight' ? 1 : -1) + stops.length) % stops.length]; |
| + setStop(next); |
| + next.focus(); |
| + }); |
| + |
| + /* ---- the "..." overflow ------------------------------------------ |
| + * |
| + * Click-away and Escape, which <details> does neither of for itself |
| + * and which rastrillo.js deliberately does not do for this one — |
| + * templates/sheet.html's own comment explains why this control is |
| + * kept out of rastrillo's MENUS, and it comes down to the panel being |
| + * this element's SIBLING rather than its content, so every |
| + * containment check rastrillo makes answers the wrong question about |
| + * it. |
| + * |
| + * Containment is therefore tested against the whole strip, not |
| + * against the <details>: a tap on a swatch inside the panel is a tap |
| + * inside the toolbar and must leave it open, and a tap on a menu bar |
| + * title or in the grid is outside and must close it. |
| + * |
| + * Escape hands focus back to the summary. A control inside the panel |
| + * is inside a box that is about to be display: none, and focus left |
| + * there is focus on something the browser has stopped rendering — |
| + * the same hole rastrillo.js's own Escape handler is written to avoid |
| + * for the menus it does own. |
| + * |
| + * Capture phase and no stopPropagation, matching rastrillo.js: this |
| + * page has a grid, a formula bar and a live stream that all listen |
| + * for clicks, and swallowing events on the way past would break |
| + * them. */ |
| + var overflow = strip.querySelector('[data-tool-overflow]'); |
| + if (overflow) { |
| + document.addEventListener('click', function (e) { |
| + if (overflow.open && !strip.contains(e.target)) overflow.open = false; |
| + }, true); |
| + strip.addEventListener('keydown', function (e) { |
| + if (e.key !== 'Escape' || !overflow.open) return; |
| + overflow.open = false; |
| + var summary = overflow.querySelector('summary'); |
| + if (summary) { setStop(summary); summary.focus(); } |
| + e.preventDefault(); |
| + e.stopPropagation(); |
| + }); |
| + /* The stop may be sitting on a control the toggle just hid or |
| + * revealed. */ |
| + overflow.addEventListener('toggle', ensureReachable); |
| + } |
| + } |
| + |
| + if (window.SheetsGrid) ready(); |
| + else document.addEventListener('sheets:ready', ready, { once: true }); |
| +}()); |
diff --git a/internal/sheets/templates/sheet.html b/internal/sheets/templates/sheet.html| index f42cf1e..b46c5a3 100644 |
| --- a/internal/sheets/templates/sheet.html |
| +++ b/internal/sheets/templates/sheet.html |
| @@ -20,6 +20,7 @@ |
| <script defer src="{{asset "static/cellqueue.js"}}"></script> |
| <script defer src="{{asset "static/clipboard.js"}}"></script> |
| <script defer src="{{asset "static/commands.js"}}"></script> |
| +<script defer src="{{asset "static/toolbar.js"}}"></script> |
| {{end}} |
| {{define "doctitle"}} |
| @@ -377,18 +378,45 @@ |
| {{/* The "..." itself, at the trailing end and not rendered at all above |
| 30rem. |
| - A <details rst-dropdown> like every other menu here, in the |
| - toolbar's own "rst-tools" exclusive group so that opening it closes |
| - the colour menus and vice versa — one panel at a time, which is |
| - what the group is for. rastrillo.js gives it click-away and Escape |
| - with nothing added. |
| + A BARE <details>: no rst-dropdown, no name="rst-tools". Both were |
| + here for one round and both were wrong, for the same underlying |
| + reason — the panel this control reveals is its SIBLING |
| + ([data-tool-extra] above), not its content, because a closed |
| + <details> hides its own content and the wide layout cannot be |
| + recovered from inside one. That is rastrillo's own idiom for the |
| + shell tail and it is the right shape; what it means is that every |
| + piece of machinery that assumes "the menu contains what it opened" |
| + is wrong about this one: |
| + |
| + - name="rst-tools" made it exclusive with the two colour menus, |
| + which sit INSIDE the panel it reveals. Opening Fill colour |
| + closed this disclosure, which hid the panel, which took the |
| + colour menu down with it. Fill and text colour were simply |
| + unreachable on a phone. It is the nested-group trap this file |
| + already warns about at the top, met from the other side: not a |
| + <details> inside a grouped <details>, but a grouped <details> |
| + whose panel holds one. |
| + |
| + - rst-dropdown put it in rastrillo.js's MENUS, whose click-away |
| + closes any open menu that does not CONTAIN the click. Every tap |
| + on a swatch or an alignment button is outside this element, so |
| + the panel closed under the finger on every single use. Its |
| + Escape handler has the same shape from the other end: focus on |
| + an alignment button finds no menu ancestor, so it closes the |
| + panel and leaves focus on an element that is no longer rendered. |
| + |
| + So this control is invisible to rastrillo.js on purpose, and |
| + static/commands.js gives it the click-away and the Escape that |
| + actually fit the sibling shape. Its summary keeps [data-tool], so it |
| + is the same 28px box as the buttons it stands in for and the roving |
| + tabindex already walks it. |
| Three dots and not icons.go's "kebab": kebab is three STACKED dots |
| and its settled meaning across this design system is "more actions |
| on this row", which is not what this is. The accessible name is on |
| the summary because the glyph is a literal ellipsis with nothing to |
| announce. */}} |
| -<details rst-dropdown name="rst-tools" data-tool-overflow> |
| +<details data-tool-overflow> |
| <summary data-tool rst-tip aria-label="More formatting" data-tip="More formatting">…</summary> |
| </details> |