-
Notifications
You must be signed in to change notification settings - Fork 647
Improve PageLayout pane drag performance with pointer capture and GPU-accelerated transforms #7307
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
Improve PageLayout pane drag performance with pointer capture and GPU-accelerated transforms #7307
Conversation
…nce with…" This reverts commit 335e9e8.
🦋 Changeset detectedLatest commit: b2e840f 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 |
| /* Exported values for JavaScript consumption */ | ||
| :export { | ||
| /* Breakpoint where --pane-max-width-diff changes (used in usePaneWidth.ts) */ | ||
| paneMaxWidthDiffBreakpoint: 1280; | ||
| /* Default value for --pane-max-width-diff below the breakpoint */ | ||
| paneMaxWidthDiffDefault: 511; | ||
| } |
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.
not sure if we have prefernces for using these across JS/CSS or just duplicating them?
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.
TIL about :export
Sorry, trying to understand the goal here. Why are these exported from PageLayout.module.css? We don't use them in this file
Nevermind, I realise you can't use the exports within the file because they will be removed during compilation
| * Handles initialization from storage, clamping on viewport resize, and provides | ||
| * functions to save and reset width. | ||
| */ | ||
| export function usePaneWidth({ |
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.
extracted from PageLayout because that file is huge and getting harder to navigate
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.
Pull request overview
This pull request re-applies performance improvements to the PageLayout component's resizable pane functionality, fixing CSS selector performance issues that caused the previous implementation to be reverted. The core optimization replaces expensive :has() selectors with simple descendant selectors by moving the data-dragging attribute from the drag handle to the PageLayoutContent parent element.
Key Changes:
- Introduces a new
usePaneWidthcustom hook to manage pane width state, localStorage persistence, and viewport-responsive constraints - Refactors drag handling to use modern pointer events with pointer capture for better cross-device compatibility
- Applies aggressive CSS containment and GPU acceleration during drag operations to minimize layout thrashing
- Synchronizes CSS breakpoint values with JavaScript using CSS
:exportto ensure consistency
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
packages/react/src/PageLayout/usePaneWidth.ts |
New custom hook encapsulating pane width state management, localStorage persistence, viewport constraint calculations, and ARIA attribute updates with optimized resize handling |
packages/react/src/PageLayout/usePaneWidth.test.ts |
Comprehensive unit tests for the new hook covering initialization, localStorage persistence, constraint calculations, and resize listener behavior |
packages/react/src/PageLayout/PageLayout.tsx |
Refactored to use the new hook, replaced mouse events with pointer events, extracted DragHandle component, and moved data-dragging attribute management to parent for performance |
packages/react/src/PageLayout/PageLayout.module.css |
Adds CSS :export for JavaScript consumption, replaces :has() selectors with descendant selectors, and adds performance optimizations (containment, GPU acceleration) during drag |
packages/react/src/PageLayout/PageLayout.test.tsx |
New tests verifying data-dragging attribute is correctly set/removed during both pointer and keyboard resize operations |
packages/react/src/PageLayout/PageLayout.performance.stories.tsx |
New Storybook stories for manual performance testing with varying DOM complexity (light/medium/heavy content) and ARIA attribute monitoring |
packages/react/src/PageLayout/__snapshots__/PageLayout.test.tsx.snap |
Updated snapshots reflecting removal of inline --pane-width style for non-resizable panes |
e2e/components/Axe.test.ts |
Skips performance test stories from accessibility tests to avoid timeouts on large DOM structures |
.changeset/shiny-buckets-study.md |
Documents the patch fix for ResizeObserver performance regression |
.changeset/healthy-poets-act.md |
Removes changeset for the reverted PR |
Co-authored-by: Copilot <[email protected]>
| // suppressHydrationWarning: We intentionally read from localStorage during | ||
| // useState init to avoid resize flicker, which causes a hydration mismatch | ||
| // for --pane-width. This only affects this element, not children. | ||
| suppressHydrationWarning |
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.
added this in a later PR where I removed a layouteffect that reads localStorage.
we should probably provide alternatives to this - but for now, just keeping it and suppressing the warning since we're accepting this by having width stored in local state and not having a good mechanism for setting it on the server
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.
this commit flips between the options here - I'm open to either - or keeping as is for now (inline with today's behavior) and moving that to a followup
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.
I filed an issue for this also #7311 since today's implementation is not safe
Lukeghenco
left a comment
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.
Approval from PR eng who uses it.
| // CSS variable, not DOM structure or children. | ||
| const [currentWidth, setCurrentWidth] = React.useState(() => { | ||
| const defaultWidth = getDefaultPaneWidth(width) | ||
|
|
||
| if (!resizable || !canUseDOM) { | ||
| return defaultWidth | ||
| } | ||
|
|
||
| try { | ||
| const storedWidth = localStorage.getItem(widthStorageKey) | ||
| if (storedWidth !== null) { | ||
| const parsed = Number(storedWidth) | ||
| if (!isNaN(parsed) && parsed > 0) { | ||
| return parsed | ||
| } | ||
| } | ||
| } catch { | ||
| // localStorage unavailable - keep default | ||
| } | ||
|
|
||
| return defaultWidth | ||
| }) |
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.
Keeping this here for now, but I think we should move to a layout effect or some other pattern.
do we have a way to preseed this into SSR to avoid hydration mismatches - or do we always want to accept a CLS flash or hydration error/supression requirement
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.
related issue (for today's impl too) #7311
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.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
…251-mc/copilot/sub-pr-7248
|
👋 Hi from github/github-ui! Your integration PR is ready: https://github.com/github/github-ui/pull/8861 |
Reverts #7305
Fixes a css issue with arbitrarily deep css
class:has()selectors by movingdata-draggingto the page layout - inrelated to https://github.com/github/pull-requests/issues/21237
I merged separate updates into this:
:hasselectors from pagelayout #7302This pull request reapplies and improves the PageLayout resizable enhancements in
@primer/react, focusing on performance optimizations during pane resizing while avoiding previous issues with INP (Interaction to Next Paint) caused by expensive CSS selectors. The changes introduce new CSS containment strategies, export key CSS variables for use in JavaScript, and add new tests to ensure correct drag state handling. It also updates test snapshots and skips new performance tests in the E2E suite.PageLayout Resizable Enhancements and Performance Optimizations:
contain: layout style paint) to.ContentWrapper,.Content, and.Paneelements during drag operations, reducing layout and paint costs and improving performance for large content areas. [1] [2] [3]data-draggingattribute onPageLayoutContentto efficiently toggle drag state, replacing reliance on global selectors and making it easier to apply performance optimizations only when needed.DraggableHandleto prevent touch scrolling and text selection during drag, and ensured the correct cursor is shown. [1] [2]Code and Documentation Updates:
Meta/Process:
Changelog
No api changes
New
n/a
Changed
Improved drag to resize performance for PageLayout resizable
Removed
n/a
Rollout strategy
Testing & Reviewing
Merge checklist