Fix dashboard URL hash handling - #14820
Open
cwickham wants to merge 3 commits into
Open
Conversation
showPage() removes `active` from every nav tab and every .tab-pane, then restores it only on the pane matching the hash. When the hash names no page, nothing is restored and the dashboard shows an empty content area. Both callers passed the hash through unchecked: the processing on load and the popstate handler, which runs on every hash change. So any in-page anchor -- a footnote link, a cross-reference, a hand-written anchor -- blanked a dashboard with more than one page. Guard both with isPage(). isPage() itself built a CSS selector by concatenating the hash, which throws a SyntaxError for a hash that is not a valid selector (`#`, `#1foo`, `#a:b`). On load that throw would happen before the `hidden` class is removed, leaving the whole dashboard invisible, so switch it to getElementById. #9411 was a narrower instance of this, fixed in #11264 by keeping external links away from showPage(); the load and popstate paths were left unchanged. Closes #14818
A dashboard navbar links to its pages with a URL hash, so `#sales` is the shareable URL for a page. Loading such a URL makes the browser set the sequential focus navigation starting point to the target element, which for a dashboard is the whole .tab-pane. The first Tab press then landed inside the page content, and the navbar -- earlier in DOM order -- could not be reached by tabbing forward at all. A page hash selects a page; it is not a position within one. So when the hash names a page, put the focus starting point back at the top of the document by focusing the body. This has to run after the browser has applied its own fragment behaviour, which is later than DOMContentLoaded, hence the deferral to load. Closes #14819
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. |
The focus-order test asserted that the navbar tab is the second stop. What sits at the top of the body is a format concern and can change -- a skip-to-content link would add a stop before it. Tab forward a bounded number of times and assert the navbar tab is reached, which tests the property the issue is about: the navbar is reachable by tabbing forward.
Member
Author
|
Stacked below #14685, which adds the skip-to-content link. That PR targets this branch and exposed both of these bugs on every dashboard, but neither is caused by it — both reproduce on |
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.
Description
Two dashboard bugs in URL hash handling. Both predate #14685; both surfaced while reviewing it.
Closes #14818. Closes #14819.
A hash that names no page blanked the dashboard (#14818)
showPage()removesactivefrom every nav tab and every.tab-pane, then restores it only on the pane that matches the hash. When no pane matches, nothing is restored and the content area is empty.Two callers passed the hash through without a test: the processing on load, and the
popstatehandler, which runs on every hash change. Any in-page anchor therefore blanked a dashboard with more than one page. A footnote link, a cross-reference, or a hand-written anchor all did it, and a reload of that URL showed the same empty state.Both callers now test with
isPage()first.isPage()needed work before it could be used as a guard. It built a CSS selector by concatenating the hash, which throws aSyntaxErrorfor a hash that is not a valid selector, such as#,#1foo, or#a:b. On load, that throw would happen before thehiddenclass is removed, and the whole dashboard would stay invisible. It now usesgetElementById.#9411 was a narrower instance of the same root cause. #11264 closed it by keeping external links away from
showPage(), and left the load andpopstatepaths unchanged.Landing on a page hash put the navbar out of reach (#14819)
A dashboard navbar links to its pages with a URL hash, so
dashboard.html#salesis the shareable URL for a page. Loading such a URL makes the browser set the sequential focus navigation starting point to the target element, which for a dashboard is the whole.tab-pane. The first Tab press landed inside the page content, and the navbar comes earlier in DOM order, so tabbing forward never reached it.A page hash selects a page. It is not a position within one. When the hash names a page, the focus starting point now returns to the top of the document. This runs after
load, because the browser applies its own fragment behavior later thanDOMContentLoaded.Tests
New Playwright spec at
tests/integration/playwright/tests/dashboard-hash-navigation.spec.ts, with a fixture attests/docs/playwright/dashboard/hash-navigation.qmd. This is the first dashboard spec in the suite.Five tests, passing on Chromium, Firefox, and WebKit:
Verified against a build without the fix: four of the five fail. The fifth is the navigation regression guard, which passes either way.
WebKit only tabs to links when Alt is held, which matches Safari's default setting, so the focus test picks its key by browser.
Checklist
I have (if applicable):
AI-assisted PR