Skip to content

Commit 5509e1b

Browse files
eps1loneps1lon
authored
[fragment-scroll] Stop focusing the first focusable host descendant (#89903)
Flagged behind `experimental.appNewScrollHandler` `focus()` on Fragment refs works different than `findDOMNode(this).focus()`. `findDOMNode` will return an actual host element so it's effectively `HTMLElement.focus()`. If the element isn't focuseable, `focus()` will do nothing. By spec, there's also no way to determine if an element is focusable. It's up to user agents (e.g. sometimes scrollable containers are considered focusable). `FragmentInstance.focus` will focus the first focusable descendant. That could be a focusable element deep within the fragment e.g. `<Fragment><section><p>A lot of text</p><a href="">Next page</a></Fragment>`. Focusing such a deep element may sometimes be counter productive. Now we just blur the active element. This is how a hard navigation already works today and ensures we don't skip past content of the new segment. This means you potentially stay a long way before the actual Segment (e.g. in a preceding nav) with the focus cursor which could be annoying. We could be clever and temporarily insert a focusable element at the start of the Segment, focus it, and then immediately remove. That would leave the focus cursor right before the Segment. However, this requires knowledge about the parent content (can we just insert an element of our choosing?) and relies on knowing what's focusable (a button would be a good bet). That's a bit too clever for a fix without concrete motivation so I'm waiting somebody to complain about the new behavior.
1 parent e17b115 commit 5509e1b

4 files changed

Lines changed: 79 additions & 40 deletions

File tree

‎packages/next/src/client/components/layout-router.tsx‎

Lines changed: 30 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -139,12 +139,12 @@ function getHashFragmentDomNode(hashFragment: string) {
139139
document.getElementsByName(hashFragment)[0]
140140
)
141141
}
142-
interface ScrollAndFocusHandlerProps {
142+
interface ScrollAndMaybeFocusHandlerProps {
143143
focusAndScrollRef: FocusAndScrollRef
144144
children: React.ReactNode
145145
segmentPath: FlightSegmentPath
146146
}
147-
class InnerScrollAndFocusHandlerOld extends React.Component<ScrollAndFocusHandlerProps> {
147+
class InnerScrollAndFocusHandlerOld extends React.Component<ScrollAndMaybeFocusHandlerProps> {
148148
handlePotentialScroll = () => {
149149
// Handle scroll and focus, it's only applied once.
150150
const { focusAndScrollRef, segmentPath } = this.props
@@ -265,7 +265,11 @@ class InnerScrollAndFocusHandlerOld extends React.Component<ScrollAndFocusHandle
265265
}
266266
}
267267

268-
function InnerScrollAndFocusHandlerNew(props: ScrollAndFocusHandlerProps) {
268+
/**
269+
* Fork of InnerScrollAndFocusHandlerOld using Fragment refs for scrolling.
270+
* No longer focuses the first host descendant.
271+
*/
272+
function InnerScrollHandlerNew(props: ScrollAndMaybeFocusHandlerProps) {
269273
const childrenRef = React.useRef<FragmentInstance>(null)
270274

271275
useLayoutEffect(
@@ -309,6 +313,22 @@ function InnerScrollAndFocusHandlerNew(props: ScrollAndFocusHandlerProps) {
309313
focusAndScrollRef.hashFragment = null
310314
focusAndScrollRef.segmentPaths = []
311315

316+
const activeElement = document.activeElement
317+
if (
318+
activeElement !== null &&
319+
'blur' in activeElement &&
320+
typeof activeElement.blur === 'function'
321+
) {
322+
// Trying to match hard navigations.
323+
// Ideally we'd move the internal focus cursor either to the top
324+
// or at least before the segment. But there's no DOM API to do that,
325+
// so we just blur.
326+
// We could workaround this by moving focus to a temporary element in
327+
// the body. But adding elements might trigger layout or other effects
328+
// so it should be well motivated.
329+
activeElement.blur()
330+
}
331+
312332
disableSmoothScrollDuringRouteTransition(
313333
() => {
314334
// In case of hash scroll, we only need to scroll the element into view
@@ -348,11 +368,6 @@ function InnerScrollAndFocusHandlerNew(props: ScrollAndFocusHandlerProps) {
348368

349369
// Mutate after scrolling so that it can be read by `disableSmoothScrollDuringRouteTransition`
350370
focusAndScrollRef.onlyHashChange = false
351-
352-
// Set focus on the element but don't scroll since we already did that.
353-
// The focus might have targetted a deep element outside of the instances
354-
// top edge.
355-
instance.focus({ preventScroll: true })
356371
}
357372
},
358373
// Used to run on every commit. We may be able to be smarter about this
@@ -363,11 +378,11 @@ function InnerScrollAndFocusHandlerNew(props: ScrollAndFocusHandlerProps) {
363378
return <Fragment ref={childrenRef}>{props.children}</Fragment>
364379
}
365380

366-
const InnerScrollAndFocusHandler = enableNewScrollHandler
367-
? InnerScrollAndFocusHandlerNew
381+
const InnerScrollAndMaybeFocusHandler = enableNewScrollHandler
382+
? InnerScrollHandlerNew
368383
: InnerScrollAndFocusHandlerOld
369384

370-
function ScrollAndFocusHandler({
385+
function ScrollAndMaybeFocusHandler({
371386
segmentPath,
372387
children,
373388
}: {
@@ -380,12 +395,12 @@ function ScrollAndFocusHandler({
380395
}
381396

382397
return (
383-
<InnerScrollAndFocusHandler
398+
<InnerScrollAndMaybeFocusHandler
384399
segmentPath={segmentPath}
385400
focusAndScrollRef={context.focusAndScrollRef}
386401
>
387402
{children}
388-
</InnerScrollAndFocusHandler>
403+
</InnerScrollAndMaybeFocusHandler>
389404
)
390405
}
391406

@@ -768,7 +783,7 @@ export default function OuterLayoutRouter({
768783
<TemplateContext.Provider
769784
key={stateKey}
770785
value={
771-
<ScrollAndFocusHandler segmentPath={segmentPath}>
786+
<ScrollAndMaybeFocusHandler segmentPath={segmentPath}>
772787
<ErrorBoundary
773788
errorComponent={error}
774789
errorStyles={errorStyles}
@@ -809,7 +824,7 @@ export default function OuterLayoutRouter({
809824
</LoadingBoundary>
810825
</ErrorBoundary>
811826
{segmentViewStateNode}
812-
</ScrollAndFocusHandler>
827+
</ScrollAndMaybeFocusHandler>
813828
}
814829
>
815830
{templateStyles}

‎test/development/browser-logs/browser-logs.test.ts‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -8,8 +8,8 @@ import { retry } from 'next-test-utils'
88
const bundlerName = process.env.IS_TURBOPACK_TEST ? 'Turbopack' : 'Webpack'
99
const enableNewScrollHandler =
1010
process.env.__NEXT_EXPERIMENTAL_APP_NEW_SCROLL_HANDLER === 'true'
11-
const innerScrollAndFocusHandlerName = enableNewScrollHandler
12-
? 'InnerScrollAndFocusHandlerNew'
11+
const innerScrollAndMaybeFocusHandlerName = enableNewScrollHandler
12+
? 'InnerScrollHandlerNew'
1313
: 'InnerScrollAndFocusHandlerOld'
1414

1515
function setupLogCapture() {
@@ -336,8 +336,8 @@ describe(`Terminal Logging (${bundlerName})`, () => {
336336
337337
...
338338
<RenderFromTemplateContext>
339-
<ScrollAndFocusHandler segmentPath={[...]}>
340-
<${innerScrollAndFocusHandlerName} segmentPath={[...]} focusAndScrollRef={{apply:false, ...}}>
339+
<ScrollAndMaybeFocusHandler segmentPath={[...]}>
340+
<${innerScrollAndMaybeFocusHandlerName} segmentPath={[...]} focusAndScrollRef={{apply:false, ...}}>
341341
<ErrorBoundary errorComponent={undefined} errorStyles={undefined} errorScripts={undefined}>
342342
<LoadingBoundary name="hydration-..." loading={null}>
343343
<HTTPAccessFallbackBoundary notFound={undefined} forbidden={undefined} unauthorized={undefined}>

‎test/e2e/app-dir/navigation-focus/navigation-focus.test.ts‎

Lines changed: 43 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -16,11 +16,17 @@ describe('navigation-focus', () => {
1616
await retry(async () => {
1717
// Good debug info is a moving target. Use Playwright traces to find out
1818
// what was focused if this fails
19-
expect(
20-
await browser.eval(() =>
21-
document.activeElement.getAttribute('data-testid')
19+
if (enableNewScrollHandler) {
20+
expect(await browser.eval(() => document.activeElement.localName)).toBe(
21+
'body'
2222
)
23-
).toBe('segment-container')
23+
} else {
24+
expect(
25+
await browser.eval(() =>
26+
document.activeElement.getAttribute('data-testid')
27+
)
28+
).toBe('segment-container')
29+
}
2430
})
2531
})
2632

@@ -29,11 +35,17 @@ describe('navigation-focus', () => {
2935
await browser.elementByCss('a[href="/scrollable-segment"]').click()
3036

3137
await retry(async () => {
32-
expect(
33-
await browser.eval(() =>
34-
document.activeElement.getAttribute('data-testid')
38+
if (enableNewScrollHandler) {
39+
expect(await browser.eval(() => document.activeElement.localName)).toBe(
40+
'body'
3541
)
36-
).toBe('segment-container')
42+
} else {
43+
expect(
44+
await browser.eval(() =>
45+
document.activeElement.getAttribute('data-testid')
46+
)
47+
).toBe('segment-container')
48+
}
3749
})
3850
})
3951

@@ -46,9 +58,9 @@ describe('navigation-focus', () => {
4658
await retry(async () => {
4759
if (enableNewScrollHandler) {
4860
// Focus goes to the focusable descendant, not the segment itself
49-
expect(
50-
await browser.eval(() => document.activeElement.textContent)
51-
).toBe('Focusable Button')
61+
expect(await browser.eval(() => document.activeElement.localName)).toBe(
62+
'body'
63+
)
5264
} else {
5365
// Focus stays on the original link
5466
expect(
@@ -63,10 +75,16 @@ describe('navigation-focus', () => {
6375
await browser.elementByCss('a[href="/uri-fragments#section-2"]').click()
6476

6577
await retry(async () => {
66-
// Focus stays on the anchor unlike native behavior
67-
expect(
68-
await browser.eval(() => document.activeElement.getAttribute('href'))
69-
).toEqual('/uri-fragments#section-2')
78+
if (enableNewScrollHandler) {
79+
expect(await browser.eval(() => document.activeElement.localName)).toBe(
80+
'body'
81+
)
82+
} else {
83+
// Focus stays on the anchor unlike native behavior
84+
expect(
85+
await browser.eval(() => document.activeElement.getAttribute('href'))
86+
).toEqual('/uri-fragments#section-2')
87+
}
7088
})
7189
// Fragment URI not targetted unlike native behavior
7290
expect(await browser.locator(':target').isVisible()).toEqual(false)
@@ -77,10 +95,16 @@ describe('navigation-focus', () => {
7795
await browser.elementByCss('a[href="#section-1"]').click()
7896

7997
await retry(async () => {
80-
// Focus stays on the anchor unlike native behavior
81-
expect(
82-
await browser.eval(() => document.activeElement.getAttribute('href'))
83-
).toEqual('#section-1')
98+
if (enableNewScrollHandler) {
99+
expect(await browser.eval(() => document.activeElement.localName)).toBe(
100+
'body'
101+
)
102+
} else {
103+
// Focus stays on the anchor unlike native behavior
104+
expect(
105+
await browser.eval(() => document.activeElement.getAttribute('href'))
106+
).toEqual('#section-1')
107+
}
84108
})
85109
// Fragment URI not targetted unlike native behavior
86110
expect(await browser.locator(':target').isVisible()).toEqual(false)

‎test/lib/next-test-utils.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1487,12 +1487,12 @@ const nextjsClientComponentNames = [
14871487
'HTTPAccessFallbackErrorBoundary',
14881488
'InnerLayoutRouter',
14891489
'InnerScrollAndFocusHandlerOld',
1490-
'InnerScrollAndFocusHandlerNew',
1490+
'InnerScrollHandlerNew',
14911491
'RedirectBoundary',
14921492
'RedirectErrorBoundary',
14931493
'RenderFromTemplateContext',
14941494
'Root',
1495-
'ScrollAndFocusHandler',
1495+
'ScrollAndMaybeFocusHandler',
14961496
'SegmentViewNode',
14971497
'SegmentTrieNode',
14981498
// These are added due to user actions e.g. loading.js -> LoadingBoundary

0 commit comments

Comments
 (0)