Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/perf-remove-expensive-has-selectors.md
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
---
'@primer/react': patch
---

Improve rendering performance in Safari by removing expensive `:has()` selectors from ActionList/NavList, Breadcrumbs, SelectPanel, and SegmentedControl. No visual or API changes.
21 changes: 11 additions & 10 deletions packages/react/src/ActionList/ActionList.module.css
Original file line numberDiff line numberDiff line change
Expand Up@@ -557,9 +557,17 @@
display: none;
}

/* show active indicator on parent collapse if child is active */
/* stylelint-disable-next-line selector-pseudo-class-disallowed-list -- scoped to CSS Module, audited (github/github-ui#17224) */
&:has(~ .SubGroup [data-active='true']) {
/*
* Show the active indicator on a collapsed parent when one of its subnav
* items is active. NavList sets `active` (→ the parent <li>'s `data-active`)
* to exactly `!isOpen && containsCurrentItem`, which is the same condition as
* `:has(~ .SubGroup [data-active='true'])`. Keying off the ancestor's
* `data-active` avoids Safari's `:has()` invalidation cost, which is
* especially high here because the old anchor (`[data-active]`) toggles on
* every navigation and combined a general-sibling with a descendant match.
* The ancestor prefix keeps the original selector specificity (0,4,0).
*/
.ActionListItem[data-active='true'] & {
background: var(--control-transparent-bgColor-selected);

& .ItemLabel {
Expand DownExpand Up@@ -655,13 +663,6 @@ default block */
word-break: normal;
}

/* stylelint-disable-next-line selector-pseudo-class-disallowed-list -- scoped to CSS Module, audited (github/github-ui#17224) */
&:has([data-truncate='true']) {
& .ItemLabel {
flex: 1 0 auto;
}
}

& .Description {
/* stylelint-disable-next-line primer/typography */
line-height: 16px;
Expand Down
5 changes: 3 additions & 2 deletions packages/react/src/Breadcrumbs/Breadcrumbs.module.css
Original file line numberDiff line numberDiff line change
Expand Up@@ -117,8 +117,9 @@
list-style: none;

/* allow menu items to wrap line */
/* stylelint-disable-next-line selector-pseudo-class-disallowed-list -- scoped to CSS Module, audited (github/github-ui#17224) */
&:has(.MenuOverlay) {
/* The overflow-menu item is marked in React (`data-overflow-menu`) instead of using
`:has(.MenuOverlay)`, avoiding Safari's `:has()` invalidation cost. */
&[data-overflow-menu] {
white-space: normal;
}

Expand Down
2 changes: 1 addition & 1 deletion packages/react/src/Breadcrumbs/Breadcrumbs.tsx
Original file line numberDiff line numberDiff line change
Expand Up@@ -313,7 +313,7 @@ function Breadcrumbs({className, children, style, overflow = 'wrap', variant = '
// In 'menu-with-root' mode the root stays visible, so drop it from the menu.
const effectiveMenuItems = effectiveHideRoot ? menuItems : menuItems.slice(1)
const menuElement = (
<li className={classes.BreadcrumbsItem} key="breadcrumbs-menu">
<li className={classes.BreadcrumbsItem} data-overflow-menu="" key="breadcrumbs-menu">
<BreadcrumbsMenuItem
ref={measureMenuButton}
items={effectiveMenuItems}
Expand Down
36 changes: 36 additions & 0 deletions packages/react/src/NavList/NavList.dev.stories.tsx
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
import type {Meta} from '@storybook/react-vite'
import {within, userEvent} from 'storybook/test'
import {PageLayout} from '../PageLayout'
import {NavList} from './NavList'
import {ArrowRightIcon, ArrowLeftIcon, BookIcon, FileDirectoryIcon} from '@primer/octicons-react'
Expand DownExpand Up@@ -69,3 +70,38 @@ export const WithGroupTitleAndHeading = () => (
<PageLayout.Content></PageLayout.Content>
</PageLayout>
)

/**
* A collapsed parent item whose sub-nav contains the current item shows an
* active indicator. NavList auto-expands sub-navs that contain the current
* item, so the play function collapses it to surface the collapsed-parent
* active styling for visual regression coverage. This exercises the CSS that
* replaced `:has(~ .SubGroup [data-active])` with the parent's `data-active`.
*/
export const CollapsedSubNavWithCurrentItem = () => (
<PageLayout>
<PageLayout.Pane position="start">
<NavList>
<NavList.Item href="#">Item 1</NavList.Item>
<NavList.Item>
Item with current sub-item
<NavList.SubNav>
<NavList.Item href="#" aria-current="page">
Current sub-item
</NavList.Item>
<NavList.Item href="#">Other sub-item</NavList.Item>
</NavList.SubNav>
</NavList.Item>
<NavList.Item href="#">Item 3</NavList.Item>
</NavList>
</PageLayout.Pane>
<PageLayout.Content></PageLayout.Content>
</PageLayout>
)

CollapsedSubNavWithCurrentItem.storyName = 'Collapsed SubNav With Current Item'
CollapsedSubNavWithCurrentItem.play = async ({canvasElement}: {canvasElement: HTMLElement}) => {
const canvas = within(canvasElement)
const parentButton = await canvas.findByRole('button', {name: /Item with current sub-item/i})
await userEvent.click(parentButton)
}
Original file line numberDiff line numberDiff line change
Expand Up@@ -188,11 +188,6 @@
}
}

/* stylelint-disable-next-line selector-pseudo-class-disallowed-list -- scoped to CSS Module, audited (github/github-ui#17224) */
&:focus-within:has(:focus-visible) {
background-color: transparent;
}

&:first-child {
/* stylelint-disable-next-line primer/spacing */
margin-left: -1px;
Expand Down
Original file line numberDiff line numberDiff line change
Expand Up@@ -121,8 +121,11 @@
.TextInput {
padding-left: var(--base-size-8) !important;

/* stylelint-disable-next-line selector-class-pattern, selector-no-qualifying-type, selector-pseudo-class-disallowed-list -- :has() scoped to CSS Module, audited (github/github-ui#17224) */
&:has(input:placeholder-shown) :global(.TextInput-action) {
/* Hide the trailing action while the input is empty (showing its placeholder). Uses a
forward sibling selector on the input instead of `:has()` to avoid Safari's `:has()`
invalidation cost; `input` and `.TextInput-action` are siblings inside the wrapper. */
/* stylelint-disable-next-line selector-class-pattern */
& input:placeholder-shown ~ :global(.TextInput-action) {
display: none;
}
}
Expand Down
Loading