-
Notifications
You must be signed in to change notification settings - Fork 647
pass at optimizing css and js to minimize blocking, recalc, layout, inp costs #7312
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Replace expensive CSS :has() selectors that scan the DOM with data
attributes set in JavaScript. This improves Interaction to Next Paint
(INP) by avoiding costly style recalculations.
Changes:
- Dialog: Replace body:has(.Dialog.DisableScroll) with direct class
toggle on body element. The :has() selector was scanning the entire
DOM on every style recalc.
- ActionList: Replace expensive descendant-scanning :has() selectors:
- Mixed descriptions: Replace double :has() that scanned all items
twice with a JS-computed data-has-mixed-descriptions attribute
- Disabled state: Replace :has([aria-disabled], [disabled]) with
data-disabled attribute on the <li>
- Loading state: Replace :has([data-loading]) with data-loading
attribute on the <li>
Selectors left unchanged (cheap/already scoped):
- &:has(> .TrailingAction) - direct child, O(1)
- .Dialog:has(.Footer) - single element container
- Adjacent sibling selectors in other components
Scope remaining :has() selectors to direct children where possible, reducing DOM traversal depth from O(n) to O(1) or O(children). Button: - Replace :has(.Visual) with data-no-visuals attribute check - Scope icon-only counter :has() to direct child path ActionList: - Scope TrailingAction loading :has() to direct child - Replace :has([data-truncate]) with self-selector (attr is on same element) - Scope TrailingActionButton :has() to direct child - Scope InactiveButtonWrap :has() to shallow path - Add data-loading to TrailingAction wrapper span
All 17 :has() selectors in PageHeader now use the direct child combinator (>) since TitleArea and Navigation are rendered as direct children of PageHeader. This changes the lookup from O(n) descendant scan to O(1) direct child check.
Scope :has() selectors to use direct child combinators (>) or shallow paths (> * >) for O(1) lookups instead of descendant scans: - Dialog: :has(.Footer) → :has(> .Footer) - SegmentedControl: :has(:focus-visible) → :has(> :focus-visible) - TreeView: :has(.TreeViewItemSkeleton) → :has(> .TreeViewItemSkeleton) - Breadcrumbs: :has(.MenuOverlay) → :has(> .MenuDetails .MenuOverlay) - ButtonGroup: :has(div:last-child:empty) → :has(> div:last-child:empty) - Banner: :has(.BannerActions) → :has(> * > .BannerActions) - AvatarStack: :has([data-square]) → :has(> [data-square]) - SelectPanel: :has(input:placeholder-shown) → :has(> input:placeholder-shown)
🦋 Changeset detectedLatest commit: 48ee449 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 |
|
👋 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 |
| * :has([data-has-description='true']):has([data-has-description='false']) | ||
| * The double :has() scans all items twice on every style recalc - O(2n). | ||
| */ | ||
| &[data-has-mixed-descriptions='true'] { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
copilot and I couldn't find a solution outside of doing some javascript for this - but maybe O(2n) scans are ok here?
3211079 to
895c4a6
Compare
895c4a6 to
0a7505c
Compare
0a7505c to
7287a23
Compare
7287a23 to
78d425b
Compare
The hover state selector was checking [aria-disabled] on the <li> element, but aria-disabled may be on a child element (ItemWrapper) depending on the _PrivateItemWrapper and listSemantics flags. Using data-disabled alone is correct because it's explicitly set on the <li> based on the disabled prop.
…elector - Replace body:has(.Dialog.DisableScroll) with direct body.DialogScrollDisabled class - Scope :has(.Footer) to direct child with > combinator for O(1) lookup Part of #7312
All TitleArea and Navigation selectors are now scoped to direct children with > combinator. Part of #7312
…r O(1) lookup Part of #7312
Added documentation explaining why the :has(input:placeholder-shown) selector is acceptable. Part of #7312
- Add useTreeItemCache hook to cache DOM queries for tree items - Update useRovingTabIndex and useTypeahead to use cached items - Add documentation for acceptable :has() selector usage Part of #7312
…useOverflow - useResizeObserver fires callback immediately on first observation, then throttles with rAF - useOverflow uses the same pattern to avoid initial flash of incorrect overflow state - Added isFirstCallback ref pattern to skip throttling on initial mount Part of #7312
…s and throttle updates - Use window resize listener instead of ResizeObserver on documentElement - Add ResizeObserver for floating element with first-immediate throttling - Use updatePositionRef to avoid callback identity changes - Deduplicate observer setup to avoid redundant work Part of #7312
Split AutocompleteContext into separate contexts for static values, setters, and dynamic state. Components now subscribe only to the context slices they need, reducing re-renders. Part of #7312
Combine display and visibility checks into a single getComputedStyle call. Part of #7312
…server throttling - UnderlineNavItem: Batch getBoundingClientRect and getComputedStyle reads - UnderlineNav: Add comment noting ResizeObserver callbacks are now throttled Part of #7312
Remove unnecessary array dependency parameter from useResizeObserver call. Part of #7312
Document that useResizeObserver is throttled by default with rAF for better INP. Part of #7312
…attribute checks - Use combined querySelectorAll selector instead of recursive traversal - Check attribute-based states (disabled, hidden, inert) before getComputedStyle - Only call getComputedStyle when CSS-based visibility check is needed Part of #7312
related to https://github.com/github/react-platform/issues/1398
CSS
:has()Selector & JavaScript Performance OptimizationsSummary
This PR optimizes CSS
:has()selectors and JavaScript patterns across multiple components to improve Interaction to Next Paint (INP) and overall Web Vitals performance. The changes target expensive style recalculations and DOM queries that block the main thread during user interactions.Changes Overview
CSS Optimizations (11 files)
:has()selector for scroll lock>)data-disabled,data-loading) instead of:has()for disabled/loading states:has()selectors to direct children:has([data-color-mode])selectors (already handled by global selectors)JavaScript Hook Optimizations
useResizeObserver- First-Immediate Throttlingobserve()This pattern preserves test compatibility (tests expect immediate first callback) while reducing layout thrashing during rapid resize events like window dragging.
useAnchoredPosition- Indirect BenefituseAnchoredPositioncallsuseResizeObservertwice internally:Both now benefit from first-immediate throttling automatically. During rapid resize events (e.g., window dragging with an open dropdown), position recalculations are coalesced to ~60fps instead of firing on every resize event.
useOverflow- Same PatternFirst callback immediate, subsequent callbacks throttled with requestAnimationFrame.
Component-Local Optimizations
useTreeItemCache(New Hook)Caches
querySelectorAllresults for TreeView typeahead with MutationObserver invalidation.Other Optimizations
querySelectorAllwith combined selector + attribute-first checksoffsetWidth) in single passgetBoundingClientRectandgetComputedStylereadsThe Problem
How
:has()affects INPWhen a browser evaluates a CSS selector like:
It must scan all descendants of
.parenton every style recalculation to determine if any match.descendant. This is an O(n) operation where n is the number of descendants.Complexity Analysis
:has(.descendant):has(> .child)body:has(.element)[data-attr]Performance Expectations
Estimated Improvements
Web Vitals Analysis
:has()scans eliminated, JS hooks throttledTesting
Files Changed
CSS Files (11+ files)
ActionList.module.css,Group.module.css,AvatarStack.module.css,Banner.module.cssBaseStyles.module.css,Breadcrumbs.module.css,ButtonBase.module.css,ButtonGroup.module.cssDialog.module.css,PageHeader.module.css,SegmentedControl.module.cssTimeline.module.css,TreeView.module.css,SelectPanel.module.cssJavaScript/TypeScript Files
hooks/useResizeObserver.ts- First-immediate throttlinghooks/useOverflow.ts- First-immediate throttlinghooks/useAnchoredPosition.ts- Benefits from useResizeObserver changes (no code changes needed)TreeView/useTreeItemCache.ts- New cache hookTreeView/useTypeahead.ts- Uses cached tree itemsAvatarStack/AvatarStack.tsx- MutationObserver throttlingActionBar/ActionBar.tsx,UnderlineNav/UnderlineNav.tsx- Comments for throttlingDialog/Dialog.tsx- Body class for scroll lockActionList/Item.tsx,ActionList/TrailingAction.tsx- Data attributesinternal/utils/hasInteractiveNodes.ts- Optimized algorithmBreadcrumbs/Breadcrumbs.tsx,UnderlineNavItem.tsx- Batch layout readsBreaking Changes
None. All changes are backward compatible:
Changelog
New
useTreeItemCachehook for caching TreeView DOM queriesdata-disabledanddata-loadingattributes on ActionList itemsChanged
:has()selectors optimized across 11+ component CSS files:has()useResizeObserveruses first-immediate, subsequent-throttled patternuseOverflowuses first-immediate, subsequent-throttled patternuseAnchoredPositionbenefits from useResizeObserver throttling (used by ActionMenu, SelectPanel, Autocomplete, etc.)hasInteractiveNodesuses querySelectorAll with attribute-first checksRollout strategy
Merge checklist