Skip to content

perf(ActionList): replace :has(~ .SubGroup [data-active]) collapsed-parent indicator with attribute selector#7902

Closed
mattcosta7 wants to merge 5 commits into
mainfrom
perf/actionlist-subgroup-active-indicator
Closed

perf(ActionList): replace :has(~ .SubGroup [data-active]) collapsed-parent indicator with attribute selector#7902
mattcosta7 wants to merge 5 commits into
mainfrom
perf/actionlist-subgroup-active-indicator

Conversation

@mattcosta7

Copy link
Copy Markdown
Contributor

Closes #

Replaces the most expensive :has() selector remaining in packages/react/src/ActionList/ActionList.module.css&:has(~ .SubGroup [data-active='true']) — with a plain attribute + child-combinator selector. This was the worst-shape :has() left in the package: it combined a general sibling combinator with a descendant search, and the engine had to re-run that walk against every collapsed parent any time a descendant data-active attribute changed.

Before:

.ActionListContent[aria-expanded='false']:has(~ .SubGroup [data-active='true']) {
  /* active-parent indicator styles */
}

After:

.ActionListItem[data-has-subitem='true'][data-active='true']
  > .ActionListContent[aria-expanded='false'] {
  /* active-parent indicator styles */
}

The selector goes from a sibling walk + descendant subtree search (re-evaluated on every data-active mutation inside the subtree) to a constant-time attribute lookup on the same <li> that the consumer already controls.

Why this is safe:

The new selector depends on data-active being set on the parent <li>. The only in-repo consumer that renders an ActionList.Item with ActionList.SubItem is NavList.ItemWithSubNav, which already does its own hasCurrentNavItem recursion and passes active={!isOpen && containsCurrentItem} to the parent — exactly matching the condition the old :has() was detecting. Net visual output is identical for NavList.SubNav usage.

Behavior change for direct ActionList.Item + ActionList.SubItem consumers

If a consumer renders ActionList.SubItem directly (without NavList.SubNav) and relied on the :has() selector to auto-light-up a collapsed parent when a descendant had data-active='true', they will now need to pass active={true} to the parent ActionList.Item themselves. This is the same pattern NavList.ItemWithSubNav already implements; no other in-repo usage exists.

Changelog

Changed

  • Internal: ActionList.module.css no longer uses :has() to detect an active descendant in a collapsed parent's sub-group; the same styles fire when the parent ActionList.Item itself has active={true} (which NavList.ItemWithSubNav already sets via its existing containsCurrentItem walk).

Removed

  • The &:has(~ .SubGroup [data-active='true']) selector from ActionList.module.css.

Rollout strategy

  • Patch release
  • Minor release
  • Major release; if selected, include a written rollout or migration plan
  • None; if selected, include a brief description as to why

Testing & Reviewing

  • All 121 packages/react/src/ActionList/** + packages/react/src/NavList/** unit tests pass.
  • tsc --noEmit on packages/react is clean.
  • Stylelint + Prettier + ESLint clean on touched files (only pre-existing -webkit-tap-highlight-color browser-compat warning at line 509, unrelated to this change).

VRT expectations: No change to NavList.WithSubItems / NavList.WithNestedSubItems / NavList.WithTrailingActionInSubItem renderings — the parent ActionList.Item already carries data-active='true' whenever the previous :has() would have matched.

Companion to #7901 (PageHeader) and #7894 (other ActionList :has() selectors).

Merge checklist

@changeset-bot

changeset-bot Bot commented May 29, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: ad88153

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@primer/react Patch

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

@github-actions github-actions Bot added the integration-tests: recommended This change needs to be tested for breaking changes. See https://arc.net/l/quote/tdmpakpm label May 29, 2026
@github-actions

Copy link
Copy Markdown
Contributor

⚠️ Action required

👋 Hi, this pull request contains changes to the source code that github/github-ui depends on. If you are GitHub staff, test these changes with github/github-ui using the integration workflow. Check the integration testing docs for step-by-step instructions. Or, apply the integration-tests: skipped manually label to skip these checks.

To publish a canary release for integration testing, apply the Canary Release label to this PR.

@github-actions github-actions Bot temporarily deployed to storybook-preview-7902 July 7, 2026 15:20 Inactive
@mattcosta7 mattcosta7 marked this pull request as ready for review July 7, 2026 16:18
Copilot AI review requested due to automatic review settings July 7, 2026 16:18
@mattcosta7 mattcosta7 requested a review from a team as a code owner July 7, 2026 16:18
@mattcosta7 mattcosta7 requested a review from liuliu-dev July 7, 2026 16:18

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.

Pull request overview

This PR reduces CSS selector-matching cost in ActionList by removing a high-cost :has(~ .SubGroup …) rule and replacing it with a constant-time attribute-based selector driven by state already computed by NavList.ItemWithSubNav.

Changes:

  • Replaced the collapsed-parent “active descendant” indicator selector from :has(~ .SubGroup [data-active='true']) to .ActionListItem[data-has-subitem='true'][data-active='true'] > .ActionListContent[aria-expanded='false'].
  • Added a changeset documenting the internal performance change and the behavioral expectation for direct ActionList.Item + ActionList.SubItem consumers.
Show a summary per file
File Description
packages/react/src/ActionList/ActionList.module.css Removes the expensive :has() rule and applies the same styles using parent data-active + data-has-subitem attributes.
.changeset/perf-actionlist-subgroup-active-indicator.md Adds a patch changeset describing the selector change and the updated expectation for parent active state.

Review details

  • Files reviewed: 2/2 changed files
  • Comments generated: 2
  • Review effort level: Low

Comment thread packages/react/src/ActionList/ActionList.module.css Outdated
'@primer/react': patch
---

ActionList: replace the `:has(~ .SubGroup [data-active='true'])` selector that styled collapsed parent items containing an active descendant with a plain attribute + child-combinator selector. The collapsed-parent active indicator now depends on the parent item carrying `data-active='true'`, which the supported `NavList.ItemWithSubNav` consumer already sets via its `containsCurrentItem` walk.
…eset

The `& + .ActionListItem .ActionListSubContent::before` half never matched
(.ActionListItem is the parent of .ActionListContent, not a sibling) and was
pre-existing dead code on main. Removing it preserves rendered output.
@github-actions github-actions Bot requested a deployment to storybook-preview-7902 July 7, 2026 16:38 Abandoned
@github-actions github-actions Bot temporarily deployed to storybook-preview-7902 July 7, 2026 16:48 Inactive
@github-actions github-actions Bot temporarily deployed to storybook-preview-7902 July 7, 2026 17:18 Inactive
@mattcosta7 mattcosta7 added the Canary Release Apply this label when you want CI to create a canary release of the current PR label Jul 7, 2026

Copy link
Copy Markdown
Contributor Author

Closing as no longer necessary. Main already merged #8115 ("perf(css): remove expensive :has() selectors for Safari style-recalc"), which removes the same :has(~ .SubGroup [data-active='true']) collapsed-parent indicator selector this PR targeted. After merging main, this branch has a net diff of zero against main, so there is nothing left to ship.

@mattcosta7 mattcosta7 closed this Jul 7, 2026
@primer primer Bot deleted the perf/actionlist-subgroup-active-indicator branch July 8, 2026 00:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Canary Release Apply this label when you want CI to create a canary release of the current PR integration-tests: recommended This change needs to be tested for breaking changes. See https://arc.net/l/quote/tdmpakpm

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants