Repository navigation
Avoid headings in native details demo and docs (#1160) - #1187
Open
chanellebingham wants to merge 2 commits into
Open
chanellebingham wants to merge 2 commits into
chanellebingham wants to merge 2 commits into
Conversation
Plain-text summaries in the <details> demo; readme now says to use native <details> only for content without headings and the Details component when headings are present. CHANGELOG entry under Accessibility.
stevejalim
requested review from
knowler and
stephaniehobson
and removed request for
stephaniehobson
October 21, 2026 11:24
Contributor
|
Hi @chanellebingham, thank you for the contribution. Open question #2 needs to be addressed. Please:
|
Following review: native <summary> is bold by default, native <details> has a 1.25em bottom margin, and the summary of an open <details> has a 0.5em bottom margin. A closed details doesn't collapse the summary's margin into its own, so that margin only applies while it's open. The demo summaries use mzp-u-heading-xs. CHANGELOG entry under Details.
Author
|
Thanks @stephaniehobson! Done in a2cdabc:
Checked on the built demo in headless Chromium:
|
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.
Description
Fixes #1160.
<summary>has no ARIA role of its own and some screen readers expose it as a button, so a heading placed inside it drops out of heading navigation (background in the issue). The native<details>demo put an<h3>inside each<summary>, and its readme told people they could combine the two.What changed:
components/00-basic-elements/details/details.html: plain-text summaries. The<h4>inside the second details body is gone too, so the demo matches the rule "native<details>only for content without headings".components/00-basic-elements/details/readme.md: the "combine with headings" sentence is replaced with the rule, the reason, and a pointer to the Details component for content with headings.components/details-component/readme.md: one sentence saying to use it instead of the native element when the content has headings.CHANGELOG.md: entries under HEAD > Accessibility and HEAD > Component changes > Details.assets/sass/protocol/base/elements/_details.scss(from review): native<summary>is bold by default, native<details>hasmargin: 0 0 1.25em, anddetails[open] > summaryhasmargin-bottom: 0.5em. A closed details doesn't collapse the summary's margin into its own, so that margin only applies while it's open.mzp-u-heading-xs(from review).No JS changes. The first commit changed no Sass either. The summary weight and spacing in the second commit came from review (open question 2).
CHANGELOG.md.Issue
#1160
Testing
The docs were built from
main(715e175) and from this branch, and headings were counted in headless Chrome oncomponents/preview/details.htmlandcomponents/render/details.html:document.querySelectorAll('summary :is(h1,h2,h3,h4,h5,h6)').length: 2 before, 0 after.document.querySelectorAll('details :is(h1,h2,h3,h4,h5,h6)').length: 3 before, 0 after.<summary>elements ("Summary One", "Summary Two"), so the zero is not a page that failed to load.Run on this branch with Node 24:
npm run build-package(what CI runs first:npm ci, eslint, stylelint, webpack): exit 0.npm run lintandnpm run test-build: exit 0. Thennpx jasmine-browser-runner runSpecs --browser=headlessChrome --config=tests/jasmine-browser.json: 47 specs, 0 failures.npm run build-docs: 524 of 524 items exported, no errors.summaryhas weight 700 and a bottom margin of 0px when closed and 8px when open. Eachdetailshas a 20px bottom margin. Withmzp-u-heading-xs, the open summary's margin is 14px. With the CSS frommain, the same check gives 400 and 0px.npm run lint,npm run build-packageandnpm run build-docs(524 of 524) pass.Not run locally: the Firefox half of
npm test(no Firefox on the machine used) and the other Node versions in the CI matrix. The change touches no JS.Open questions
<h4>Sub Heading</h4>from the demo body, not only the<h3>s in the summaries, reading the rule in the issue ("use<details>only for content where headings are not used") as covering body headings too. If you'd rather keep a body heading in the demo, it's a one-line revert.summaryis bold by default, the demo usesmzp-u-heading-xs, and nativedetailsandsummaryhave default margins (second commit).Made with my AI assistant, a setup I've built on Anthropic's Claude over months to read everything and prove its fixes with tests. It wrote and tested the change; I reviewed it before opening.