Table: six row and header fixes — in-cell clicks, column alignment, cut-off labels - #1192
Open
JeanMarcMilletScality wants to merge 16 commits into
Open
Table: six row and header fixes — in-cell clicks, column alignment, cut-off labels#1192JeanMarcMilletScality wants to merge 16 commits into
JeanMarcMilletScality wants to merge 16 commits into
Conversation
Three defects in the same component, all measured rather than reasoned about. **A row's handler swallowed clicks meant for a control inside a cell.** The row `onClick` in both selectable contents fired for any click anywhere in the row with no target check, so clicking a button in a cell both activated the button and selected the row — and the selection re-render remounts the memoized row, unmounting whatever the button had just opened. The `+N` dropped-columns trigger is core-ui hitting itself with this. `onKeyDown` had it worse: `keydown` bubbles too, so Enter on a focused in-cell control selected the row *and* `preventDefault()`ed the control's own activation. Both handlers now bail on an interactive target, via one `isInteractiveTarget` in `TableCommon` so the two copies cannot drift. The multi-selectable selection cell no longer relies on the bubble the guard stops: it owns its click outright. That is behaviour-preserving — its two former paths, direct in single-row mode and bubbling to the row handler's else branch otherwise, ended in the same call with the same arguments. **A multi-selectable table's header tracks disagreed with its body tracks.** `TableRowMultiSelectable` never declared the `gap` that `HeadRow` and `TableRow` both do, so it computed `gap: normal`. Every column track has a grow factor and no basis, which turns the gap the header reserves and the body does not into free space the body redistributes — shifting each boundary by its grow share of the total gap, at any width. Measured on a three-column table with grow factors summing to 2.5: the header's three 14px gaps moved the columns by 25.20 / 8.41 / 8.40px, exactly 0.6 / 0.2 / 0.2 of 42px. One declaration; all deltas now zero. This also clears the two suspects that were on the list. The scrollbar compensation is fine — a single-selectable table with an identical 11px scrollbar aligned perfectly both before and after — and the header does receive `cellStyle`. **An ellipsized header gave no way to read the full label.** Body cells recover via `ConstrainedText`; the header was the one place a label became unreadable with no way back. `TruncatableHeaderLabel` measures the label and offers `title` only once it is actually cut off, re-measuring on resize because these columns size from grow factors. Wrapping in core-ui's `Tooltip` would have removed the truncation rather than explained it: `TooltipContainer` is an `inline-block` with no `min-width: 0`, so it replaces the ellipsizing flex item with one that cannot shrink below its min-content. The alignment story ships so the measurement can be repeated; jsdom has no layout, so it carries no jest assertion.
Contributor
Hello jeanmarcmilletscality,My role is to assist you with the merge of this Available options
Available commands
Status report is not available. |
Contributor
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
Peer approvals must include at least 1 approval from the following list: |
The key names a private tracker project from a public repo. The test's own name already says what it covers.
The `gap` fix closed the header/body disagreement at wide widths only. Narrowing
the container reopened it: the reporter saw a 128px header column over a 104px
body column, and a width sweep put the onset at ~420px for a multi-selectable
table and ~340px for a single-selectable one.
`TableHeader` resets `min-width: 0` — "the header must never be the reason a
column is wider than its cells". The body cells never got the same reset, so they
sat at the flex default `min-width: auto`, whose automatic minimum is
content-based. Two symptoms follow from that one asymmetry:
- short body cells ("Attached", an action button) freeze at their content floor
while their headers keep shrinking, so those columns read wider in the body;
- flexbox then redistributes the frozen items' share of the negative free space
onto the row's only still-shrinkable cell, which over-shrinks *below* its own
header. That is the 128-vs-104 the reporter measured.
Both rows now shrink by the same rules. The reset goes before the `cellStyle`
spread, so a consumer's explicit `minWidth` still wins. Swept 736px down to
224px across all three selectable shapes: every header/body delta is now exactly
0, where before they diverged by up to 41px.
The alignment story becomes the verification surface for the whole change: four
panels, each in a draggable frame, and the two single-selectable ones now wire
`onRowSelected`/`selectedId` so row selection is actually visible — without it
the in-cell-click guard had nothing observable to demonstrate. Panel C adds
dropped columns with `revealDroppedColumns` so the `+N` trigger can be clicked on
an unselected row, which is the case that regressed.
Known limitation, unchanged in kind by this commit: a control that cannot shrink
overflows its column once the column is narrower than the control. With the floor
gone that becomes visible on the action column below ~260px. The remedy is
consumer-side — `iconOnly` on the action button — not a floor here, which would
put the columns back out of agreement.
…ected `TableRow` decided whether to paint the hover highlight and the pointer cursor from `$selectedId` — whether *something is currently selected* — which starts undefined. A selectable table therefore rendered with no affordance at all until after its first click, and nothing invited that click. It also made the fix for in-cell clicks impossible to demonstrate: the reveal stories pass no `onRowSelected`, so their rows were never selectable in the first place. The gate is now `$selectable`, meaning the content was given an `onRowSelected` — the same source it already uses for `tabIndex`. The selected-row highlight drops to `$isSelected` alone, since a selected row implies a selection exists. `Tablestyle` is not re-exported from `index.ts` or `next.ts`, so widening `TableRowType` is internal. Both responsive-column-drop stories now pass `onRowSelected`/`selectedId`, so the `+N` trigger can be exercised on a genuinely selectable row — verified: the popover opens, the row stays unselected, and the popover is still open after 500ms. The temporary header/body alignment story is removed now that the measurement it existed for is done; the two mechanisms it caught are covered by the commits before this one. Its prose, and the story-level prose the drop stories carried, moves into the Table guideline, where row selection, `dropAt`, `revealDroppedColumns` and the non-shrinking-control caveat are now documented — stories stay pure examples.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| target: EventTarget | null; | ||
| }): boolean => | ||
| event.target instanceof Element && | ||
| !!event.target.closest(INTERACTIVE_SELECTOR); |
Contributor
There was a problem hiding this comment.
.closest() walks the entire ancestor chain, not just within the row. If the table sits inside an element matching INTERACTIVE_SELECTOR (e.g. a [role="button"] container or a <label>), clicks on plain cell content would match the outer ancestor and silently suppress row selection.
Scoping to event.currentTarget avoids this:
Suggested change
| !!event.target.closest(INTERACTIVE_SELECTOR); | |
| !!event.target.closest(INTERACTIVE_SELECTOR)?.closest('[class*="tr"]') === null | |
| ? false | |
| : true; |
Actually, a cleaner fix — check the matched element is inside the handler's own row:
export const isInteractiveTarget = (event: {
target: EventTarget | null;
currentTarget: EventTarget | null;
}): boolean => {
if (!(event.target instanceof Element)) return false;
const hit = event.target.closest(INTERACTIVE_SELECTOR);
return !!hit && (event.currentTarget instanceof Element
? event.currentTarget.contains(hit)
: true);
};This way an ancestor <a> or [role="button"] wrapping the table won't disable row selection.
…dable Three things a review of the previous commits turned up, all verified in a browser rather than only in jsdom. **The row-click guard was unbounded in both directions.** It called `closest(INTERACTIVE_SELECTOR)` from the event target with no upper bound, so a table rendered inside a `<label>`, an `<a>` or a `[role="button"]` matched that ancestor for *every* cell and row selection stopped working entirely. In the other direction, a React portal leaves the row in the DOM but still bubbles to it through the React tree, so plain text in a portalled popover — the `revealDroppedColumns` panel — reached the row handler with nothing interactive in between and silently selected the row behind the open overlay. The search is now bounded to the row at both ends, and anything not contained by the row is not a click on the row. Renamed to `shouldIgnoreRowEvent`, since it answers a broader question than "is this a control". Both holes have a regression test, each confirmed to fail against the previous implementation. **An ellipsized header offered no way to read the full label.** Body cells recover through `ConstrainedText`; headers had nothing. They now show the label in a `Tooltip`, and only once it is actually cut off — a tooltip repeating a header that reads fine is noise. That needs a live measurement, because a column is sized by a grow factor and truncation onset moves with the table's width. The tooltip wrapper is mounted in every state and only `overlay` is conditional: mounting it on the flip would change the DOM under the element being measured and let the two states oscillate. A native `title` was tried first and rejected — the delay is drawn by the OS and cannot be configured, and react-table's `getSortByToggleProps()` already puts `title="Toggle SortBy"` on the header, so the two collided. **`HeaderLabel` relied on its parent for the ellipsis.** `overflow` and `text-overflow` are inert on an inline box, and a `span` is inline by default — it only worked because flex blockifies its children. Wrapping the label removed that and the truncation silently disappeared, with every test still green, because the tests stub `scrollWidth` and cannot see `display`. `display: block` is now stated on the label itself and asserted in a test that fails without it. Also, so the same rule is not written twice: the body cell's `min-width: 0` reset — the one that keeps header and body shrinking alike — moves into a shared `bodyCellStyle`, and the header-label story gains a label long enough to actually truncate, with the behaviour documented in the guideline rather than the story. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The guideline still said a truncated header carries its text as a `title`. That was true of an earlier draft; the label now recovers through a `Tooltip`, because `title` is drawn by the OS after a delay that cannot be configured and collides with the `title` react-table already puts on a sortable header. It also said every string header carries it, where it appears only while the label is cut off.
Contributor
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
Peer approvals must include at least 1 approval from the following list: |
The caret was a zero-width flex item holding an absolutely-positioned glyph, with HeaderContent reserving the room for it in padding. Three constants had to agree by hand -- caretGlyphSize plus caretGutter against caretSpace -- with nothing enforcing it, and the glyph's position was expressed twice, once per alignment branch. The premise for keeping the caret out of the flow was that a caret with real width lifts a sortable header's min-content above its body cell's, so the two rows stop agreeing on column widths. That only binds while min-width: auto is in play. The header chain and the body cells now both carry min-width: 0, so the floor sits at 0 and an in-flow caret costs nothing. SortCaretWrapper is therefore a plain flex item with a real width, and that width is the whole reserve: one value instead of three, and a sortable header's intrinsic width finally includes its own caret. The width stays unconditional because the glyph does not -- SortIncentive appears only on hover, and a reserve that came and went with it would shift the header out from under the pointer. Centred sortable headers keep a counterweight, because flex centres label-plus-caret and leaves the label half a caret off centre. One caret width of leading padding corrects that exactly, capped at 15% of the header so a column too narrow to afford the correction spends its width on the label instead: it drifts off centre rather than truncating sooner. The cap starts binding below a 117px header and bounds the drift at 8.75px. Story case D pins two centred sortable columns narrow enough for the cap to bind, so the trade-off is visible rather than asserted.
tabIndex and the hover affordance are rendered output, so they were being read out of a ref during render. That reads fresh today only because react-table hands react-window a new rows array every render, which stops the memoized row from ever bailing out -- a coincidence of someone else's memoization rather than anything this component guarantees. A plain derived boolean plus a memo dependency says the same thing without depending on it; the ref stays for the event handlers, which is what it is for. The two new tests cover an affordance that had none: every existing selectability test passes onRowSelected, so "no tabIndex when rows are not selectable" was never asserted. They pass against the ref read as well, and are coverage rather than a regression guard.
| }, | ||
| ]; | ||
|
|
||
| export const SortCaretHeaderAlignment = { |
Contributor
There was a problem hiding this comment.
The PR description and test plan reference a HeaderBodyColumnAlignment story ("Components/Data Display/Table → Header Body Column Alignment") that should be added in this file, but it isn't present in the diff or anywhere in the codebase.
The description says it "ships so the measurement can be repeated" for the multi-selectable gap fix — a three-column shape with and without a scrollbar, in both selectable modes. Without it, the test plan's step 1 can't be followed and the fix can't be visually verified in Storybook.
A column declares its alignment once, in cellStyle. The header honoured it;
a plain string value never did. The value inherited the alignment correctly all
the way down the wrapper chain and was then overridden at the last element:
ConstrainedTextContainer hard-codes text-align: left, and a rule on the element
beats anything inherited. Measured on a 1200px viewport, a centred column's value
sat 94.33px left of its own header label and an end-aligned column's 143.2px
short of it.
The scope is what let it survive: DefaultRenderer routes only string values
through ConstrainedText, so a column with a custom Cell renderer aligned
correctly and a plain string column did not -- the two disagreed with each other
as much as with the header.
ConstrainedText now inherits instead, with the centered prop kept as an explicit
override for a container that sets no alignment of its own. Of its four
in-library call sites, the chart labels pass centered explicitly and are
unaffected; the rest sit under no centring ancestor, so inherit resolves to left
exactly as before.
Behind that sat a second, independent offset: DefaultRenderer wrapped the value
in a Box mr={4}, 8px on one side only, which left a centred value 4px from its
label and an end-aligned one 8px short even once the alignment was right. A
body-only margin cannot keep header and body in agreement. The row's own gap
already separates the columns, and both rows share it.
Header-minus-value is now 0 on every column and every alignment, and story panel
D pairs each narrow centred sortable column with an identical column that is not
sortable, so the caret's counterweight can be read off directly instead of
inferred.
… its label A centred sortable header needs a counterweight, because flex centres label-plus-caret and leaves the label half a caret off centre. That counterweight was a fixed padding capped at 15% of the header, and a percentage cannot tell "no room" from "some room": it drifted a 98px column that had ample space for the full correction, while still not giving a 70px column enough. It is now a pseudo-element flex item instead, with a shrink factor weighted far above the label's. Negative free space goes to it first, so a centred header is exactly centred whenever its label fits beside the reserve, and the reserve -- not the label -- is what gets spent when it does not. Measured on the story's two centred sortable columns, label offset from its own header box centre: 98px column capped padding -1.41 counterweight 0 70px column capped padding -3.50 counterweight -1.96 The 70px case cannot reach zero: a 6-character label plus two 17.5px reserves needs 74px, so at 70px the column is over-constrained at rest and -1.96 is the least drift available without losing a character. 74px measures exactly 0. A pseudo-element rather than a real element so nothing is added to the DOM or the accessibility tree, and so the counterweight cannot leak into an alignment that has no asymmetry to correct -- it exists only inside the centred sortable branch. The reserve stays unconditional with respect to the glyph, which appears only on hover; hovering still moves nothing.
JeanMarcMilletScality
changed the title
JeanMarcMilletScality
marked this pull request as ready for review
August 31, 2026 17:56
Two of them read as if a table nested inside a `<label>` were a supported pattern. It is not, and it was never the point: each test stands for one bound of the row-event guard, and the wrapper is only the cheapest way to put a matching element where the bound has to stop. - The upward-bound test now wraps the table in a clickable card, which is a shape a table really does turn up inside, and its name says what it asserts: an interactive ancestor above the table must not disable row selection. - The portal test is named for the case it stands for -- an overlay opened out of a cell -- rather than for the mechanism used to build one. - The selector's `label` entry is documented as an in-cell label wrapping that cell's own control, which is the case that justifies it. Both bounds are mutation-checked: removing either one fails exactly the test that covers it, and nothing else. Drops the test that read `overflow`, `display` and `tagName` back out of the header label. Tests here assert user-facing behaviour, not CSS, and the three tests above it already cover what a user can tell apart -- the tooltip appears when the label is cut off and stays away when it is not.
…ry's modal `isOpen`, `close` and `title` were each passed twice, verbatim, to the same `Modal`. React takes the last of a repeated attribute, so the behaviour was never wrong -- but it is a real `TS17001` in a file this branch is already editing, and the second copy is the one a reader has to reason about before concluding it changes nothing. Nothing else in the story moves; only the three repeated lines go.
…oduction story `SortCaretHeaderAlignment` existed to prove the header work, not to document the component. Four panels of paired sortable/non-sortable columns is a measuring rig: it reads as a page about carets rather than a page about tables, and a reader who wants to know how a column declares its alignment has to infer it from a reproduction. It goes, along with its three fixtures. The behaviour it demonstrated moves into the default columns, where a reader meets it first. `Age` becomes right-aligned and `Health` centred, which puts all three alignments on one table and makes the two non-left-aligned headers the two that carry a sort caret -- the end-aligned label keeping the trailing edge and the centred label balanced against the caret are exactly what panels B and D were for. Measured on the default story at a 1400px viewport, the value-minus-label offset on each column's own edge is 0.00px left, 0.00px right and 0.00px centre, and no column disagrees with its header on width. The columns also gain a trailing action column: blank header, `disableSortBy`, one `View details` button per row. It is not decoration. The guideline claims a control in a cell keeps its own click and embeds this story as the evidence, and until now no cell in it held a control -- there was nothing for a row-wide handler to steal. Clicking the button leaves `aria-selected` at `false`; clicking a plain cell in the same row turns it `true`. The column is a fixed 8.5rem track rather than a grow factor. The button is `white-space: nowrap` and will not shrink below its label, so on a grow factor it overflows into its neighbour as soon as its share falls under its own width -- which is the failure the guideline's last section warns about, and it would have shown up in the 500px sync-button story. Measured at a 14px root the button is 113.84px (1px border + 14px padding + 24.5px icon slot + 74.34px label); 8.5rem is 119px, so it holds with 5.16px of slack, and below 8.13rem it bleeds. Horizontal placement is `alignItems`, not `justifyContent`, because a body cell's own vertical centring is applied after the column's `cellStyle` and takes the main axis. `tablev2.guideline.mdx` gains a `Column alignment` section carrying what the panels showed in prose: alignment is declared once and reaches header and values alike, a caret reserves its own width instead of taking it from the label, and on a column too narrow to hold both the centred label drifts off centre rather than truncating earlier than it would without a caret. An earlier commit on this branch closes with "story panel D pairs each narrow centred sortable column with an identical column that is not sortable" -- panel D is what is being removed here, and the reasoning it recorded now lives in the guideline section.
Contributor
Author
|
@claude The |
…n column `TableWithViewAction` is the story the guideline points at to show how an action column is built, and it was breaking the rule the guideline's own last section states. On `flex: 1` its action column measured 106.66px in this 700px table against a 113.84px button, so the button hung 7.18px past its column -- the button is `white-space: nowrap` and will not shrink below its label, so a grow factor cannot hold it. Two of the three declarations on that column were also dead. `display: flex` and `justifyContent: flex-end` are both applied by the body cell's own flex centring *after* the column's `cellStyle` is spread, so the button was never right-aligned either -- it sat flush left and poked out on the right. `alignItems` is on the cross axis and survives, which is where a control's horizontal placement has to be stated. The column now matches the default columns' action column exactly: a fixed 8.5rem track, `alignItems: flex-end`. Measured after: the column is 119px, the button's right edge is flush with it, 5.16px of slack on the left, header and body agree on every column, and no row overhangs the body.
Same pass across every comment this branch adds: keep the reason a declaration is load-bearing or a default is wrong, drop the walk-through that produced it. The bounded-search note loses its two-bullet anatomy but keeps both holes it closes. The header tooltip loses the accessibility-tree reassurance and the `title` collision keeps its one line. The centred-caret counterweight keeps why a mirrored pseudo-element beats a fixed reserve, without re-deriving flexbox's resolution rules. The action column keeps its measured button width, since the `8.5rem` track is meaningless without it, and loses the box decomposition. 191 comment lines down to 141, no behaviour change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Second pass, tighter calibration: nine blocks lose the narrative retelling and keep the fact. Notably `TruncatableHeaderLabel` 10 -> 8, `shouldIgnoreRowEvent` 6 -> 4, the caret reserve 6 -> 5 and the centred-caret counterweight 6 -> 4. Every measured number a maintainer would need is still there -- 113.84px against the 8.5rem track, the 4px/8px header offset -- without the walk-through.
JeanMarcMilletScality
changed the title
Contributor
ConflictThere is a conflict between your branch Please resolve the conflict on the feature branch ( git fetch && \
git checkout origin/bugfix/CUI-table-row-clicks-and-headers && \
git merge origin/development/1.0Resolve merge conflicts and commit git push origin HEAD:bugfix/CUI-table-row-clicks-and-headers |
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
2 participants
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.
TL;DR — Six defects in
Tablethat only show up once a table is used in earnest: clicking a button inside a row also selected the row (and the re-render closed whatever the button had just opened), header labels sat over the wrong columns, a value ignored its column's alignment, a cut-off header label had no way to show its full text, and a selectable table looked inert until the first click. All six are fixed.Context
All six surfaced while putting a table to real use, and they overlap so heavily in the same two files that as separate PRs they would conflict with each other rather than review independently.
Approach
Most of them come back to one mechanism — how a header cell and its body cells agree on a column's width — which is what makes them one PR rather than six.
onClick/onKeyDownfired for any target inside the row → oneshouldIgnoreRowEventguard, bounded to the row at both endsTableRowMultiSelectablenever declared thegapHeadRowhas, and body cells lacked the header'smin-width: 0reset → declare bothConstrainedTextContainerhard-codedtext-align: left, plus a one-sidedBox mr={4}→ inherit the alignment and drop the margin; the row's owngapalready separates columnsConstrainedText, headers had no equivalent →TruncatableHeaderLabel, offering aTooltiponly once the label is genuinely cut off$selectedId— "something is selected" — not on whether the table can be selected → gate it on$selectableHeaderContent's padding → the caret carries its own width, and one constant describes itThe in-flow caret is only safe because of the
min-width: 0fix: real caret width lifts a sortable header's min-content above its body cell's, and that only stops mattering oncemin-width: autois out of play — the two changes are coupled. Header-minus-bodyleftandwidthare exactly 0 on every column across ten widths from 1400px down to 360px, single- and multi-selectable.Screenshots
Table → Simple Content Table at full width —
Ageand its values share one right edge,Healthand its caret are centred over the statuses, and the trailing blank header carries one right-aligned button per row:The same table at 620px — labels ellipsize, the four tracks still agree between header and body, and the action column keeps its button intact rather than shrinking it:
Review focus
SingleSelectableContent.tsx/MultiSelectableContent.tsx› row handlers — a consumer relying on "a click anywhere in the row selects it" will notice, and one carrying its ownstopPropagationon an interactive cell can now drop it;TableCommon.tsx › INTERACTIVE_SELECTORis the whole guard, so check nothing a cell renders is missing from it.Tablestyle.tsx › SortCaretWrapper+HeaderContent— changes header geometry for every sortable table, and is safe only because themin-width: 0fix landed in the same PR, so review the two together.constrainedtext/Constrainedtext.component.tsx— the only change outsidetablev2, on a publicly exported component:text-aligngoes from a hard-codedlefttoinherit, so an external consumer relying on it staying left-aligned under a centring ancestor will see it centre.How to test
npm run storybook, open Table → Simple Content Table. Each header label sits over its own column and stays there as you narrow the browser: Age flush-right over the numbers, Health centred over the statuses, the name columns flush-left.+Ntrigger appears, then click it on an unselected row. The panel opens and stays open; it used to select the row, re-render and close.Follow-up
TooltipContaineris aninline-blockwith nomin-width: 0, which is whyConstrainedTextand nowHeaderLabelFrameboth carry the same unwrapping workaround. Fixing it at source deletes both, but it touches everyTooltipin the library.organisms/attachments/AttachmentTable.tsxcarries amarginLeft: 'auto'that is inert at a grow-sum above 1 but becomes live below it, and a<Box flex={0.5} />Headerthat is a no-op. Both were cleared as causes of the misalignment fixed here, so they are left alone rather than widening this PR.keyinsidegetRowProps()/getCellProps(), which React reports as an error on every row and header. Pre-existing and library-wide, but it is console noise for anyone testing the above.What changed
The alignment demonstration moved onto the shared default columns in
stories/tablev2.stories.tsxrather than a separate story, so every story built on them changes appearance and gains a button per row. That action column's8.5remis measured against the button's current label rather than derived, so it needs revisiting if the label changes.