Uh oh!
There was an error while loading. Please reload this page.
perf(TreeView): make rows paint-safe and reduce hover/focus invalidation - #7899
Conversation
🦋 Changeset detectedLatest commit: bf5953f 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 |
|
a12402c to
ddb1910Compare…ver/focus invalidation
- Add 'overflow-clip-margin: var(--base-size-8)' to '.TreeViewItem' and
'.TreeViewItemContainer' so the current-item indicator (positioned at
'left: -8px' of the row container) remains visible when a consumer applies
'contain: paint' to the row, or when the documented 'containIntrinsicSize'
prop triggers 'content-visibility: auto' (which implies paint containment)
on the container. The property is a no-op when no paint containment is
active, so default rendering is unchanged. Safari 17 silently ignores the
property and keeps the pre-existing clipped-indicator behavior under
consumer-side 'contain: paint' (no regression from prior versions); Safari
18+ supports it, as does 'content-visibility: auto'.
- Replace ':has(.TreeViewItemSkeleton):hover' with a positive '[data-loading]'
marker emitted on the placeholder row's <li> by 'LoadingItem' via a
module-private 'LoadingPlaceholderContext'. No public-API change; avoids the
broad ':has()' invalidation cost across every row in large trees.
- Replace the root-scope ':hover'/':focus-within' descendant selectors on
level lines with an inherited '--tree-line-color' custom property toggled on
the root <ul>. Same UX; one property toggle on one element instead of
selector re-matching against every '.TreeViewItemLevelLine' descendant.
- Fix unitless 'outline-offset: -2' in the forced-colors focus-ring fallback
that browsers were silently dropping (so forced-colors users now actually
get a focus indicator on tree items).
- Make 'grid-template-columns' declare the 'trailingAction' track explicitly
('auto') so it matches the 5-area 'grid-template-areas' declaration.ddb1910 to
59c4714Compare🤖 Lint issues have been automatically fixed and committed to this PR. |
Removed comments regarding Safari 17 behavior and stylelint rules.
There was a problem hiding this comment.
Pull request overview
Targeted performance and correctness fixes to TreeView styling. Rows are made paint-safe so the current-item indicator survives contain: paint / content-visibility: auto, the skeleton hover-suppression :has() selector is replaced with a positively-marked data-loading attribute, and nesting-line hover toggling is moved from a descendant selector to a single inherited custom property. Also fixes a unitless outline-offset: -2 in the forced-colors focus fallback and makes the trailing-action grid column explicit.
Changes:
- Add
overflow-clip-margin: var(--base-size-8)on.TreeViewItemand.TreeViewItemContainer; fix unitlessoutline-offset; makegrid-template-columnsdeclare thetrailingActiontrack explicitly. - Replace
:has(.TreeViewItemSkeleton):hoverwith a module-privateLoadingPlaceholderContextthat setsdata-loadingon the<li>, and key the CSS off that attribute. - Drive nesting indicator color through an inherited
--tree-line-colorcustom property toggled on the root<ul>instead of via:hover/:focus-withindescendant selectors; add a VRT story forcurrent+containIntrinsicSize.
Show a summary per file
| File | Description |
|---|---|
| packages/react/src/TreeView/TreeView.tsx | Adds LoadingPlaceholderContext, consumes it in Item to emit data-loading, and wraps both LoadingItem return paths with the provider. |
| packages/react/src/TreeView/TreeView.module.css | Adds overflow-clip-margin, explicit auto trailing column, fixes outline-offset: -2px, replaces :has() skeleton selector with [data-loading], refactors level-line color to inherited --tree-line-color. |
| packages/react/src/TreeView/TreeView.features.stories.tsx | Adds CurrentItemWithContainIntrinsicSize story for VRT coverage of the previously-clipped indicator. |
| .changeset/treeview-indicator-paint-safe.md | Patch changeset summarizing the five changes. |
Copilot's findings
- Files reviewed: 4/4 changed files
- Comments generated: 0
| &:where([data-omit-spacer='true']) .TreeViewItemContainer { | ||
| grid-template-columns: 0 0 0 1fr; | ||
| grid-template-columns: 0 0 0 1fr auto; |
There was a problem hiding this comment.
auto is the default, but it reads nicer imp when it's 5 columns defined for 5 columns
👋 Hi from github/github-ui! Your integration PR is ready: https://github.com/github/github-ui/pull/21960 |
Integration test results from github/github-ui PR:
All checks passed! |
Closes #
Five targeted improvements to `TreeView` style + structure aimed at making rows cheaper to render at scale and removing a latent bug in the documented `containIntrinsicSize` API. No visual, layout, or public-API changes at the default rendering — all changes are either invisible at the default state or behind a CSS containment property the consumer opts into.
Changelog
Changed
`. Same UX (lines fade in while the tree is interacted with on hover-capable devices, always visible on coarse pointers); the engine updates one property on one element instead of re-matching the descendant selector against every `.TreeViewItemLevelLine` in the tree.- Fixed unitless `outline-offset: -2` in the forced-colors focus-ring fallback. Browsers were silently dropping the declaration, so forced-colors users were getting no focus indicator on tree items.
- Made `grid-template-columns` declare the `trailingAction` track explicitly (`auto`) so it matches the 5-area `grid-template-areas` declaration on the same element. Clarity, no behavior change.
New
Removed
Nothing public.
Rollout strategy
Testing & Reviewing
VRT expectations: No change in default rendering. The one place a change could surface is forced-colors focus rings on tree items, which go from "nothing" (the silent `outline-offset: -2` drop) to a 2px inset `HighlightText` outline. That's an accessibility fix.
Internal-only note: this lets the github-ui escape hatch in `packages/diff-file-tree/DiffFileTree.module.css` (`content-visibility: visible; contain: none;` for `[aria-current='true']` and `:focus-visible`) be removed once this lands.
Merge checklist