Repository navigation
Make the row-access grant look like something you can press - #39
Merged
Merged
Conversation
Two people have now failed to find it, and both fixes before this one changed the wording. The settled scope for this control's discoverability is wording and presentation, and presentation had never been touched: measured on the built app, `enable browsing` was a 92x14px run of blue text with `cursor: pointer` and nothing else - no border, no background, no padding - sitting beside a status badge that did have a border. The row therefore showed one bordered thing, which is the state, and some coloured words, which are the only way to grant anything. It now draws a box: 112x21px with a 1px blue border, a filled background and room around the label, against the 92x14 it replaced. The label itself is unchanged at 12px - an earlier draft of this set `font-size: 0.72rem` "for the button", which the button was already inheriting, so the only thing it did was shrink the words two people could not find by 4%. It is also a real <button> where one is legal, which is the surface that matters. The overview card cannot hold one: the card is itself a <button> that selects the profile, and a <button> inside a <button> is invalid HTML - the accessibility tree of a nested interactive element is undefined and readers disagree about what to announce. It is NOT a rendering failure, which an earlier version of the comment claimed: Svelte builds the card and this block as separate templates and joins them by DOM insertion, so the parser repair that would close the card's <button> early never runs. The profile bar has no interactive host, and it already proved the point, since `delete` up there has been a real button since the component existed. The flag is `asButton`, off by default, so the answer a host gets by forgetting is the one that renders correctly anywhere. It was first written as `insideButton = false`, which reads as a safe default and was the opposite of one: that spelling handed a real <button> to any host that said nothing, which is the single case the flag exists to prevent. The three controls are one derived list now rather than three written-out branches, so the two element kinds cannot drift into offering different grants. The e2e asserts the box and the colours, not "has some border and some background": a bare <button> with this app's rule deleted still carries Chromium's own 2px border, grey fill and padding, so three inequalities on those pass while the control is back to a run of text. Broken four ways while writing it - the bar falling back to a span, the fill and padding deleted, the border made transparent, and the card forgetting its host - each failed a different assertion. Computed style is read through `getPropertyValue`, because a hand-written record of camelCase fields makes the type authoritative over the real API and a renamed field then reads `undefined`, which passes an inequality forever. Recorded and not fixed: on the card, Space reaches the card's own <button> and selects the profile instead of granting, and Enter fires both. Pre-existing - the bar's spans did the same before this control had a box - and left as it is, but it is the sharper reason the bar takes a real button. Not touched: the MCP controls and the card's own refresh/delete are still linkish text, so the bar now shows a bordered button on one row and coloured words on the next. Only the row-access control has a report behind it, and widening this to every clickable word in the app is a separate decision. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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
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.
Two people failed to find the row-access grant, and the two fixes before this one both changed only the wording. Presentation had never been touched: measured on the built app,
enable browsingwas a 92x14px run of blue text withcursor: pointerand nothing else — no border, no background, no padding — next to a status badge that did have a border. The row showed one bordered thing (the state) and some coloured words (the only way to grant anything).What changes
<button>where one is legal. The profile bar gets a button (deleteup there has been one since the component existed). The overview card cannot hold one: the card is itself a<button>that selects the profile, and a<button>inside a<button>is invalid HTML — the accessibility tree of a nested interactive element is undefined and readers disagree about what to announce. Note this is not a rendering failure (an earlier version of the code comment claimed it was): Svelte builds the card and this block as separate templates and joins them by DOM insertion, so the parser repair that would close the card's<button>early never runs.asButton, off by default, so a host that forgets gets the rendering that is legal anywhere. It was first writteninsideButton = false, which reads as a safe default and is the opposite of one — that spelling hands a real<button>to any silent host, the single case the flag exists to prevent.Test
e2e/app.spec.tsasserts the box and the colours, not "has some border and some background": a bare<button>with this app's rule deleted still carries Chromium's own 2px border, grey fill and padding, so three inequalities on those pass while the control is back to a run of text. Broken four ways while writing it — bar falling back to a span, fill and padding deleted, border made transparent, card forgetting its host — each failed a different assertion. Computed style is read throughgetPropertyValue, because a hand-written record of camelCase fields makes the type authoritative over the real API and a renamed field then readsundefined, which passes an inequality forever.Local:
typecheck/lint/test:unit(338) /test:e2e(9) all green onpnpm build && pnpm test:e2e.Deferred, not fixed
<button>. Pre-existing — the bar's spans did the same before this control had a box — left as it is by decision, and noted in the code. It is the sharper reason the bar takes a real button.<span>branch is now unreached by any e2e click (every click in the suite lands on the bar, i.e. the button branch). The branch is asserted for its rendering, not its behaviour..linkishtext, so the bar shows a bordered button on one row and coloured words on the next —deletenow looks less pressable than the grant. Only the row-access control has a report behind it; widening this to every clickable word is a separate decision.Design-challenge candidates raised by review and blocked (operator's call)
Reported here rather than applied — each reverses a settled choice:
<button>, so the grant can be one there too..linkishacross the app in this PR.Implementation-defect findings from the same review were fixed in this commit (the
font-sizeshrink, theinsideButtondefault, the false "rendering failure" comment, the Chromium-default-passing assertions, the camelCase style record).