Conversation
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.
Contributor
|
✅ OpenCodeReview: Review complete: 0 finding(s) across 1 selected item(s). |
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. |
Author
|
Hi @lizhengfeng101 - thanks for the pointer! The CLA was signed right after opening: the |
This branch has not been deployed
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.
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-visiblecarriesoutline-offset: -2px, but three rules are more specific and kept+2px, so the inset only ever applied to the sessionsComparelink. All three now use-2px:.sessions-page .session-id:focus-visible.repos-page .col-repository a:focus-visible.repos-page .repo-check:focus-visibleTestFocusCSS_CoversCollapsiblesAndTableLinksonly asserted that the selector existed, which passes just as happily with the wrong value. It now reads each rule block and asserts the effectiveoutline-offset: -2px.2. A long CWD or branch stayed unreadable at narrow widths
.meta spanisdisplay: 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 inheritedoverflow: hidden, with no ellipsis (text-overflowdoes not apply to a flex container). The 768px override now addsflex-wrap: wrapandoverflow-wrap: anywhere, and lets.meta-truncateoverflow visibly.TestResponsiveCSS_MetaOverrideComesAfterBasematched 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.goprependsa11y.jsahead ofsession.js, which callswindow.ocrArrowScrollon its first line. Dropping the concatenation throws on line 1 and takes every bit of interactivity with it.TestExportSession_SelfContainednow asserts the inlined script carrieswindow.ocrArrowScroll.TestHandleSession_ServedPageKeepsStaticRefsnow assertssrc="/static/a11y.js"alongside the existing session.js reference.TestStaticAssetRefs_OnReposAndSessionsPagescovers 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_MeetWCAGAAOnPageBackgroundrequires 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=1passes,go vet ./internal/viewer/passes,gofmt -s -lis clean, andgo run scripts/verify-english-only.goreports 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.