Skip to content

Commit 457a1fb

Browse files
authored
perf: optimize DOM operations for better web vitals (INP, CLS, FID) (#657)
* perf: optimize DOM operations and reduce layout thrashing for better web vitals - Batch reflow-causing reads in isFocusable() to minimize layout thrashing - Use Set for O(1) tag lookups instead of Array.includes() - Optimize getClippingRect() and getPositionedParent() with early exits - Batch MutationObserver DOM operations (read phase, then write phase) - Add sr-only inline styles to focus-trap sentinels to prevent CLS - Cache isMacOS() result to avoid repeated userAgent parsing - Add passive: true to mousemove listener in focus-zone - Use :scope selector for faster direct-child sentinel lookup - Replace for...of with indexed for loop in querySelectorAll iteration * fix: remove useless passive option and fix trailing whitespace * chore: add changeset * fix: handle fixed/sticky positioning and correct attribute mutation logic - Fixed isFocusable() to not exclude position:fixed/sticky elements (offsetParent is null for these) - Fixed MutationObserver to check current attribute state instead of relying on oldValue - Use Sets for deduplication in MutationObserver to avoid processing same element twice - Added tests for fixed/sticky positioned elements * fix: disconnect MutationObserver on abort to prevent memory leak * test: add abort cleanup test and update mocks for sticky positioning * fix: remove unused variable from test * update optimizations for trap and zone * add missing test * docs * focus-zone+indexed-set * slight general perf * release refs * refactor: use setAttribute consistently for sentinel elements * refactor: extract createSentinel helper to reduce duplication * refactor: use options object for createSentinel * avoid private field * test: add comprehensive tests for IndexedSet * refactor: remove redundant isMacOS cache (already cached in user-agent.ts)
1 parent f0a3de8 commit 457a1fb

10 files changed

Lines changed: 618 additions & 117 deletions
Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
---
2+
'@primer/behaviors': patch
3+
---
4+
5+
Optimize DOM operations for better web vitals (INP, CLS, FID)
6+
7+
- Batch reflow-causing reads in `isFocusable()` to minimize layout thrashing
8+
- Use `Set` for O(1) tag lookups instead of `Array.includes()`
9+
- Optimize `getClippingRect()` and `getPositionedParent()` with early exits
10+
- Batch MutationObserver DOM operations (read phase, then write phase)
11+
- Add sr-only inline styles to focus-trap sentinels to prevent CLS
12+
- Use `:scope` selector for faster direct-child sentinel lookup
13+
- Add `IndexedSet` for O(1) membership checks in focus zone hot paths

src/__tests__/focus-trap.test.tsx

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -367,3 +367,27 @@ it('Should only have one set of sentinels', async () => {
367367
trapContainer.removeChild(newTrapContainer)
368368
expect(document.querySelectorAll('.sentinel').length).toEqual(2)
369369
})
370+
371+
it('Should apply sr-only styles to sentinels to prevent layout shift', () => {
372+
const {container} = render(
373+
<div id="trapContainer">
374+
<button tabIndex={0}>Apple</button>
375+
</div>,
376+
)
377+
378+
const trapContainer = container.querySelector<HTMLElement>('#trapContainer')!
379+
const controller = focusTrap(trapContainer)
380+
381+
const sentinels = trapContainer.querySelectorAll<HTMLElement>('.sentinel')
382+
expect(sentinels.length).toEqual(2)
383+
384+
// Verify both sentinels have sr-only styles to prevent CLS
385+
for (const sentinel of sentinels) {
386+
expect(sentinel.style.position).toEqual('absolute')
387+
expect(sentinel.style.width).toEqual('1px')
388+
expect(sentinel.style.height).toEqual('1px')
389+
expect(sentinel.style.overflow).toEqual('hidden')
390+
}
391+
392+
controller?.abort()
393+
})

src/__tests__/focus-zone.test.tsx

Lines changed: 40 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,8 @@ beforeAll(() => {
3636
}
3737
}
3838

39-
if (this.style?.position?.toLowerCase() === 'fixed') {
39+
const position = this.style?.position?.toLowerCase()
40+
if (position === 'fixed' || position === 'sticky') {
4041
return null
4142
}
4243

@@ -914,3 +915,41 @@ it('Should not set initial focus via active descendant when focusInStrategy is "
914915

915916
controller.abort()
916917
})
918+
919+
it('Should not respond to DOM changes after abort is called', async () => {
920+
const {container, rerender} = render(
921+
<div id="focusZone">
922+
<button tabIndex={0}>Apple</button>
923+
<button tabIndex={0}>Banana</button>
924+
<button tabIndex={0}>Cantaloupe</button>
925+
</div>,
926+
)
927+
928+
const focusZoneContainer = container.querySelector<HTMLElement>('#focusZone')!
929+
const [firstButton] = focusZoneContainer.querySelectorAll('button')
930+
const controller = focusZone(focusZoneContainer)
931+
932+
firstButton.focus()
933+
expect(document.activeElement).toEqual(firstButton)
934+
935+
// Abort the focus zone
936+
controller.abort()
937+
938+
// Add a new button - should not cause issues since observer is disconnected
939+
rerender(
940+
<div id="focusZone">
941+
<button tabIndex={0}>Apple</button>
942+
<button tabIndex={0}>Banana</button>
943+
<button tabIndex={0}>Cantaloupe</button>
944+
<button tabIndex={0}>Dragonfruit</button>
945+
</div>,
946+
)
947+
948+
await nextTick()
949+
950+
// Focus zone should no longer be managing focus, so all buttons should have their original tabindex
951+
const buttons = focusZoneContainer.querySelectorAll('button')
952+
for (const button of buttons) {
953+
expect(button.getAttribute('tabindex')).toEqual('0')
954+
}
955+
})

src/__tests__/indexed-set.test.ts

Lines changed: 224 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,224 @@
1+
import {IndexedSet} from '../utils/indexed-set'
2+
3+
describe('IndexedSet', () => {
4+
describe('insertAt', () => {
5+
it('inserts elements at the specified index', () => {
6+
const set = new IndexedSet<string>()
7+
set.insertAt(0, 'a', 'b', 'c')
8+
9+
expect(set.get(0)).toBe('a')
10+
expect(set.get(1)).toBe('b')
11+
expect(set.get(2)).toBe('c')
12+
expect(set.size).toBe(3)
13+
})
14+
15+
it('inserts elements in the middle', () => {
16+
const set = new IndexedSet<string>()
17+
set.insertAt(0, 'a', 'c')
18+
set.insertAt(1, 'b')
19+
20+
expect(set.get(0)).toBe('a')
21+
expect(set.get(1)).toBe('b')
22+
expect(set.get(2)).toBe('c')
23+
})
24+
25+
it('deduplicates elements', () => {
26+
const set = new IndexedSet<string>()
27+
set.insertAt(0, 'a', 'b')
28+
set.insertAt(2, 'b', 'c') // 'b' should be ignored
29+
30+
expect(set.size).toBe(3)
31+
expect(set.get(0)).toBe('a')
32+
expect(set.get(1)).toBe('b')
33+
expect(set.get(2)).toBe('c')
34+
})
35+
36+
it('handles inserting all duplicates', () => {
37+
const set = new IndexedSet<string>()
38+
set.insertAt(0, 'a', 'b')
39+
set.insertAt(0, 'a', 'b') // all duplicates
40+
41+
expect(set.size).toBe(2)
42+
})
43+
})
44+
45+
describe('delete', () => {
46+
it('removes an element and returns true', () => {
47+
const set = new IndexedSet<string>()
48+
set.insertAt(0, 'a', 'b', 'c')
49+
50+
expect(set.delete('b')).toBe(true)
51+
expect(set.size).toBe(2)
52+
expect(set.has('b')).toBe(false)
53+
expect(set.get(0)).toBe('a')
54+
expect(set.get(1)).toBe('c')
55+
})
56+
57+
it('returns false for non-existent element', () => {
58+
const set = new IndexedSet<string>()
59+
set.insertAt(0, 'a')
60+
61+
expect(set.delete('b')).toBe(false)
62+
expect(set.size).toBe(1)
63+
})
64+
})
65+
66+
describe('has', () => {
67+
it('returns true for existing elements', () => {
68+
const set = new IndexedSet<string>()
69+
set.insertAt(0, 'a', 'b')
70+
71+
expect(set.has('a')).toBe(true)
72+
expect(set.has('b')).toBe(true)
73+
})
74+
75+
it('returns false for non-existent elements', () => {
76+
const set = new IndexedSet<string>()
77+
set.insertAt(0, 'a')
78+
79+
expect(set.has('b')).toBe(false)
80+
})
81+
82+
it('returns false after element is deleted', () => {
83+
const set = new IndexedSet<string>()
84+
set.insertAt(0, 'a')
85+
set.delete('a')
86+
87+
expect(set.has('a')).toBe(false)
88+
})
89+
})
90+
91+
describe('indexOf', () => {
92+
it('returns the index of an existing element', () => {
93+
const set = new IndexedSet<string>()
94+
set.insertAt(0, 'a', 'b', 'c')
95+
96+
expect(set.indexOf('a')).toBe(0)
97+
expect(set.indexOf('b')).toBe(1)
98+
expect(set.indexOf('c')).toBe(2)
99+
})
100+
101+
it('returns -1 for non-existent elements', () => {
102+
const set = new IndexedSet<string>()
103+
set.insertAt(0, 'a')
104+
105+
expect(set.indexOf('b')).toBe(-1)
106+
})
107+
})
108+
109+
describe('get', () => {
110+
it('returns the element at the specified index', () => {
111+
const set = new IndexedSet<string>()
112+
set.insertAt(0, 'a', 'b', 'c')
113+
114+
expect(set.get(0)).toBe('a')
115+
expect(set.get(1)).toBe('b')
116+
expect(set.get(2)).toBe('c')
117+
})
118+
119+
it('returns undefined for out-of-bounds index', () => {
120+
const set = new IndexedSet<string>()
121+
set.insertAt(0, 'a')
122+
123+
expect(set.get(1)).toBeUndefined()
124+
expect(set.get(-1)).toBeUndefined()
125+
})
126+
})
127+
128+
describe('size', () => {
129+
it('returns 0 for empty set', () => {
130+
const set = new IndexedSet<string>()
131+
expect(set.size).toBe(0)
132+
})
133+
134+
it('returns correct size after insertions', () => {
135+
const set = new IndexedSet<string>()
136+
set.insertAt(0, 'a', 'b', 'c')
137+
expect(set.size).toBe(3)
138+
})
139+
140+
it('returns correct size after deletions', () => {
141+
const set = new IndexedSet<string>()
142+
set.insertAt(0, 'a', 'b', 'c')
143+
set.delete('b')
144+
expect(set.size).toBe(2)
145+
})
146+
})
147+
148+
describe('clear', () => {
149+
it('removes all elements', () => {
150+
const set = new IndexedSet<string>()
151+
set.insertAt(0, 'a', 'b', 'c')
152+
set.clear()
153+
154+
expect(set.size).toBe(0)
155+
expect(set.has('a')).toBe(false)
156+
expect(set.get(0)).toBeUndefined()
157+
})
158+
})
159+
160+
describe('find', () => {
161+
it('returns the first matching element', () => {
162+
const set = new IndexedSet<{id: number; name: string}>()
163+
set.insertAt(0, {id: 1, name: 'a'}, {id: 2, name: 'b'}, {id: 3, name: 'c'})
164+
165+
const found = set.find(el => el.id === 2)
166+
expect(found).toEqual({id: 2, name: 'b'})
167+
})
168+
169+
it('returns undefined when no match', () => {
170+
const set = new IndexedSet<{id: number}>()
171+
set.insertAt(0, {id: 1}, {id: 2})
172+
173+
expect(set.find(el => el.id === 99)).toBeUndefined()
174+
})
175+
})
176+
177+
describe('[Symbol.iterator]', () => {
178+
it('allows iteration with for...of', () => {
179+
const set = new IndexedSet<string>()
180+
set.insertAt(0, 'a', 'b', 'c')
181+
182+
const result: string[] = []
183+
for (const item of set) {
184+
result.push(item)
185+
}
186+
187+
expect(result).toEqual(['a', 'b', 'c'])
188+
})
189+
190+
it('allows spread operator', () => {
191+
const set = new IndexedSet<string>()
192+
set.insertAt(0, 'a', 'b', 'c')
193+
194+
expect([...set]).toEqual(['a', 'b', 'c'])
195+
})
196+
197+
it('allows Array.from', () => {
198+
const set = new IndexedSet<string>()
199+
set.insertAt(0, 'a', 'b', 'c')
200+
201+
expect(Array.from(set)).toEqual(['a', 'b', 'c'])
202+
})
203+
})
204+
205+
describe('with HTMLElements', () => {
206+
it('works with DOM elements', () => {
207+
const set = new IndexedSet<HTMLElement>()
208+
const div1 = document.createElement('div')
209+
const div2 = document.createElement('div')
210+
const div3 = document.createElement('div')
211+
212+
set.insertAt(0, div1, div2, div3)
213+
214+
expect(set.has(div1)).toBe(true)
215+
expect(set.has(div2)).toBe(true)
216+
expect(set.indexOf(div2)).toBe(1)
217+
expect(set.get(0)).toBe(div1)
218+
219+
set.delete(div2)
220+
expect(set.has(div2)).toBe(false)
221+
expect(set.size).toBe(2)
222+
})
223+
})
224+
})

src/__tests__/iterate-focusable-elements.test.tsx

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,8 @@ beforeAll(() => {
2525
}
2626
}
2727

28-
if (this.style?.position?.toLowerCase() === 'fixed') {
28+
const position = this.style?.position?.toLowerCase()
29+
if (position === 'fixed' || position === 'sticky') {
2930
return null
3031
}
3132

@@ -213,6 +214,18 @@ describe('isFocusable', () => {
213214
const focusable = isFocusable(container.firstChild as HTMLElement, true)
214215
expect(focusable).toBeFalsy()
215216
})
217+
218+
it('position: fixed elements are focusable in strict mode', async () => {
219+
const {container} = render(<button style={{position: 'fixed', top: 0, left: 0}}>Fixed Button</button>)
220+
const focusable = isFocusable(container.firstChild as HTMLElement, true)
221+
expect(focusable).toBeTruthy()
222+
})
223+
224+
it('position: sticky elements are focusable in strict mode', async () => {
225+
const {container} = render(<button style={{position: 'sticky', top: 0}}>Sticky Button</button>)
226+
const focusable = isFocusable(container.firstChild as HTMLElement, true)
227+
expect(focusable).toBeTruthy()
228+
})
216229
})
217230

218231
describe('isTabbable', () => {

0 commit comments

Comments
 (0)