-
Notifications
You must be signed in to change notification settings - Fork 646
Revert "Revert "Improve PageLayout pane drag performance with pointer capture and GPU-accelerated transforms"" #7275
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
Revert "Revert "Improve PageLayout pane drag performance with pointer capture and GPU-accelerated transforms"" #7275
Conversation
… capture…" This reverts commit 0cd9ff1.
🦋 Changeset detectedLatest commit: 41cb836 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 |
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 PR re-applies a performance optimization for PageLayout pane dragging that was previously reverted due to layout issues in memex. The fix improves drag performance to 60fps using pointer capture and GPU-accelerated CSS transforms, while addressing the previous regression by only applying containment during active resize operations rather than globally.
Key Changes:
- Replaces mouse events with pointer events and pointer capture for smoother drag interactions
- Eliminates React state updates during drag by using refs and direct DOM manipulation (
style.setProperty) - Implements shared ResizeObserver via
useSyncExternalStorefor efficient viewport width tracking - Applies CSS containment and GPU acceleration only during active drag operations using
:has()selector
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
packages/react/src/PageLayout/PageLayout.tsx |
Core implementation: adds viewport width tracking, replaces mouse/keyboard drag with pointer events, uses DOM manipulation instead of React state for performance |
packages/react/src/PageLayout/PageLayout.module.css |
Removes global body drag styles, adds targeted CSS optimizations (containment, GPU acceleration) only during active drag using :has() selector |
packages/react/src/PageLayout/__snapshots__/PageLayout.test.tsx.snap |
Updates snapshots to reflect removal of inline --pane-width style (now set via DOM manipulation) |
packages/react/src/PageLayout/PageLayout.performance.stories.tsx |
Adds comprehensive performance test stories with varying DOM sizes (100-5000 elements) and keyboard/ARIA testing |
e2e/components/Axe.test.ts |
Excludes performance test stories from accessibility tests (appropriate since they contain large datasets) |
.changeset/olive-heads-enter.md |
Adds changeset for patch release documenting the performance improvement |
|
👋 Hi from github/github-ui! Your integration PR is ready: https://github.com/github/github-ui/pull/8187 |
|
🟢 ci completed with status |
llastflowers
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.
✨
Reverts #7274 and reapplies #7251 with minor changes
Removes a few CSS optimizations that caused conflicts with some curent impelemtnations in projects.
we removed a couple lines of css optimization that we had added initially. Otherwise this PR is functionally the same as before - just with a few less browser rendering optimizations
.ContentWrapper { /** * OPTIMIZATION: Isolate content area from rest of page * Note: No 'paint' containment to allow overflow effects (tooltips, modals, etc.) */ - contain: layout style; } .Pane { /** * OPTIMIZATION: Full containment for pane - isolates from rest of page */ - contain: layout style paint; - content-visibility: auto; } .Content { /** * OPTIMIZATION: Skip rendering off-screen content during scrolling/resizing * This automatically helps consumers with large content by only rendering * elements that are visible in the viewport */ - content-visibility: auto; - contain-intrinsic-size: auto 500px; }This means we lose a few browser/style optimization, but fixes an issue where some fixed position layout could be mis-attributed AFAICT this only occured in memex, in an overlay when dragging a card in an board view.
I opted to remove that optimization and instead only apply containment in cases where the page layout is actively resizing - which avoids the issue we saw here.
This is not ideal but avoids possible issues that could arise from fixed positioning internals inside of the containment space. We might revisit this later if we find it valuable to improve performance again, with a bit more nuance in the validation phase (and re-evaluating some fixed position portalling or other strategies)