Skip to content

perf(Button): replace :has(.Visual) with data-no-visuals attribute#7597

Open
hectahertz wants to merge 2 commits intomainfrom
hectahertz/perf-button-css-has-selector
Open

perf(Button): replace :has(.Visual) with data-no-visuals attribute#7597
hectahertz wants to merge 2 commits intomainfrom
hectahertz/perf-button-css-has-selector

Conversation

@hectahertz
Copy link
Contributor

@hectahertz hectahertz commented Feb 26, 2026

Closes #

Replaces 4 CSS :has(.Visual) / :not(:has(.Visual)) selectors in the Button link variant with [data-no-visuals] / :not([data-no-visuals]) attribute selectors.

ButtonBase.tsx already sets data-no-visuals on the button element when no visuals are present, but the link variant underline styling in CSS was still using :has(.Visual) to detect them. :has() forces the browser to re-evaluate style on the parent whenever any descendant's class list changes, while attribute selectors only check the element itself.

This is a continuation of #7327 which went stale. Same approach, rebased on current main.

Related PRs: #7556 (merged, ActionList :has() fix), #7327 (stale, previous attempt at this exact fix)

Changelog

New

N/A

Changed

  • Button link variant CSS now uses [data-no-visuals] attribute selectors instead of :has(.Visual) for visual detection

Removed

N/A

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

Pure CSS selector replacement. The data-no-visuals attribute was already being set in ButtonBase.tsx, so this is a CSS-only change. Specificity is preserved: both :has(.Visual) and [data-no-visuals] have specificity 0,1,0.

  1. Open Storybook, navigate to Button > Link Variant stories
  2. Verify underline behavior with and without visuals matches current behavior
  3. Toggle the data-a11y-link-underlines attribute on an ancestor to verify both branches work

Merge checklist

@hectahertz hectahertz requested a review from a team as a code owner February 26, 2026 11:47
@hectahertz hectahertz requested review from Copilot and francinelucca and removed request for Copilot February 26, 2026 11:47
@changeset-bot
Copy link

changeset-bot bot commented Feb 26, 2026

🦋 Changeset detected

Latest commit: 0c34541

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 Feb 26, 2026
@github-actions
Copy link
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. Or, apply the integration-tests: skipped manually label to skip these checks.

Copilot AI review requested due to automatic review settings February 26, 2026 11:54
@github-actions github-actions bot requested a deployment to storybook-preview-7597 February 26, 2026 11:57 Abandoned
Copy link
Contributor

Copilot AI left a comment

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 improves performance in @primer/react’s Button link variant styling by replacing expensive :has(.Visual) selectors with constant-time [data-no-visuals] / :not([data-no-visuals]) attribute selectors, leveraging an attribute already set by ButtonBase.

Changes:

  • Replace 4 occurrences of :has(.Visual) / :not(:has(.Visual)) in link-underline styling with [data-no-visuals] selectors.
  • Add a patch changeset documenting the perf improvement.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
packages/react/src/Button/ButtonBase.module.css Swaps :has(.Visual)-based underline logic for [data-no-visuals] attribute selectors in link variant blocks.
.changeset/perf-button-css-has-selector.md Patch changeset entry for the Button perf improvement.
Comments suppressed due to low confidence (1)

packages/react/src/Button/ButtonBase.module.css:606

  • Same data-no-visuals vs :has(.Visual) behavior change applies here: in a loading state with no provided visuals/actions, the loading spinner adds a .Visual descendant but data-no-visuals remains present, so this block will apply where the old :not(:has(.Visual)) block would not. If the intent is to keep loading behavior consistent with the pre-change selectors, consider excluding [data-loading='true'] from the [data-no-visuals] branch (or adjusting how the attribute is computed).
    &[data-no-visuals] {
      text-decoration: none;
      background-image: none;
    }

    &:not([data-no-visuals]) {

Copy link
Member

@francinelucca francinelucca left a comment

Choose a reason for hiding this comment

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

LGTM, curious about copilot's review comment though 👀

@primer-integration
Copy link

👋 Hi from github/github-ui! Your integration PR is ready: https://github.com/github/github-ui/pull/14652

@primer-integration
Copy link

Integration test results from github/github-ui:

Passed  CI   Passed
Passed  VRT   Passed
Passed  Projects   Passed

All checks passed!

@hectahertz hectahertz enabled auto-merge February 27, 2026 12:42
@hectahertz
Copy link
Contributor Author

hectahertz commented Feb 27, 2026

@francinelucca I looked into it and it's safe. During loading the button content gets replaced by a spinner and data-loading is set, so the underline styles don't apply anyway. Replied on the thread with more detail and resolved it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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.

3 participants