-
Notifications
You must be signed in to change notification settings - Fork 648
refactor(PageLayout): drag/resize performance with inline styles and O(1) CSS selectors #7349
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
🦋 Changeset detectedLatest commit: 545af79 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 |
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
|
@mattcosta7 I've opened a new pull request, #7389, to work on those changes. Once the pull request is ready, I'll request review from you. |
|
@mattcosta7 I've opened a new pull request, #7390, to work on those changes. Once the pull request is ready, I'll request review from you. |
…7389) Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: mattcosta7 <[email protected]>
…7390) Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: mattcosta7 <[email protected]>
francinelucca
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.
lgtm
- Create contentWrapperRef pointing to actual .ContentWrapper element - Set data-dragging on .Pane and .ContentWrapper directly (O(1) selectors) - Remove ref from .PageLayoutContent wrapper (not needed) - Update tests to query for ContentWrapper element
|
👋 Hi from github/github-ui! Your integration PR is ready: https://github.com/github/github-ui/pull/9583 |
This PR further optimizes PageLayout's drag/resize interactions for large DOMs by:
The Problem
The previous implementation used CSS descendant selectors to apply containment during drag:
When the browser performs style recalculation, descendant selectors require traversing all descendants to check if they match. On large DOMs (50k+ nodes), this becomes a significant bottleneck—every style recalc during drag required O(n) traversal.
The Solution
This PR uses direct attribute selectors on the elements themselves:
Direct attribute selectors are O(1)—only the attributed element is invalidated during style recalculation, regardless of DOM size.
Performance Impact by DOM Size
For typical applications with small DOMs, there's no user-visible difference. The improvements are most significant on pages with large, complex content areas.
Why Each Optimization Helps
background-colorand CSS variables applied directlypointer-events: nonecontent-visibility: autocontain: layout style paintdata-draggingattribute applied during viewport resizeHorizontalDivider,VerticalDivider, andDragHandleskip unnecessary rendersWhat Changed
Changed
.PageLayoutContent[data-dragging='true'] .Pane) with direct attribute selectors (.Pane[data-dragging='true'],.ContentWrapper[data-dragging='true'])--draggable-handle--drag-opacitywith:: beforepseudo-element for hover state, plus inlinebackground-colorduring drag for instant feedback. Transition disabled during drag via--draggable-handle--transition: nonedata-draggingattribute during resize for containment parity with drag. Containment removed after 150ms debounce when resize stops.isDraggingRef(ref) instead of reading from DOM attribute for cheaper state checksAdded
paneUtils.ts— ExtractedsetDraggingStyles()andremoveDraggingStyles()utilities for applying/removing drag optimizations via inline stylesrequestAnimationFrame, dropping intermediate eventsReact.memo— Added toHorizontalDivider,VerticalDivider, andDragHandlecomponents to prevent unnecessary re-rendersdata-draggingattribute during resize for consistent optimization; cleaned up on unmountdata-draggingattribute is applied/removed correctly, cleanup on unmount, and thatwill-changeis not usedrequestAnimationFrameto prevent stale callbacksRemoved
.PageLayoutContent[data-dragging='true'] .Pane,.PageLayoutContent[data-dragging='true'] .Content,.PageLayoutContent[data-dragging='true'] .ContentWrapperwill-changeproperties — Removedwill-change: widthandwill-change: width, transformas they were counterproductivetransform: translateZ(0)andbackface-visibility: hidden— Removed compositor hints that weren't providing benefitTechnical Details
Drag flow:
onPointerDown→ callssetDraggingStyles()frompaneUtils.tswhich:background-coloron handle for instant visual feedback--draggable-handle--drag-opacity: 1and--draggable-handle--transition: noneon handledata-dragging="true"on Pane and ContentWrapper elements.Pane[data-dragging='true']and.ContentWrapper[data-dragging='true']apply containmentonPointerMove→ coalesced via rAF, only latest position processed per frameonLostPointerCapture→ callsremoveDraggingStyles()to restore normal state, cancels pending rAFWhy direct attribute selectors are O(1):
When the browser invalidates styles for an element with a changed attribute, it only needs to re-match selectors for that specific element. With
.Pane[data-dragging='true'], the browser checks if the element has class.Paneand attributedata-dragging='true'—a constant-time operation. With descendant selectors like.PageLayoutContent[data-dragging='true'] .Pane, the browser must traverse ancestors to check if any parent matches.Window resize flow:
resizeevent →startResizeOptimizations()setsdata-dragging="true"on Pane and ContentWrapperendResizeOptimizations()removesdata-draggingattributeRollout Strategy
No API changes. Internal performance optimization only.
Testing & Reviewing
Tested with synthetic large DOMs in PageLayout performance stories. Verified with Chrome DevTools Performance profiler.
To test manually:
mainbranchMerge Checklist