Skip to content

perf(SelectPanel v2): replace :has(input:placeholder-shown) with data-empty attribute#7904

Closed
mattcosta7 wants to merge 6 commits into
mainfrom
perf/selectpanel2-empty-input
Closed

perf(SelectPanel v2): replace :has(input:placeholder-shown) with data-empty attribute#7904
mattcosta7 wants to merge 6 commits into
mainfrom
perf/selectpanel2-empty-input

Conversation

@mattcosta7

Copy link
Copy Markdown
Contributor

Closes #

Replaces the &:has(input:placeholder-shown) :global(.TextInput-action) selector that hid the clear-action button when the search input is empty. The previous selector forced a descendant :has() evaluation on every keystroke (placeholder-shown toggles with each character typed/deleted), and the inner :global(.TextInput-action) selector required descendant matching across the whole TextInput subtree.

Approach: track the empty state in React state on the SelectPanelSearchInput component, derive an isEmpty value, and pass it through as data-empty on the rendered TextInput wrapper. The CSS now matches a plain attribute on the same .TextInput element.

The controlled/uncontrolled cases are both handled:

const [uncontrolledEmpty, setUncontrolledEmpty] = React.useState(
  () => !(props.value ?? props.defaultValue),
)
const isEmpty = props.value !== undefined ? props.value === '' : uncontrolledEmpty
  • For controlled inputs (value prop provided), isEmpty is derived from the prop directly so it stays in sync without an effect.
  • For uncontrolled inputs, the change handler and the clear-action click handler update the uncontrolledEmpty state.

Changelog

New

  • data-empty attribute on SelectPanel.SearchInput when the search input is empty.

Changed

  • Internal: SelectPanel.module.css clear-action visibility no longer uses :has(input:placeholder-shown).

Removed

  • The &:has(input:placeholder-shown) selector from SelectPanel.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 16 packages/react/src/experimental/SelectPanel2/** unit tests pass.
  • tsc --noEmit on packages/react is clean.
  • Stylelint + Prettier + ESLint clean on touched files (only pre-existing content-visibility browser-compat warning at line 87, unrelated).

VRT expectations: None. The clear-action button is shown/hidden under identical conditions as before — when the input is empty, it's hidden; otherwise, it's shown. The only behavioral difference is timing: the previous :has(input:placeholder-shown) updated on the browser's :placeholder-shown flip (which happens on the same frame as the input event), while the new data-empty flips on the React commit that follows the change event. In practice both update in the same animation frame and the user can't observe the difference.

Part of the :has()-reduction series: #7901 (PageHeader), #7902 (ActionList SubGroup), #7903 (SegmentedControl divider).

Merge checklist

@changeset-bot

changeset-bot Bot commented May 29, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 7906c23

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.

TextInput spreads unknown props onto the inner <input> rather than the
.TextInput wrapper, so the data attribute landed on the input element.
Use a general-sibling combinator from the input to the clear-action span
(which sits later in the wrapper).

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 removes an expensive :has(input:placeholder-shown)-based selector from the experimental SelectPanel2 search input styling by hoisting “empty” state into React and exposing it via a data-empty attribute, letting CSS use a simple sibling/attribute selector to hide the clear-action button.

Changes:

  • Track empty state in SelectPanelSearchInput and set data-empty based on controlled vs uncontrolled usage.
  • Replace the :has() CSS rule with an attribute + general-sibling selector targeting .TextInput-action.
  • Add a patch changeset documenting the internal perf-driven change.
Show a summary per file
File Description
packages/react/src/experimental/SelectPanel2/SelectPanel.tsx Adds empty-state tracking and emits data-empty on the underlying TextInput input element.
packages/react/src/experimental/SelectPanel2/SelectPanel.module.css Replaces :has(input:placeholder-shown) with input[data-empty] ~ .TextInput-action to hide the clear action without :has().
.changeset/perf-selectpanel2-empty-input.md Documents the change as a patch release item.

Copilot's findings

  • Files reviewed: 3/3 changed files
  • Comments generated: 2

Comment thread packages/react/src/experimental/SelectPanel2/SelectPanel.tsx Outdated
Comment thread packages/react/src/experimental/SelectPanel2/SelectPanel.tsx
@github-actions
github-actions Bot temporarily deployed to storybook-preview-7904 July 7, 2026 15:22 Inactive
@mattcosta7
mattcosta7 marked this pull request as ready for review 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 jonrohan July 7, 2026 16:18
@mattcosta7 mattcosta7 self-assigned this Jul 7, 2026
@mattcosta7
mattcosta7 requested a review from Copilot July 7, 2026 16:34
@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

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.

Review details

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

Comment on lines 443 to 449
const internalOnChange = (event: React.ChangeEvent<HTMLInputElement>) => {
// If props.onChange is given, the application controls search,
// otherwise the component does
if (typeof propsOnChange === 'function') propsOnChange(event)
else setSearchQuery(event.target.value)
setUncontrolledEmpty(event.target.value === '')
}
Comment on lines 472 to 479
onClick={() => {
if (inputRef.current) inputRef.current.value = ''
if (typeof propsOnChange === 'function') {
// @ts-ignore TODO this is a hacky solution to clear
propsOnChange({target: inputRef.current, currentTarget: inputRef.current})
}
setUncontrolledEmpty(true)
}}
Comment on lines +435 to +442
// Track whether the input is empty so the clear-action can be hidden via a
// data attribute instead of a `:has(input:placeholder-shown)` selector that
// forced a descendant re-evaluation on every keystroke.
const [uncontrolledEmpty, setUncontrolledEmpty] = React.useState(
() => (props.value ?? props.defaultValue ?? '') === '',
)
const isEmpty = props.value !== undefined ? props.value === '' : uncontrolledEmpty

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 replaced the :has(input:placeholder-shown) selector on .TextInput with a pure-CSS forward-sibling selector (& input:placeholder-shown ~ .TextInput-action). The perf goal here is already achieved on main without extra React state, so this PR's data-empty approach is redundant.

@mattcosta7 mattcosta7 closed this Jul 7, 2026
@primer
primer Bot deleted the perf/selectpanel2-empty-input 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