Skip to content

[feature] Add shell landmarks, skip link, and focus ring - #17418

Merged
mayagbarnes merged 8 commits into
developfrom
feature/app-shell-landmarks-skip-link
Oct 13, 2026
Merged

mayagbarnes merged 8 commits into
developfrom
feature/app-shell-landmarks-skip-link

Conversation

@mayagbarnes

@mayagbarnes mayagbarnes commented Oct 11, 2026 •

Copy link
Copy Markdown
Collaborator

Describe your changes

Gives keyboard and screen-reader users a way to skip app chrome (WCAG 2.4.1) and a visible focus indicator on the main scroll region (WCAG 2.4.7).

  • Renders primary content as <main id="stMain"> and multipage sidebar/top nav as <nav aria-label="App pages">
  • Adds a “Skip to main content” link as the first tab stop, shown only while it has keyboard focus. Activation focuses main and calls preventDefault(), so the URL hash stays unchanged — embed hosts mirror hashchange as UPDATE_HASH
  • Shows a muted focus ring on the main scroll region while it is keyboard-focused. The ring stays below the header and pinned while content scrolls.

Compatibility: stMain changes from <section> to <main>; data-testid and .stMain stay. Page navigation containers change from <div> to <nav>. The first Tab lands on the skip link. Custom CSS that qualifies these with a tag, such as section[data-testid="stMain"], section.stMain, div[data-testid="stSidebarNav"], or div[data-testid="stTopNav"], should drop the tag qualifier.

Firefox full-app sidebar-theme snapshots shifted slightly after the element change. Chromium and WebKit baselines did not.

GitHub Issue Link (if applicable)

Closes #17416
Closes #17338

Testing Plan

  • Unit Tests (JS and/or Python)
  • E2E Tests
  • Manual Testing

Note

Medium Risk
Changes global shell DOM semantics and tab order (first Tab hits the skip link), which can affect custom CSS and keyboard flows; embed hash behavior is intentionally altered for skip-link activation.

Overview
Improves app-shell accessibility for WCAG 2.4.1 (bypass blocks) and keyboard focus on the main scroll region.

The primary content area is now a <main id="stMain"> landmark (was <section>). Multipage sidebar and top navigation render as <nav aria-label="App pages">. A new “Skip to main content” link is the first tab stop, visually hidden until focused; activating it focuses main with preventScroll and preventDefault so the URL hash does not change (avoiding hashchange → UPDATE_HASH for embed hosts).

StyledAppViewMain adds a sticky inset focus ring (:focus-visible::before) that stays under the header while content scrolls. Unit, Playwright, and wiki accessibility docs are updated accordingly.

Compatibility: data-testid / .stMain are unchanged, but custom CSS that targets section.stMain or div[data-testid="stSidebarNav|stTopNav"] needs tag qualifiers removed.

Reviewed by Cursor Bugbot for commit a053a19. Bugbot is set up for automated code reviews on this repo. Configure here.

@mayagbarnes mayagbarnes added change:feature PR contains new feature or enhancement implementation impact:users PR changes affect end users area:accessibility Related to accessibility (a11y) compliance labels Oct 11, 2026
@snyk-io

snyk-io Bot commented Oct 11, 2026 •

Copy link
Copy Markdown
Contributor

✅ Snyk checks have passed. No issues have been found so far.

Status Scan Engine Image Critical Image High Image Medium Image Low Total (0)
✅ Open Source Security 0 0 0 0 0 issues
✅ Licenses 0 0 0 0 0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

@github-actions

github-actions Bot commented Oct 11, 2026 •

Copy link
Copy Markdown
Contributor

✅ PR preview is ready!

Name Link
📦 Wheel file https://core-previews.s3-us-west-2.amazonaws.com/pr-17418/streamlit-1.65.0-py3-none-any.whl
📦 @streamlit/component-v2-lib Download from artifacts

@mayagbarnes mayagbarnes added the ai-review If applied to PR or issue will run AI review workflow label Oct 11, 2026
@github-actions github-actions Bot removed the ai-review If applied to PR or issue will run AI review workflow label Oct 11, 2026

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

This PR adds app-shell accessibility for WCAG 2.4.1 (Bypass Blocks) and 2.4.7 (Focus Visible). stMain is rendered as <main id="stMain">, multipage sidebar and top navigation are rendered as <nav aria-label="App pages">, a visually hidden "Skip to main content" link is the first tab stop, and keyboard focus on the main scroll region draws a muted inset ring. Class stMain and data-testid="stMain" stay. The change closes #17416 and #17338, and it includes unit tests, a Playwright scenario, two Firefox snapshot updates, and accessibility inventory notes.

All three expected reviews completed (gpt-5.6-terra-xhigh, claude-opus-5-5-medium, grok-4.7-xhigh). All three request changes. They agree the landmarks and skip-link placement match the issues, that there is no public API or hot-path impact, and that the new focus ring scrolls away with stMain. Two reviews also treat the skip link's #stMain hash write as merge-blocking; the third did not flag it. Checking SkipToContentLink and AppView's hashchange listener confirms the hash is forwarded to embed hosts as UPDATE_HASH. They disagree on @pytest.mark.external_test (two Yes, one No); that is resolved below.

Product Alignment

This PR is user-facing: every app gains a main landmark, a page-navigation landmark when multipage nav is shown, a new first tab stop, and a focus indicator on the main scroll region. It adds no public st.* API, config, or CLI surface. No product spec in specs/ covers it, and none is required. specs/AGENTS.md exempts bug fixes and small non-controversial enhancements. This is that case: standard shell landmarks and a skip link, already described in #17416 and #17338. The PR body does not claim a separate product approval.

The structure matches those issues: one <main>, a named navigation landmark only where page nav is shown, a skip link that targets main, and theme.shadows.focusRingMuted (the same muted ring as header chrome) because the target is a scroll region. The hash write and the scrolling ring are implementation bugs, covered inline.

Code Quality

The change is small and local: a skip-link component, styled.main, landmark attributes, and a top-nav root so rc-overflow still measures one flex host. Keeping the landmark on the measured node is the right constraint, because an extra wrapper previously forced a false "N more". Complexity matches the feature.

The focus-ring and hash defects, and the top-nav ref fallback, are on the changed lines. Reviewers agreed on the focus ring. The hash side effect is confirmed against the existing host hash channel. The top-nav wrapper is a real findDOMNode fallback in rc-resize-observer 1.4.3 on React 18; responsive width still works through that fallback, so it is secondary to the two interaction bugs.

Performance

There is no meaningful runtime impact. The PR adds one static link, semantic attributes, and CSS that applies on :focus-visible. It adds no rerun, serialization, parsing, data traversal, extra React state, or hot-path work. The top-nav wrapper does not add a DOM node around the measured flex host. No benchmark is warranted.

Test Coverage

Unit tests cover the main landmark, the skip-link href and its position as the first child, and the named navigation landmarks in SidebarNav and TopNav. The navigation e2e covers sidebar mode: one main, one "App pages" nav, top nav absent, first Tab on the skip link, Enter focusing main, and an inset ::after shadow at scroll 0.

The tests that need to land with the fixes are an assertion that activation leaves the URL hash unchanged, and a check that the ring still frames the visible area after stMain scrolls. Top-nav landmark coverage is unit-only, and rc-overflow is mocked there, so a crowded header would not catch a measurement failure. An e2e assertion in top-nav mode, including the overflow state, is useful and optional. Two Firefox full-app sidebar theme snapshots were updated; confirm the visual diff is the intended result of the tag change.

Backwards Compatibility

There is no Python API or config break. stMain changes from section to main, the navigation containers change from div to nav, and the first Tab stop is the skip link. .stMain and data-testid="stMain" remain, which is what the PR's compatibility note preserves. Selectors that require section.stMain or div[data-testid="stSidebarNav"] will stop matching. Streamlit does not treat the internal tag name as a contract; the risk is low and worth a release-note line.

position: relative on stMain makes that element the containing block for absolutely positioned descendants. On small viewports the existing media query already set position: absolute. The shell box itself is unchanged on desktop because stMain already fills its parent. Custom CSS that positioned a descendant against an outer ancestor can shift. id="stMain" is new. An author node with the same id would duplicate it; the shell node is first in the document, so the fragment still resolves to the landmark. Chat apps that auto-pin st.chat_input still render data-testid="stAppScrollToBottomContainer"; the new id is forwarded, so the skip link still targets it.

Security & Risk

No security-sensitive surface changed: no server routes, WebSocket or session changes, asset handling, external requests, HTML injection, storage, cookies, CSP, or new dependencies. The skip-link label and href are constants. The regression is behavioral. Fragment navigation emits the existing guest-to-host UPDATE_HASH message and can change an embedded parent URL. That is not XSS or token handling.

External test recommendation

  • Recommend external_test: Yes
  • Triggered categories: 4. Embedding and iframe boundary
  • Evidence:
    • frontend/app/src/components/AppView/SkipToContentLink.tsx: href="#stMain" changes location.hash on activation.
    • frontend/app/src/components/AppView/AppView.tsx: the existing hashchange listener sends UPDATE_HASH to the host. Protocol code is unchanged; this PR adds a user action that uses that channel.
  • Suggested external_test focus areas:
    • In an embedded iframe, activate "Skip to main content" and confirm the parent URL is not rewritten to #stMain, and that focus moves into the main landmark.
    • Confirm the skip link becomes visible on keyboard focus inside the iframe.
  • Confidence: Medium
  • Assumptions and gaps: gpt-5.6-terra-xhigh recommended No because no embedding, routing, or postMessage implementation changed. claude-opus-5-5-medium and grok-4.7-xhigh recommended Yes. Resolved to Yes: embedded behavior can diverge from a local run, and a parent URL update only shows up in a host page. Hosts outside this repo (Community Cloud, SiS) were not inspected. If activation stops writing the hash, category 4 no longer hits and the recommendation becomes No. A separate claim that the ring's headerHeight offset breaks bare embed does not hold: StyledHeader always keeps theme.sizes.headerHeight, including the transparent empty header.

Accessibility

The direction matches the shell guidelines: <main> for primary content, <nav> with an accessible name for page navigation, an <a href> skip link, :focus-visible without a :focus fallback, and the main outline removed only where a replacement ring is supplied. pointer-events: none on the ring avoids an extra hit target. Sidebar and top nav are mutually exclusive, so the shared name "App pages" does not create two landmarks at once. The skip link uses theme.shadows.focusRing with body text and background colors.

The ring fails the 2.4.7 goal once main scrolls, which is the case where keyboard users focus main. The skip link does move focus, which is what 2.4.1 asks for, and it also rewrites the URL hash. Both are inline.

When the sidebar is collapsed, StyledSidebar translates off-screen and does not set aria-hidden. Print mode is the path that uses display: none. The new nav landmark stays in the accessibility tree while off-screen. Hiding it while collapsed is a follow-up: the page links were already exposed before this landmark.

Readability

frontend/app/src/components/AppView/styled-components.ts

  • Lines 115–117: "Not primary" has no clear referent, and the comment mixes the header offset with the muted-ring choice. Proposed rewrite: // Inset ring frames the area below the absolutely positioned header. Use the muted ring because stMain is a scroll region, not an actionable control.
  • Line 126: Proposed rewrite: // Above main content and below the header.
  • Lines 43–46: the eslint disable reason is hard to parse. Proposed rewrite:
/* eslint-disable streamlit-custom/no-hardcoded-theme-values --
 * The visually hidden clip pattern needs literal px values. Focused styles use theme tokens.
 */

frontend/app/src/components/AppView/SkipToContentLink.tsx

  • Lines 24–27: "Keyboard-first link" does not say what the control is. Proposed rewrite:
/**
 * Skip link that moves focus to the main landmark (WCAG 2.4.1).
 * Hidden until it receives keyboard focus.
 */

frontend/app/src/components/Navigation/TopNav.tsx

  • Lines 54–57: the JSDoc opens with the library name. If the wrapper stays, proposed rewrite:
/**
 * Top-nav overflow root. Landmark attributes live on this measured flex host;
 * an extra wrapper shrinks the measured width and shows a false "N more".
 */

frontend/app/src/components/Navigation/styled-components.ts

  • Lines 269–270: the comment repeats the TopNav explanation. Proposed rewrite: // TopNav styled components. The rc-overflow root is the <nav> landmark itself, so responsive collapse measures the correct width.

PR title and description

  • Title: [feature] Add app shell landmarks and skip link stands alone and matches [type] Description. No change.
  • Description: it leads with the change and why, and it includes the template sections. "Soft DOM note" hides the compatibility impact, and one bullet uses the internal token focusRingMuted. Proposed replacement for the note:

Compatibility note: stMain changes from a <section> to <main>; its data-testid and .stMain class remain unchanged. Page navigation containers change from <div> to <nav>. The first Tab now reaches the skip link.

Proposed replacement for the ring bullet:

Shows a muted inset focus ring on the main scroll region when it is keyboard-focused, inset below the header so the header does not cover it.

A short note on why the Firefox snapshots changed would help release-note authors.

Recommendations

  1. Activate the skip link without setting location.hash to #stMain, and assert the hash is unchanged (inline).
  2. Keep the stMain focus indicator on the visible scrollport while the region scrolls, including above the sticky bottom container, and cover that scrolled state (inline).
  3. Pass the top-nav landmark attributes through <Overflow>, or forwardRef the root, so responsive measurement does not depend on findDOMNode (inline).
  4. Optionally assert the top-nav landmark in top-nav mode, confirm the Firefox snapshot diffs, hide the sidebar nav landmark while the sidebar is collapsed, and apply the description rewrites above.

Verdict

CHANGES REQUESTED: All three reviews request changes. The landmarks and skip link are the right shell fix, but the focus ring scrolls off the visible area and the skip link writes a hash that heading anchors and embed hosts already use.


This is an automated AI review consolidated by grok-4.7-xhigh. Please verify the feedback and use your judgment.

This review also includes 5 inline comment(s) on specific code lines.

Comment thread frontend/app/src/components/AppView/SkipToContentLink.tsx
Comment thread frontend/app/src/components/AppView/styled-components.ts Outdated
Comment thread frontend/app/src/components/Navigation/TopNav.tsx Outdated
Comment thread e2e_playwright/st_navigation_test.py
Comment thread e2e_playwright/st_navigation_test.py Outdated
@github-actions github-actions Bot added the do-not-merge PR is blocked from merging label Oct 11, 2026
@mayagbarnes mayagbarnes added the ai-review If applied to PR or issue will run AI review workflow label Oct 11, 2026
@github-actions github-actions Bot removed the ai-review If applied to PR or issue will run AI review workflow label Oct 11, 2026

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

This PR gives the app shell a main landmark, a named page-navigation landmark, a keyboard skip link, and a visible focus ring on the main scroll region. It closes #17416 (WCAG 2.4.1) and #17338 (WCAG 2.4.7). stMain is now <main id="stMain">, sidebar and top page navigation are <nav aria-label="App pages">, and “Skip to main content” is the first tab stop. Skip activation calls preventDefault() before focusing main, so it does not write location.hash or fire the hashchange that AppView forwards to embed hosts. The focus ring is a sticky ::before pinned below the header. The top-nav landmark sits on rc-overflow’s measured flex host.

All three expected reviews completed (gpt-5.6-terra-xhigh, claude-opus-5-5-medium, grok-4.7-xhigh). They agree on product fit, the hash and overflow-measurement constraints, performance, security, and that external e2e coverage is unnecessary. Two approve. One requests changes for a focus-ring scroll assertion that cannot fail and for the PR’s do-not-merge label. Verified against the diff: the assertion is a real test gap and a follow-up, and the label is a draft process gate. Landmarks, skip-link activation, hash stability, and the inset ring are already covered.

Product Alignment

The PR is user-facing: every app gains a main landmark, a page-navigation landmark when multipage nav is shown, a new first tab stop, and a keyboard focus indicator on the main scroll region. It adds no public st.* API, config option, CLI surface, or dependency.

No product spec in specs/ covers this, and none is required. specs/AGENTS.md exempts small non-controversial enhancements. This is that case: standard shell landmarks and a skip link, already described in #17416 and #17338. The PR body does not claim a separate product approval, and none is needed.

The implementation matches those issues and Streamlit’s shell patterns. There is one <main>. The navigation landmark appears only where page nav is shown, and sidebar and top nav stay mutually exclusive, so the shared name “App pages” does not create two landmarks at once. The skip link is an <a href> with a plain-language label, consistent with other hard-coded chrome strings such as “Main menu”. The ring uses theme.shadows.focusRingMuted because the target is a scroll region. App authors get the behavior with no new parameters. All three reviews agree there is no material product issue.

Code Quality

The change is small and local: a stateless skip-link component, styled.main, landmark attributes, and landmark props on the existing top-nav overflow root. MAIN_ELEMENT_ID is shared by the link and <main>. ScrollToBottomContainer forwards id="stMain", so chat apps that auto-pin st.chat_input still receive the skip target.

Keeping the landmark on rc-overflow’s measured flex host is the right constraint. An extra wrapper shrinks the measured width and shows a false “N more”. The sticky ::before (fixed height, negative bottom margin, z-index above the sticky bottom container, pointer-events: none) is the most intricate part, and the comment next to it explains why that complexity is there. It matches the value of a ring that stays in the scrollport under the absolute header.

Performance

No meaningful runtime impact. The PR adds one static link, semantic attributes, and CSS that applies on :focus-visible. It adds no rerun, serialization, parsing, data traversal, extra React state, scroll listener, or work on the frontend hot paths (WebSocket, delta tree, element reconciliation, widget state, Markdown). No benchmark is warranted.

Test Coverage

Unit tests cover the main landmark and id, the skip link’s href, test id, and position as the first shell child, hash stability on activation, and the named navigation landmarks in SidebarNav and TopNav. The TopNav rc-overflow mock forwards component and the remaining props, so the landmark assertion checks real attribute forwarding.

The navigation e2e covers sidebar mode (one main, one “App pages” nav, top nav absent, first Tab on the skip link, Enter focusing main, hash unchanged) and the named landmark in top-nav mode. It also checks that the focused ring’s boxShadow is an inset shadow. Firefox sidebar-theme snapshots were updated for the section → main rendering change.

The assertions that the ring stays pinned while scrolling do not prove that. The concrete fix is in an inline comment. That gap is a follow-up: the landmarks, skip path, hash contract, and ring presence are tested.

Backwards Compatibility

There is no Python API, config, or protocol break. .stMain and data-testid="stMain" stay on the default main element. stMain changes from <section> to <main>, page-navigation containers change from <div> to <nav>, and the first Tab reaches the skip link. Selectors that require the old tag (section.stMain, div[data-testid="stSidebarNav"]) will stop matching. Streamlit does not treat the internal tag name as a contract. The risk is low, and the PR body already calls it out.

id="stMain" is new. An author node with the same id later in the tree still loses getElementById to the landmark, which is earlier in the document.

Security & Risk

No security-sensitive surface changed: no server routes, WebSocket or session changes, asset handling, external requests, HTML injection, storage, cookies, CSP, or new dependencies. The skip-link label and href are constants. Activation calls preventDefault before focusing #stMain, so it does not fire the existing hashchange listener that forwards UPDATE_HASH to embed hosts. Regression risk is limited to shell layout (the Firefox snapshots) and custom CSS that names the old tags.

External test recommendation

  • Recommend external_test: No
  • Triggered categories: None
  • Evidence:
    • frontend/app/src/components/AppView/SkipToContentLink.tsx: activation prevents fragment navigation, so location.hash and hashchange stay unchanged. The existing UPDATE_HASH host channel is not modified and is not invoked by this control.
    • frontend/app/src/components/AppView/styled-components.ts and frontend/app/src/components/Navigation/styled-components.ts: landmark tags, skip-link CSS, and the focus ring are guest-document presentation. The iframe resizer anchor (data-iframe-height) is unchanged. No route, cookie, WebSocket, iframe attribute, asset, CORS, storage, or CSP change.
  • Suggested external_test focus areas:
    • None. Local Playwright coverage of skip activation, hash stability, and landmarks is the right layer.
  • Confidence: High
  • Assumptions and gaps: Hosts outside this repo were not inspected. An earlier revision wrote #stMain and would have hit category 4 (embedding and iframe boundary); that path is removed. The recommendation would change if skip-link activation began writing the fragment or altered host-to-guest messaging.

Accessibility

The shell follows the frontend accessibility guidelines. Primary content is <main>. Page navigation is <nav> with an accessible name. The skip control is an <a href> (with href="#stMain" kept as the no-JS fallback). Focus styling uses :focus-visible, with theme.shadows.focusRing on the skip link and theme.shadows.focusRingMuted on the scroll region. The main outline is removed where that ring replaces it. The skip link uses the standard clip pattern, so it stays in the accessibility tree while visually hidden.

When the sidebar is collapsed, StyledSidebar translates it off-screen. display: none applies in print mode. The new nav landmark stays in the accessibility tree while off-screen. The page links were already exposed before this landmark. Marking the collapsed landmark inert is a reasonable follow-up.

Readability

frontend/app/src/components/Navigation/TopNav.tsx

  • Lines 48–51 and 170–171: the JSDoc calls TopNav the “overflow root,” and the JSX comment repeats the measurement constraint. One reviewer preferred deleting the JSDoc and keeping the JSX comment; another preferred one component JSDoc. Frontend guidance prefers JSDoc on components, so the JSDoc should name TopNav and state the false “N more” constraint, and the JSX comment should go. Proposed rewrite is in the inline comment.

frontend/app/src/components/AppView/AppView.test.tsx

  • Line 204: the comment says the assertion proves the skip link is the first focusable control, but it only checks that it is the first child. The e2e test is the tab-order check. Proposed rewrite: // The app shell renders the skip link before all other chrome.

e2e_playwright/st_navigation_test.py

  • Line 50: the docstring covers landmarks and the skip link, while the test also checks the focus ring. Proposed rewrite: """Shell exposes main and page-nav landmarks, a skip link into main, and a focus ring on main."""

frontend/app/src/components/AppView/SkipToContentLink.tsx

  • The JSDoc explains the non-obvious preventDefault() reason (embed UPDATE_HASH). No change.

PR title and description

  • Title: [feature] Add app shell landmarks and skip link stands alone and matches [type] Description. No change.
  • Description: the opening states the user outcome, the WCAG criteria, and the compatibility and Firefox snapshot notes. One improvement to the compatibility note, so readers know which selectors to edit: Custom CSS using section[data-testid="stMain"] or section.stMain should drop the section tag (for example [data-testid="stMain"] or .stMain).

Recommendations

  1. Tighten or drop the focus-ring pin assertions in test_app_shell_landmarks_and_skip_link (inline comment). This can land as a follow-up.
  2. Collapse the duplicated TopNav comments into one JSDoc (inline comment).
  3. When writing release notes, mention the <section> to <main> change for custom CSS that names the tag, and name the selectors in the PR compatibility note.
  4. Optionally mark the sidebar nav landmark inert while the sidebar is collapsed off-screen.
  5. Remove the do-not-merge label when the draft is ready to merge. That label is a process gate and does not change this code-review verdict.

Verdict

APPROVED: Landmarks, the skip link, and the scroll-region focus ring match the two issues, the hash and top-nav measurement fixes hold, and the remaining test, comment, and draft-label notes are follow-ups.


This is an automated AI review consolidated by grok-4.7-xhigh. Please verify the feedback and use your judgment.

This review also includes 3 inline comment(s) on specific code lines.

Comment thread e2e_playwright/st_navigation_test.py Outdated
Comment thread frontend/app/src/components/Navigation/TopNav.tsx Outdated
Comment thread frontend/app/src/components/AppView/styled-components.ts Outdated
@github-actions github-actions Bot removed the do-not-merge PR is blocked from merging label Oct 11, 2026
@mayagbarnes
mayagbarnes marked this pull request as ready for review October 12, 2026 00:06
Copilot AI balanced review requested due to automatic review settings October 12, 2026 00:06

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@greptile-apps

greptile-apps Bot commented Oct 12, 2026 •

Copy link
Copy Markdown
Contributor

RetriggerConfidence Score: 5/5

[Low impact] The PR appears safe to merge; no blocking issues remain.

Summary

Adds main and page-navigation landmarks, a keyboard skip link, and a focus ring around main content.

  • Keyboard users can skip app chrome and focus the main content.
  • Page navigation is marked as “App pages” in either layout.

Reviews (3) · Last reviewed commit: "Address final-review readability and foc..." · Reviewed by Greptile

Comment thread e2e_playwright/st_navigation_test.py Outdated
@mayagbarnes mayagbarnes added the ai-final-review If applied to PR or issue will run AI review workflow with the latest generation of AI models. label Oct 12, 2026
@github-actions github-actions Bot removed the ai-final-review If applied to PR or issue will run AI review workflow with the latest generation of AI models. label Oct 12, 2026

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

This PR improves app-shell accessibility for every app. Primary content becomes <main id="stMain">, multipage sidebar and top navigation become <nav aria-label="App pages">, and “Skip to main content” is the first tab stop. Activating the skip link focuses main with preventScroll and preventDefault(), so it does not write location.hash or send UPDATE_HASH to embed hosts. A muted inset :focus-visible ring frames the main scrollport below the header and stays pinned while content scrolls. It closes #17416 (WCAG 2.4.1) and #17338 (WCAG 2.4.7).

Consolidated from four completed reviews: gpt-5.6-sol-high, claude-opus-5-5-high, gemini-3.8-flash-high, and grok-4.7-xhigh. None failed. All four approve. They agree the change is a small accessibility fix, needs no product spec, stays off the runtime hot paths, and is covered by unit tests, Playwright, and a green CI run (js-unit-tests, playwright-e2e-tests). They differ on title wording, how explicit the CSS migration note should be, and how strict the sticky-ring e2e probe should be. Those differences are non-blocking; resolutions are in the sections below.

Product Alignment

The PR has user-facing product impact: keyboard tab order, a focus-revealed skip link, a focus ring on the main scroll region, and the element type of the main and page-nav containers. It adds no public st.* command, parameter, config key, CLI flag, or return type.

No product spec for this work is merged on develop, and the PR body does not record a separate product approval. A spec is not required. #17338 is a bug fix for a missing focus indicator, and #17416 is a small accessibility enhancement whose shape is the standard WCAG pattern (main landmark, named navigation landmark, skip link). specs/AGENTS.md exempts bug fixes and small non-controversial enhancements. All four reviewers agree.

The implementation matches those issues and the API principles that apply to shell behavior:

  • Landmarks and the skip link are always on. The common case stays free of new configuration.
  • Sidebar and top nav share the accessible name “App pages” and stay mutually exclusive.
  • The skip link keeps href="#stMain" and suppresses fragment navigation when JavaScript runs, so embed hosts do not mirror #stMain into the parent URL.
  • The ring uses the existing focusRingMuted token, the same treatment header chrome uses for non-primary controls.
  • .stMain and data-testid="stMain" stay, so the semantic tag change is the smallest migration for a real <main> landmark.

Code Quality

The change is localized and proportionate: a stateless SkipToContentLink, a shared MAIN_ELEMENT_ID, landmark attributes on the elements that already own those regions, and focus-only CSS. All four reviewers agree there is no extra state, effect, or dependency.

Passing the <nav> through rc-overflow’s measured root, instead of wrapping it, is the right fix. An extra wrapper shrinks the measured width and produces a false “N more” overflow. ScrollToBottomContainer forwards id, so #stMain still exists when the auto-positioned chat input replaces StyledAppViewMain.

The sticky ::before (height 100dvh - headerHeight, equal negative margin-bottom, top: headerHeight, z-index just above the sticky bottom container) is the densest part of the diff. That complexity matches the layout constraints: the ring has to frame the area under the absolute header, stay pinned while stMain scrolls, paint above stBottom, and leave content position unchanged.

One suggested cleanup was not adopted. VISUALLY_HIDDEN_STYLES in frontend/lib/src/theme/consts.ts matches the skip link’s unfocused clip values, but it is not part of the @streamlit/lib public export, and frontend/lib/src/index.ts keeps that surface minimal. The Navigation visually-hidden copy is pre-existing. The local clip styles and the scoped eslint-disable are appropriate here.

Performance

No meaningful runtime impact. The work is outside the WebSocket, delta-tree, widget-state, markdown, and large-data hot paths. The skip link is one static anchor. The ring is a :focus-visible pseudo-element, so it is absent during ordinary rendering and reruns. No new listeners, subscriptions, or unstable props are introduced. check-bundle-size and analyze-frontend-bundle passed. Performance benchmarks are not warranted.

Test Coverage

Coverage is sufficient for merge. Frontend unit tests check the main landmark and target id, skip-link placement as the first child, activation without changing location.hash, and the named navigation landmark for both sidebar and top nav. The TopNav mock forwards component and the remaining props, so the landmark assertion is real.

test_app_shell_landmarks_and_skip_link covers first-Tab order, Enter activation, unchanged hash, no content shift on focus, an inset ring, and the ring staying under the header after the scrollport overflows. Top-navigation mode checks the named landmark. Firefox sidebar-theme snapshots were updated for the tag change; Chromium and WebKit baselines were unchanged. js-unit-tests and playwright-e2e-tests are green.

The scroll-pin check has to use a real node, because a pseudo-element has no getBoundingClientRect. Reviewers split on that stand-in: one would replace it with a screenshot, another would keep it and also copy margin-bottom. The stand-in is a reasonable way to measure sticky geometry. It should also copy marginBottom, because that negative margin is what pins the ring; the header-alignment check can pass without it. That tightening is inline and does not block merge.

Backwards Compatibility

Python, protobuf, config, and protocol surfaces are unchanged. Widget state, routing, and script APIs behave as before. Skip activation leaves any existing fragment in the URL.

Two observable shell changes are intentional:

  • stMain changes from <section> to <main>. Page navigation changes from <div> to <nav>. .stMain, data-testid="stMain", stSidebarNav, and stTopNav stay.
  • The first Tab stop in every app is the skip link.

Custom CSS that qualifies these nodes with a tag will stop matching. Streamlit does not guarantee custom CSS, and the PR already says to drop section from section.stMain and section[data-testid="stMain"]. Reviewers differ on whether that note is complete. The prose does state the <div> to <nav> change, which is enough to merge. The selector examples still omit div[data-testid="stSidebarNav"] and div[data-testid="stTopNav"], which are the selectors community snippets use to hide the page nav or inject a logo. Adding those examples to the compatibility note, and to the release notes if this PR is included, is a follow-up documentation improvement.

Security & Risk

No new trust boundary. The skip-link label and aria-label are static strings. There is no user HTML, eval, new dependency, cookie, storage, auth, or postMessage protocol change. Suppressing fragment navigation avoids pushing #stMain through the existing embed UPDATE_HASH path. The ring is pointer-events: none, so it does not intercept clicks. The main regression risk is visual and selector-based, and it is covered or documented above.

External test recommendation

  • Recommend external_test: No
  • Triggered categories: None
  • Evidence:
    • frontend/app/src/components/AppView/SkipToContentLink.tsx: activation calls preventDefault() and focuses #stMain. It does not change location.hash or the hashchange listener in AppView that forwards UPDATE_HASH. Unit and e2e tests assert the hash stays the same.
    • frontend/app/src/components/AppView/styled-components.ts, frontend/app/src/components/Navigation/SidebarNav.tsx, frontend/app/src/components/Navigation/TopNav.tsx: guest-document landmarks and CSS. Iframe sizing, sandbox, postMessage, routing, storage, assets, CORS, and security headers are unchanged.
  • Suggested external_test focus areas:
    • None required. Local Playwright coverage verifies focus transfer and hash stability at the layer this PR changes.
  • Confidence: High
  • Assumptions and gaps: External host implementations were not inspected. One review rated confidence medium-high because preventScroll inside a cross-origin iframe was not run on an external host. That is an unverified assumption, and it does not hit the embedding checklist: this PR does not modify host-to-guest messaging, iframe attributes, or resize behavior. The recommendation would change if activation wrote the fragment or changed postMessage handling.

Accessibility

The shell changes match #17416, #17338, and the frontend accessibility rules. Primary content is a single <main>. Page navigation is a named <nav> only when multipage nav is shown, with one landmark at a time. The skip link is a real anchor, stays in the accessibility tree, is visually hidden until :focus-visible, and moves focus to main. stMain keeps tabIndex={0} for keyboard scrolling and replaces the removed outline with a :focus-visible ring. Print hides the skip link and the ring. The link does not set aria-hidden on a focusable control.

focusRingMuted is already scored in wiki/accessibility-theme.md as below WCAG 1.4.11’s 3:1 non-text contrast target (about 1.5:1 light, 1.8:1 dark) on header chrome. This PR reuses that token for the new main-region indicator and does not claim to close that theme-wide gap. A visible ring still fixes the previous 2.4.7 failure (outline removed, no replacement). Changing the global token is out of scope. This is not merge-blocking.

Collapsed sidebars remain in the accessibility tree (existing translateX behavior). The new nav landmark sits inside that sidebar, so rotor users can still reach “App pages” while the sidebar is visually collapsed. That matches the current sidebar region.

Safari skips links during Tab unless “Press Tab to highlight each item” is enabled. That is a platform limitation of skip links, and it is specific to Safari’s default keyboard settings.

Readability

Line-specific comment and test-name nits are inline. Remaining findings:

PR title

Issue: [feature] Add app shell landmarks and skip link matches [type] Description, stands alone, and is within the length limit (45 characters). Two reviews would leave it. Two would mention the focus ring, which is the #17338 half of the user-visible change. The current title is acceptable for merge.

Proposed rewrite (optional): [feature] Add shell landmarks, skip link, and focus ring

PR description

Issue: The opening states the change and the WCAG purpose, and the compatibility note belongs there. Two reviews found the description sufficient. The other two are right about two gaps: the decision to avoid writing location.hash is stated without the embed-host reason (that reason appears in the generated summary), and the migration examples cover section.stMain while the <div> to <nav> change is only in prose. The third bullet also narrates CSS mechanics the diff already shows. The Firefox snapshot paragraph is useful.

Proposed rewrite (optional):

## Describe your changes

Gives keyboard and screen-reader users a way to skip app chrome (WCAG 2.4.1) and a visible focus indicator on the main scroll region (WCAG 2.4.7).

- Renders primary content as `<main id="stMain">` and multipage sidebar/top nav as `<nav aria-label="App pages">`
- Adds a “Skip to main content” link as the first tab stop, shown only while it has keyboard focus. Activation focuses main and calls `preventDefault()`, so the URL hash stays unchanged — embed hosts mirror `hashchange` as `UPDATE_HASH`
- Shows a muted focus ring on the main scroll region while it is keyboard-focused. The ring stays below the header and pinned while content scrolls.

Compatibility: `stMain` changes from `<section>` to `<main>`; `data-testid` and `.stMain` stay. Page navigation containers change from `<div>` to `<nav>`. The first Tab lands on the skip link. Custom CSS that qualifies these with a tag, such as `section[data-testid="stMain"]`, `section.stMain`, `div[data-testid="stSidebarNav"]`, or `div[data-testid="stTopNav"]`, should drop the tag qualifier.

Firefox full-app sidebar-theme snapshots shifted slightly after the element change. Chromium and WebKit baselines did not.

frontend/app/src/components/Navigation/TopNav.tsx (TopNav JSDoc, lines 48–53)

Issue: “Landmark attributes are passed” hides the actor, and the second sentence packs the constraint into one clause.

Proposed rewrite:

/**
 * Responsive top navigation, rendered as the "App pages" `<nav>` landmark.
 * Pass landmark attributes to rc-overflow's root, which is the measured flex host.
 * Wrapping that root in another element shrinks the measured width and shows a false "N more" overflow.
 */

frontend/app/src/components/AppView/styled-components.ts (eslint disable, lines 43–45)

Issue: The disable wraps the whole skip-link component. The second sentence can be read as saying the focused styles need the hardcoded pixels.

Proposed rewrite:

/* eslint-disable streamlit-custom/no-hardcoded-theme-values --
 * The unfocused clip pattern needs literal px values. Focused styles use theme tokens.
 */

Recommendations

  1. Optionally extend the compatibility note, and the release notes if this PR is included, with div[data-testid="stSidebarNav"] and div[data-testid="stTopNav"].
  2. Optionally apply the title, description, and comment rewrites in Readability.
  3. Confirm the click-then-keyboard focus ring in Chrome, Firefox, and Safari. The question is inline and does not block merge.

Verdict

APPROVED: All four reviews approve, and the landmarks, skip link, and pinned focus ring match #17416 and #17338 with no merge-blocking bug, security issue, or test gap.


This is an automated AI review by grok-4.7-xhigh. Please verify the feedback and use your judgment.

This review also includes 7 inline comment(s) on specific code lines.

Comment thread frontend/app/src/components/AppView/styled-components.ts
Comment thread e2e_playwright/st_navigation_test.py
Comment thread frontend/app/src/components/AppView/styled-components.ts
Comment thread frontend/app/src/components/AppView/SkipToContentLink.tsx Outdated
Comment thread e2e_playwright/st_navigation_test.py Outdated
Comment thread frontend/app/src/components/AppView/styled-components.ts Outdated
Comment thread frontend/app/src/components/Navigation/styled-components.ts Outdated
@mayagbarnes mayagbarnes changed the title [feature] Add app shell landmarks and skip link [feature] Add shell landmarks, skip link, and focus ring Oct 12, 2026
@mayagbarnes
mayagbarnes merged commit 5557b5a into develop Oct 13, 2026
38 checks passed
@mayagbarnes
mayagbarnes deleted the feature/app-shell-landmarks-skip-link branch October 13, 2026 03:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:accessibility Related to accessibility (a11y) compliance change:feature PR contains new feature or enhancement implementation impact:users PR changes affect end users

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Improve app-shell landmarks and add a skip link (WCAG 2.4.1) Main scroll region (stMain) takes keyboard focus with no visible focus indicator

3 participants