Fix invalid HTML nesting in ActionList.Heading#8066
Conversation
Apply the visually-hidden styles directly to the heading element instead of wrapping it in a VisuallyHidden span. A heading is not valid phrasing content inside a span, so the wrapper produced invalid HTML for every ActionList.Heading instance. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
🦋 Changeset detectedLatest commit: 0c52807 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
|
Integration test results from github/github-ui PR:
CI check runs linting, type checking, and unit tests. Check the workflow logs for specific failures. Need help? If you believe this failure is unrelated to your changes, please reach out to the Primer team for assistance. |
Note for reviewers: the github-ui integration test failures are unrelated to this PRThe auto-generated integration test (github/github-ui#24809) shows failing What's actually failing: every error references Root cause — version skew:
This PR's only diff vs What resolves the integration test (all github-ui-side, after release):
Safe to ignore for this PR. The unit tests covering this change (ActionList + NavList) pass. |
Background
ActionList.Headingrenders the section heading for anActionList(e.g. anh1–h6that labels the list). Until now it wrapped that heading in aVisuallyHiddencomponent:VisuallyHiddenalways renders a<span>around its children — it only toggles a CSS class to hide/show. That means everyActionList.Headingended up as a heading nested inside a<span>, regardless of whethervisuallyHiddenwas set.That nesting is invalid HTML: a heading (
<h1>–<h6>) is flow content, and a<span>may only contain phrasing content. So we were emitting a<span><h2>…</h2></span>structure that browsers technically have to error-correct.ActionListis used very widely (including indirectly byNavList), so this affected a lot of pages.This is the same problem — and the same fix — we just landed for
NavList.Headingin #8031. This PR bringsActionList.Headingin line.What changed
Instead of wrapping the heading in a
VisuallyHiddenspan, we apply the visually-hidden CSS class directly to the heading element usingclsx:The wrapping
<span>is gone, so the heading is now a direct child of the list container — valid HTML.One detail worth calling out: unlike
NavList.Heading, theActionListHeaderclass (which provides the heading's margins and thedata-list-variant-based spacing) must stay on the heading in all states. So the visually-hidden class is added on top ofActionListHeaderonly whenvisuallyHiddenis true — they are not mutually exclusive here.Changed
ActionList.Headingno longer wraps its heading in aVisuallyHidden<span>; the visually-hidden styles are applied directly to the heading element.Removed
<span>wrapper around theActionList.Headingelement.Why this is safe
ActionListis high-traffic, so we checked the things that could plausibly depend on the old structure:List.tsxandGroup.tsxderivearia-labelledbyfrom the heading slot's React props (id ?? headingId), not from the DOM. Removing the wrapper span doesn't change the id wiring.List.tsxrenders{slots.heading}directly; only the wrapper element disappears, the heading itself is still emitted.ActionListHeader(margins) stays on the heading in every state. WhenvisuallyHiddenis set,InternalVisuallyHiddenis added (absolute position, 1px clip), matching the previous behaviour.ActionList.GroupHeadingis not affected. It uses its own local heading component and only imports the type fromHeading.tsx.Testing & Reviewing
ActionList/Heading.test.tsxcovering: the heading is no longer wrapped in a<span>,visuallyHiddenappliesInternalVisuallyHiddento the heading, and the default (visible) case keeps onlyActionListHeader.ActionListandNavListunit suites together (NavList consumes ActionList.Heading indirectly): all passing.This branch is rebased on top of #8031 (NavList.Heading), since the two changes are closely related.
Rollout strategy
Merge checklist