Repository: vercel/next.js
Pull request: [fragment-scroll] Stop focusing the first focusable host descendant

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.

Changed files:
filename: packages/next/src/client/components/layout-router.tsx
patch: @@ -139,12 +139,12 @@ function getHashFragmentDomNode(hashFragment: string) {
     document.getElementsByName(hashFragment)[0]
   )
 }
-interface ScrollAndFocusHandlerProps {
+interface ScrollAndMaybeFocusHandlerProps {
   focusAndScrollRef: FocusAndScrollRef
   children: React.ReactNode
   segmentPath: FlightSegmentPath
 }
-class InnerScrollAndFocusHandlerOld extends React.Component<ScrollAndFocusHandlerProps> {
+class InnerScrollAndFocusHandlerOld extends React.Component<ScrollAndMaybeFocusHandlerProps> {
   handlePotentialScroll = () => {
     // Handle scroll and focus, it's only applied once.
     const { focusAndScrollRef, segmentPath } = this.props
@@ -265,7 +265,11 @@ class InnerScrollAndFocusHandlerOld extends React.Component<ScrollAndFocusHandle
   }
 }
 
-function InnerScrollAndFocusHandlerNew(props: ScrollAndFocusHandlerProps) {
+/**
+ * Fork of InnerScrollAndFocusHandlerOld using Fragment refs for scrolling.
+ * No longer focuses the first host descendant.
+ */
+function InnerScrollHandlerNew(props: ScrollAndMaybeFocusHandlerProps) {
   const childrenRef = React.useRef<FragmentInstance>(null)
 
   useLayoutEffect(
@@ -309,6 +313,22 @@ function InnerScrollAndFocusHandlerNew(props: ScrollAndFocusHandlerProps) {
         focusAndScrollRef.hashFragment = null
         focusAndScrollRef.segmentPaths = []
 
+        const activeElement = document.activeElement
+        if (
+          activeElement !== null &&
+          'blur' in activeElement &&
+          typeof activeElement.blur === 'function'
+        ) {
+          // Trying to match hard navigations.
+          // Ideally we'd move the internal focus cursor either to the top
+          // or at least before the segment. But there's no DOM API to do that,
+          // so we just blur.
+          // We could workaround this by moving focus to a temporary element in
+          // the body. But adding elements might trigger layout or other effects
+          // so it should be well motivated.
+          activeElement.blur()
+        }
+
         disableSmoothScrollDuringRouteTransition(
           () => {
             // In case of hash scroll, we only need to scroll the element into view
@@ -348,11 +368,6 @@ function InnerScrollAndFocusHandlerNew(props: ScrollAndFocusHandlerProps) {
 
         // Mutate after scrolling so that it can be read by `disableSmoothScrollDuringRouteTransition`
         focusAndScrollRef.onlyHashChange = false
-
-        // Set focus on the element but don't scroll since we already did that.
-        // The focus might have targetted a deep element outside of the instances
-        // top edge.
-        instance.focus({ preventScroll: true })
       }
     },
     // Used to run on every commit. We may be able to be smarter about this
@@ -363,11 +378,11 @@ function InnerScrollAndFocusHandlerNew(props: ScrollAndFocusHandlerProps) {
   return <Fragment ref={childrenRef}>{props.children}</Fragment>
 }
 
-const InnerScrollAndFocusHandler = enableNewScrollHandler
-  ? InnerScrollAndFocusHandlerNew
+const InnerScrollAndMaybeFocusHandler = enableNewScrollHandler
+  ? InnerScrollHandlerNew
   : InnerScrollAndFocusHandlerOld
 
-function ScrollAndFocusHandler({
+function ScrollAndMaybeFocusHandler({
   segmentPath,
   children,
 }: {
@@ -380,12 +395,12 @@ function ScrollAndFocusHandler({
   }
 
   return (
-    <InnerScrollAndFocusHandler
+    <InnerScrollAndMaybeFocusHandler
       segmentPath={segmentPath}
       focusAndScrollRef={context.focusAndScrollRef}
     >
       {children}
-    </InnerScrollAndFocusHandler>
+    </InnerScrollAndMaybeFocusHandler>
   )
 }
 
@@ -768,7 +783,7 @@ export default function OuterLayoutRouter({
       <TemplateContext.Provider
         key={stateKey}
         value={
-          <ScrollAndFocusHandler segmentPath={segmentPath}>
+          <ScrollAndMaybeFocusHandler segmentPath={segmentPath}>
             <ErrorBoundary
               errorComponent={error}
               errorStyles={errorStyles}
@@ -809,7 +824,7 @@ export default function OuterLayoutRouter({
               </LoadingBoundary>
             </ErrorBoundary>
             {segmentViewStateNode}
-          </ScrollAndFocusHandler>
+          </ScrollAndMaybeFocusHandler>
         }
       >
         {templateStyles}

filename: test/development/browser-logs/browser-logs.test.ts
patch: @@ -8,8 +8,8 @@ import { retry } from 'next-test-utils'
 const bundlerName = process.env.IS_TURBOPACK_TEST ? 'Turbopack' : 'Webpack'
 const enableNewScrollHandler =
   process.env.__NEXT_EXPERIMENTAL_APP_NEW_SCROLL_HANDLER === 'true'
-const innerScrollAndFocusHandlerName = enableNewScrollHandler
-  ? 'InnerScrollAndFocusHandlerNew'
+const innerScrollAndMaybeFocusHandlerName = enableNewScrollHandler
+  ? 'InnerScrollHandlerNew'
   : 'InnerScrollAndFocusHandlerOld'
 
 function setupLogCapture() {
@@ -336,8 +336,8 @@ describe(`Terminal Logging (${bundlerName})`, () => {
 
          ...
            <RenderFromTemplateContext>
-             <ScrollAndFocusHandler segmentPath={[...]}>
-               <${innerScrollAndFocusHandlerName} segmentPath={[...]} focusAndScrollRef={{apply:false, ...}}>
+             <ScrollAndMaybeFocusHandler segmentPath={[...]}>
+               <${innerScrollAndMaybeFocusHandlerName} segmentPath={[...]} focusAndScrollRef={{apply:false, ...}}>
                  <ErrorBoundary errorComponent={undefined} errorStyles={undefined} errorScripts={undefined}>
                    <LoadingBoundary name="hydration-..." loading={null}>
                      <HTTPAccessFallbackBoundary notFound={undefined} forbidden={undefined} unauthorized={undefined}>

filename: test/e2e/app-dir/navigation-focus/navigation-focus.test.ts
patch: @@ -16,11 +16,17 @@ describe('navigation-focus', () => {
     await retry(async () => {
       // Good debug info is a moving target. Use Playwright traces to find out
       // what was focused if this fails
-      expect(
-        await browser.eval(() =>
-          document.activeElement.getAttribute('data-testid')
+      if (enableNewScrollHandler) {
+        expect(await browser.eval(() => document.activeElement.localName)).toBe(
+          'body'
         )
-      ).toBe('segment-container')
+      } else {
+        expect(
+          await browser.eval(() =>
+            document.activeElement.getAttribute('data-testid')
+          )
+        ).toBe('segment-container')
+      }
     })
   })
 
@@ -29,11 +35,17 @@ describe('navigation-focus', () => {
     await browser.elementByCss('a[href="/scrollable-segment"]').click()
 
     await retry(async () => {
-      expect(
-        await browser.eval(() =>
-          document.activeElement.getAttribute('data-testid')
+      if (enableNewScrollHandler) {
+        expect(await browser.eval(() => document.activeElement.localName)).toBe(
+          'body'
         )
-      ).toBe('segment-container')
+      } else {
+        expect(
+          await browser.eval(() =>
+            document.activeElement.getAttribute('data-testid')
+          )
+        ).toBe('segment-container')
+      }
     })
   })
 
@@ -46,9 +58,9 @@ describe('navigation-focus', () => {
     await retry(async () => {
       if (enableNewScrollHandler) {
         // Focus goes to the focusable descendant, not the segment itself
-        expect(
-          await browser.eval(() => document.activeElement.textContent)
-        ).toBe('Focusable Button')
+        expect(await browser.eval(() => document.activeElement.localName)).toBe(
+          'body'
+        )
       } else {
         // Focus stays on the original link
         expect(
@@ -63,10 +75,16 @@ describe('navigation-focus', () => {
     await browser.elementByCss('a[href="/uri-fragments#section-2"]').click()
 
     await retry(async () => {
-      // Focus stays on the anchor unlike native behavior
-      expect(
-        await browser.eval(() => document.activeElement.getAttribute('href'))
-      ).toEqual('/uri-fragments#section-2')
+      if (enableNewScrollHandler) {
+        expect(await browser.eval(() => document.activeElement.localName)).toBe(
+          'body'
+        )
+      } else {
+        // Focus stays on the anchor unlike native behavior
+        expect(
+          await browser.eval(() => document.activeElement.getAttribute('href'))
+        ).toEqual('/uri-fragments#section-2')
+      }
     })
     // Fragment URI not targetted unlike native behavior
     expect(await browser.locator(':target').isVisible()).toEqual(false)
@@ -77,10 +95,16 @@ describe('navigation-focus', () => {
     await browser.elementByCss('a[href="#section-1"]').click()
 
     await retry(async () => {
-      // Focus stays on the anchor unlike native behavior
-      expect(
-        await browser.eval(() => document.activeElement.getAttribute('href'))
-      ).toEqual('#section-1')
+      if (enableNewScrollHandler) {
+        expect(await browser.eval(() => document.activeElement.localName)).toBe(
+          'body'
+        )
+      } else {
+        // Focus stays on the anchor unlike native behavior
+        expect(
+          await browser.eval(() => document.activeElement.getAttribute('href'))
+        ).toEqual('#section-1')
+      }
     })
     // Fragment URI not targetted unlike native behavior
     expect(await browser.locator(':target').isVisible()).toEqual(false)

filename: test/lib/next-test-utils.ts
patch: @@ -1487,12 +1487,12 @@ const nextjsClientComponentNames = [
   'HTTPAccessFallbackErrorBoundary',
   'InnerLayoutRouter',
   'InnerScrollAndFocusHandlerOld',
-  'InnerScrollAndFocusHandlerNew',
+  'InnerScrollHandlerNew',
   'RedirectBoundary',
   'RedirectErrorBoundary',
   'RenderFromTemplateContext',
   'Root',
-  'ScrollAndFocusHandler',
+  'ScrollAndMaybeFocusHandler',
   'SegmentViewNode',
   'SegmentTrieNode',
   // These are added due to user actions e.g. loading.js -> LoadingBoundary