Skip to content

fix(viewer): inset in-table focus rings and let long meta values wrap - #1465

Open
misslead wants to merge 1 commit into
alibaba:mainfrom
misslead:fix/viewer-focus-ring-and-meta-wrap
Open

misslead wants to merge 1 commit into
alibaba:mainfrom
misslead:fix/viewer-focus-ring-and-meta-wrap

Conversation

@misslead

Copy link
Copy Markdown

Fixes #1455

This lands items 1-3 of #1455 and records a decision on item 4.

1. In-table focus rings were still clipped

.table a:focus-visible carries outline-offset: -2px, but three rules are more specific and kept +2px, so the inset only ever applied to the sessions Compare link. All three now use -2px:

  • .sessions-page .session-id:focus-visible
  • .repos-page .col-repository a:focus-visible
  • .repos-page .repo-check:focus-visible

TestFocusCSS_CoversCollapsiblesAndTableLinks only asserted that the selector existed, which passes just as happily with the wrong value. It now reads each rule block and asserts the effective outline-offset: -2px.

2. A long CWD or branch stayed unreadable at narrow widths

.meta span is display: flex, so the value is an anonymous flex item whose min-content is the whole path: max-width: 100% alone left it clipped by the inherited overflow: hidden, with no ellipsis (text-overflow does not apply to a flex container). The 768px override now adds flex-wrap: wrap and overflow-wrap: anywhere, and lets .meta-truncate overflow visibly.

TestResponsiveCSS_MetaOverrideComesAfterBase matched the old declarations literally, so the strings are updated; the cascade guard itself is unchanged.

3. The exported page's script concatenation had no test

export.go prepends a11y.js ahead of session.js, which calls window.ocrArrowScroll on its first line. Dropping the concatenation throws on line 1 and takes every bit of interactivity with it.

  • TestExportSession_SelfContained now asserts the inlined script carries window.ocrArrowScroll.
  • TestHandleSession_ServedPageKeepsStaticRefs now asserts src="/static/a11y.js" alongside the existing session.js reference.
  • New TestStaticAssetRefs_OnReposAndSessionsPages covers repos.html and sessions.html, which reference the same script.

4. WCAG coverage for the panel surfaces: not landed, and why

The three panel surfaces measure 4.61-4.84 against both themes, so they do clear AA; the tightest margin is 0.11. Extending TestTextTokens_MeetWCAGAAOnPageBackground requires blending the token alpha over each surface color instead of over the page background - the current helper treats the page as pure black or pure white, which only holds for --bg. That is a real change to the luminance helper and belongs in its own PR rather than folded into a CSS fix. Happy to follow up with it if you would rather have it here.

Verification

go test ./internal/viewer/ -count=1 passes, go vet ./internal/viewer/ passes, gofmt -s -l is clean, and go run scripts/verify-english-only.go reports no unapproved non-English text.

Items 1-2 still want a browser check at 375px and at desktop width, light and dark. I could not run a browser here, so that part is unverified.

Fixes alibaba#1455

This lands items 1-3 of alibaba#1455 and records a decision on item 4.

### 1. In-table focus rings were still clipped

`.table a:focus-visible` carries `outline-offset: -2px`, but three rules are more specific and kept `+2px`, so the inset only ever applied to the sessions `Compare` link. All three now use `-2px`:

- `.sessions-page .session-id:focus-visible`
- `.repos-page .col-repository a:focus-visible`
- `.repos-page .repo-check:focus-visible`

`TestFocusCSS_CoversCollapsiblesAndTableLinks` only asserted that the selector existed, which passes just as happily with the wrong value. It now reads each rule block and asserts the effective `outline-offset: -2px`.

### 2. A long CWD or branch stayed unreadable at narrow widths

`.meta span` is `display: flex`, so the value is an anonymous flex item whose min-content is the whole path: `max-width: 100%` alone left it clipped by the inherited `overflow: hidden`, with no ellipsis (`text-overflow` does not apply to a flex container). The 768px override now adds `flex-wrap: wrap` and `overflow-wrap: anywhere`, and lets `.meta-truncate` overflow visibly.

`TestResponsiveCSS_MetaOverrideComesAfterBase` matched the old declarations literally, so the strings are updated; the cascade guard itself is unchanged.

### 3. The exported page's script concatenation had no test

`export.go` prepends `a11y.js` ahead of `session.js`, which calls `window.ocrArrowScroll` on its first line. Dropping the concatenation throws on line 1 and takes every bit of interactivity with it.

- `TestExportSession_SelfContained` now asserts the inlined script carries `window.ocrArrowScroll`.
- `TestHandleSession_ServedPageKeepsStaticRefs` now asserts `src="/static/a11y.js"` alongside the existing session.js reference.
- New `TestStaticAssetRefs_OnReposAndSessionsPages` covers repos.html and sessions.html, which reference the same script.

### 4. WCAG coverage for the panel surfaces: not landed, and why

The three panel surfaces measure 4.61-4.84 against both themes, so they do clear AA; the tightest margin is 0.11. Extending `TestTextTokens_MeetWCAGAAOnPageBackground` requires blending the token alpha over each surface color instead of over the page background - the current helper treats the page as pure black or pure white, which only holds for `--bg`. That is a real change to the luminance helper and belongs in its own PR rather than folded into a CSS fix. Happy to follow up with it if you would rather have it here.

### Verification

`go test ./internal/viewer/ -count=1` passes, `go vet ./internal/viewer/` passes, `gofmt -s -l` is clean, and `go run scripts/verify-english-only.go` reports no unapproved non-English text.

Items 1-2 still want a browser check at 375px and at desktop width, light and dark. I could not run a browser here, so that part is unverified.
@CLAassistant

CLAassistant commented Sep 19, 2026

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@github-actions

Copy link
Copy Markdown
Contributor

OpenCodeReview: Review complete: 0 finding(s) across 1 selected item(s).

@lizhengfeng101

Copy link
Copy Markdown
Contributor

@misslead Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.

@misslead

Copy link
Copy Markdown
Author

Hi @lizhengfeng101 - thanks for the pointer! The CLA was signed right after opening: the license/cla commit status on the head commit is green and the CLA assistant badge reports all committers have signed. If it still shows as pending on your side, happy to go through the flow again - just let me know.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(viewer): unfinished items from the #1329 responsive/a11y pass

3 participants