Axe overlay polish: left-align report and scroll highlighted elements clear of it - #14680
Merged
Conversation
Collaborator
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
cwickham
force-pushed
the
axe/overlay-polish
branch
from
July 13, 2026 17:56
27cc6b2 to
fa02ea5
Compare
Member
Author
cwickham
marked this pull request as ready for review
July 13, 2026 18:44
cderv
marked this pull request as draft
July 16, 2026 09:35
cderv
approved these changes
Jul 16, 2026
cderv
left a comment
cderv
left a comment
Member
There was a problem hiding this comment.
Looks good ! Nice improvments.
Something I questioned while reviewing: is exporting overlayAwareScrollTop only to be able to test it in unit test a good idea, as this will expose the function in browser console context unscoped. I am curious to ask Claude to see if this is good thinking or just me worrying too much on best practices.
The overlay is appended into <main>, so page-level centering (e.g. the
jolla about template's 'div.quarto-about-jolla main { text-align:
center }') centered the report text. The reveal report slide already
pins text-align: left; do the same in the base overlay rule.
Hovering a selector in the report scrolled the target element to the
viewport center with scrollIntoView({block: 'center'}), where the fixed
bottom-right overlay (max-height: 50vh) could cover it. Compute the
scroll target with a new exported pure helper, overlayAwareScrollTop,
that centers the element in the viewport band above the overlay instead;
elements with no horizontal overlap (or when the overlay leaves no
usable band) keep the default centering. Only the plain-HTML fixed
overlay takes this path: reveal navigates slides and the dashboard
offcanvas is a static full-height panel that vertical scrolling can't
dodge.
The overlay itself never moves or fades, so the mouse stays inside it
during the scroll and no mouseleave -> unhighlight flicker can fire.
Unit tests cover the helper's geometry; a Playwright test hovers a
violation near the bottom of a long page (with enough violations to
inflate the report to its max-height, without which default centering
already clears the overlay and the test has no teeth) and asserts the
highlighted element settles fully above the overlay. The spec also
gains a text-align: left assertion on the HTML overlay for the
left-alignment fix.
…board The overlay scrolls once the report exceeds max-height: 50vh, but wasn't keyboard-focusable — failing axe's own scrollable-region-focusable rule (serious, WCAG 2.1.1) when scanned directly. Add tabindex=0 plus role/aria-label in createReportOverlay only: the reveal slide and dashboard offcanvas variants don't scroll the report element itself, so they don't get the extra tab stop. Verified by running the pinned axe-core build against the rendered overlay on the long-report test page: scrollable-region-focusable before the change, no violations after. Note the violation-target spans remain mouse-only; full keyboard operability of the report is tracked separately.
The overlay is injected after the page scan, so it never audits itself and nothing else guards against new violations as the report UI grows. Scan it with the vendored axe-core build the page already loaded (window.axe), on the long-report page where the overlay is scrollable. Requires the vendored axe-core from #14677, which this branch is now stacked on. Verified the test fails with scrollable-region-focusable when the tabindex fix is removed.
cwickham
force-pushed
the
axe/overlay-polish
branch
from
July 17, 2026 18:58
fa02ea5 to
268b116
Compare
cwickham
marked this pull request as ready for review
July 17, 2026 19:01
A violation node's target array is one element's cross-frame selector chain, not a set of distinct elements. Registering the hover/click listeners inside the per-selector span loop attached one listener set per selector, so a single hover over a multi-selector node fired one competing window.scrollTo per selector (last-wins jank). Split selector-span rendering (still shows the full chain) from listener registration, which now iterates a pure nodeScrollTargets() helper that returns the primary target only. Single-selector nodes -- the common case -- are unchanged. The helper is DOM-free like overlayAwareScrollTop, so the one-scroll-per-hover decision is unit-tested directly.
Member
|
Update: I've reverted that follow-up ( On a closer look it wasn't fixing a real bug. A multi-selector |
This reverts commit 3dc2133.
cderv
added a commit
that referenced
this pull request
Jul 21, 2026
Recent axe-core changes (#14607, #14655, #14667, #14677, #14680) reworked the accessibility-report feature repeatedly with no dedicated regression tests guarding several of its behaviors. A coverage audit found three gaps; this closes them. - No test guarded against reintroducing a CDN dependency for axe-core, despite #14677 vendoring it specifically to drop the Skypack CDN. - The report overlay's keyboard-scrollable region (added in #14680) was only checked for static tabindex/role attributes, never an actual keypress. - The self-scan pattern (added in #14680) only covered the plain-HTML overlay, not the RevealJS report slide or dashboard offcanvas that render the same violation markup. Extending the self-scan to RevealJS and the dashboard surfaced a real pre-existing issue: both report-chrome regions are scrollable but not keyboard-focusable once they overflow, the same class of gap already fixed for the plain-HTML overlay. Filed as #14710, not fixed here — the dashboard test filters that one known node out of the scrollable-region-focusable violation (not the whole violation), so it still catches other regressions in that scope. Related to #14710
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
3 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.
Description
Note
Was stacked on #14677 (vendored axe-core); that PR merged, and this branch is rebased onto
main.Three quality-of-life fixes to the
axe: output: documentfixed overlay in HTML output.Left-align the report overlay. The overlay is appended into
<main>, so page-level centering leaks into the report text — e.g. an about page withabout: template: jollashipsdiv.quarto-about-jolla main { text-align: center }and centers the whole report. The reveal report slide already pinstext-align: left; the base overlay rule now does the same.Don't scroll highlighted elements underneath the overlay. Hovering a selector in the report scrolls the target element into view with
scrollIntoView({block: "center"}), but the overlay isposition: fixed; bottom: 3rem; right: 3rem; max-height: 50vh— a centered element (especially a full-width one) could land partly or wholly underneath it. The scroll target is now computed by a new exported pure helper,overlayAwareScrollTop, which centers the element in the viewport band above the overlay. Elements with no horizontal overlap keep the default centering, and only the plain-HTML fixed overlay takes this path: reveal navigates slides (the report is its own slide) and the dashboard offcanvas is a static full-height panel that vertical scrolling can't dodge. The overlay itself never moves or fades, so the mouse stays inside it during the scroll and nomouseleave→ unhighlight flicker can fire.Make the overlay itself pass its own scan. Running axe-core against the injected report (it never audits itself — it's added after the scan) flagged
scrollable-region-focusable(serious, WCAG 2.1.1): once the report exceedsmax-height: 50vhit scrolls, but wasn't keyboard-focusable. The overlay is now a focusable, labeled region (tabindex="0",role="region",aria-label), added only in the fixed-overlay path — the reveal slide and dashboard offcanvas don't scroll the report element itself, so they skip the extra tab stop. The violation-target spans remain mouse-only; full keyboard operability of the report is a separate issue.Tests: unit tests for the helper's geometry (
tests/unit/axe-overlay-scroll.test.ts), a Playwright test that hovers a violation near the bottom of a long page and asserts the highlighted element settles fully above the overlay (verified to fail against the old scrolling behavior), and assertions on the existing HTML overlay test fortext-align: leftand the focusable-region attributes. A self-scan regression test runs the vendored axe-core build (window.axe) against the injected overlay on the long-report page and asserts zero violations — the overlay is added after the page scan, so nothing else ever audits it (verified this test fails withscrollable-region-focusablewhen the fix is removed).Checklist
I have (if applicable):
AI-assisted PR