Skip to content

Conversation

Copy link
Contributor

Copilot AI commented Dec 29, 2025

PR #7349 introduced inline style manipulation to avoid O(n) descendant selector matching during drag/resize. However, the issue was descendant selectors specifically—direct attribute selectors like .Pane[data-dragging='true'] are O(1) and only invalidate the attributed element.

This refactor moves containment properties from JavaScript to CSS, reducing code complexity while maintaining performance.

Changes

Before:

// Inline styles applied via JS
export function setContainmentOptimizations(element: HTMLElement | null) {
  if (!element) return
  element.style.contain = 'layout style paint'
  element.style.contentVisibility = 'auto'
  element.style.containIntrinsicSize = `auto ${element.offsetHeight}px`
  element.style.pointerEvents = 'none'
}

After:

/* Direct attribute selector - O(1) invalidation */
.Pane[data-dragging='true'],
.ContentWrapper[data-dragging='true'] {
  contain: layout style paint;
  pointer-events: none;
  content-visibility: auto;
}
// Simple attribute toggle
pane?.setAttribute('data-dragging', 'true')
content?.setAttribute('data-dragging', 'true')

Changelog

Changed

  • PageLayout: Containment optimizations during drag/resize now use CSS attribute selectors instead of inline styles

Removed

  • paneUtils.setContainmentOptimizations() function (internal)
  • paneUtils.removeContainmentOptimizations() function (internal)
  • Dynamic contain-intrinsic-size calculation (acceptable—optimization active <1s during drag)

Rollout strategy

  • Patch release
  • Minor release
  • Major release; if selected, include a written rollout or migration plan
  • None; if selected, include a brief description as to why

Internal optimization change with no API impact. Same performance characteristics, simpler implementation.

Testing & Reviewing

All existing PageLayout drag/resize tests pass. Tests updated to check data-dragging attribute presence instead of inline styles.

Merge checklist

Original prompt

Summary

Simplify the drag/resize performance optimizations in PR #7349 by using direct CSS attribute selectors instead of inline styles for containment properties. This maintains O(1) performance while reducing code complexity.

Background

PR #7349 introduced inline style manipulation via paneUtils.ts to avoid O(n) descendant selector matching. However, the original problem was descendant selectors, not attribute selectors themselves. A direct attribute selector like .Pane[data-dragging='true'] is O(1) because it only invalidates the element with the attribute, not all descendants.

Changes Required

1. Add CSS rules with direct attribute selectors

In packages/react/src/PageLayout/PageLayout.module.css, add:

/**
 * OPTIMIZATION: CSS containment during drag/resize
 * Direct attribute selectors are O(1) - only the attributed element is invalidated
 * (Unlike descendant selectors which require O(n) traversal)
 */
.Pane[data-dragging='true'],
.ContentWrapper[data-dragging='true'] {
  contain: layout style paint;
  pointer-events: none;
  content-visibility: auto;
}

2. Simplify paneUtils.ts

Remove setContainmentOptimizations and removeContainmentOptimizations functions. Keep only the drag handle visual feedback:

type DraggingStylesParams = {
  handle: HTMLElement | null
  pane: HTMLElement | null
  content: HTMLElement | null
}

const DATA_DRAGGING_ATTR = 'data-dragging'

/** Apply visual feedback and performance optimizations during drag */
export function setDraggingStyles({handle, pane, content}: DraggingStylesParams) {
  // Handle visual feedback (must be inline for instant response)
  handle?.style.setProperty('background-color', 'var(--bgColor-accent-emphasis)')
  handle?.style.setProperty('--draggable-handle--drag-opacity', '1')
  handle?.style.setProperty('--draggable-handle--transition', 'none')
  
  // Set attribute for CSS containment (O(1) direct selector, not descendant)
  pane?.setAttribute(DATA_DRAGGING_ATTR, 'true')
  content?.setAttribute(DATA_DRAGGING_ATTR, 'true')
}

/** Remove drag styles and restore normal state */
export function removeDraggingStyles({handle, pane, content}: DraggingStylesParams) {
  handle?.style.removeProperty('background-color')
  handle?.style.removeProperty('--draggable-handle--drag-opacity')
  handle?.style.removeProperty('--draggable-handle--transition')
  
  pane?.removeAttribute(DATA_DRAGGING_ATTR)
  content?.removeAttribute(DATA_DRAGGING_ATTR)
}

3. Update usePaneWidth.ts

Replace inline style containment with attribute toggling for window resize:

In the resize handler, instead of calling setContainmentOptimizations(paneRef.current) and setContainmentOptimizations(contentRef.current), set the data attribute:

const startResizeOptimizations = () => {
  if (isResizing) return
  isResizing = true
  paneRef.current?.setAttribute('data-dragging', 'true')
  contentRef.current?.setAttribute('data-dragging', 'true')
}

const endResizeOptimizations = () => {
  if (!isResizing) return
  isResizing = false
  paneRef.current?.removeAttribute('data-dragging')
  contentRef.current?.removeAttribute('data-dragging')
}

4. Update tests

Update tests in PageLayout.test.tsx and usePaneWidth.test.ts to check for the data-dragging attribute instead of inline contain styles:

// Before
expect(content!.style.getPropertyValue('contain')).toBe('layout style paint')

// After  
expect(content).toHaveAttribute('data-dragging', 'true')

Why This Is Better

Aspect Inline Styles (current #7349) CSS Attribute Selectors (this PR)
Performance O(1) ✅ O(1) ✅
Code complexity setContainmentOptimizations, removeContainmentOptimizations functions Simple setAttribute/removeAttribute
Auditability Must inspect JS to see what styles are applied CSS is declarative, easy to find
Overridability Hard to override inline styles Normal CSS specificity rules
Bundle size JS bytes CSS bytes (better caching)
contain-intrinsic-size Can use dynamic offsetHeight Static or omitted (acceptable tradeoff)

Tradeoff

We lose the ability to set contain-intrinsic-size: auto ${element.offsetHeight}px dynamically. However, this is acceptable because:

  1. content-visibility: auto is only applied during drag (~1 second typically)
  2. User is focused on the drag handle, not scrolling content
  3. Any layout shift is imperceptible during the brief optimization window
  4. Removing contain-intrinsic-size entirely is fine — the browser handles it gracefully

Files to Modify

  1. packages/react/src/PageLayout/PageLayout.module.css — Add direct attribute selector rules
  2. packages/react/src/PageLayout/paneUtils.ts — Simplify to attribute-based approach
  3. packages/react/src/PageLayout/usePaneWidth.ts — Use attributes instead of inline styles for res...

This pull request was created from Copilot chat.


💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

@changeset-bot
Copy link

changeset-bot bot commented Dec 29, 2025

⚠️ No Changeset found

Latest commit: 2da52e4

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

- Add CSS rules with direct attribute selectors for containment
- Simplify paneUtils.ts to use data-dragging attributes
- Update usePaneWidth.ts to use attributes instead of inline styles
- Update tests to check for data-dragging attribute instead of inline styles

Co-authored-by: mattcosta7 <[email protected]>
Copilot AI changed the title [WIP] Simplify drag/resize performance optimizations using CSS selectors Refactor PageLayout drag/resize optimizations to use CSS attribute selectors Dec 29, 2025
Copilot AI requested a review from mattcosta7 December 29, 2025 18:47
@github-actions github-actions bot added the integration-tests: recommended This change needs to be tested for breaking changes. See https://arc.net/l/quote/tdmpakpm label Dec 29, 2025
@github-actions
Copy link
Contributor

👋 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 integration-tests: skipped manually label to skip these checks.

@mattcosta7 mattcosta7 marked this pull request as ready for review December 29, 2025 18:47
@mattcosta7 mattcosta7 requested a review from a team as a code owner December 29, 2025 18:47
@mattcosta7 mattcosta7 merged commit 7fb56ef into second-optimization-pass-for-page-layout Dec 29, 2025
40 of 41 checks passed
@mattcosta7 mattcosta7 deleted the copilot/simplify-drag-resize-optimizations branch December 29, 2025 18:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

integration-tests: recommended This change needs to be tested for breaking changes. See https://arc.net/l/quote/tdmpakpm

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants