Skip to content

Commit

Permalink
Fix scroll bailout logic when targeting fixed/sticky elements (vercel…
Browse files Browse the repository at this point in the history
…#53873)

### What?
When navigating to a new page with fixed or sticky positioned element as the first element, we were bailing on scroll to top behavior, which often isn't expected.

### Why?
Currently, we decide to bail on scroll to top behavior on navigation if the content that is swapped into view is visible within the viewport. Since fixed/sticky positioned elements are often intended to be relative to the current viewport, it's most likely not the case that you'd want it to be considered in this heuristic. For example, if you were scrolled far down on a page, and you navigated to a page that makes use of a sticky header, you would not be scrolled to the top of the page because that sticky header is technically visible within the viewport. 

### How?
I've updated the previous implementation that was intended to skip targeting invisible elements to also skip over fixed or sticky elements. This should help by falling back to the next level of the layout tree to determine which element to scroll to.

I've deleted the `// TODO-APP` comments as I couldn't think of a scenario in which we'd need a global scrollTop handler -- if we've bailed on every element up the tree, it's likely the page wasn't scrollable.

Some additional considerations:
- Is the warning helpful or annoying?
- Is the parallel route trade-off an acceptable one? (ie, a parallel modal slot might not be considered in the content visibility check unless if it’s fixed positioned)

Closes NEXT-1393
Fixes vercel#47475
  • Loading branch information
ztanner committed Aug 15, 2023
1 parent ec6d2c7 commit cb432eb
Show file tree
Hide file tree
Showing 6 changed files with 130 additions and 25 deletions.
27 changes: 19 additions & 8 deletions packages/next/src/client/components/layout-router.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -116,9 +116,22 @@ const rectProperties = [
'y',
] as const
/**
* Check if a HTMLElement is hidden.
* Check if a HTMLElement is hidden or fixed/sticky position
*/
function elementCanScroll(element: HTMLElement) {
function shouldSkipElement(element: HTMLElement) {
// we ignore fixed or sticky positioned elements since they'll likely pass the "in-viewport" check
// and will result in a situation we bail on scroll because of something like a fixed nav,
// even though the actual page content is offscreen
if (['sticky', 'fixed'].includes(getComputedStyle(element).position)) {
if (process.env.NODE_ENV === 'development') {
console.warn(
'Skipping auto-scroll behavior due to `position: sticky` or `position: fixed` on element:',
element
)
}
return true
}

// Uses `getBoundingClientRect` to check if the element is hidden instead of `offsetParent`
// because `offsetParent` doesn't consider document/body
const rect = element.getBoundingClientRect()
Expand Down Expand Up @@ -192,17 +205,15 @@ class InnerScrollAndFocusHandler extends React.Component<ScrollAndFocusHandlerPr
domNode = findDOMNode(this)
}

// TODO-APP: Handle the case where we couldn't select any DOM node, even higher up in the layout-router above the current segmentPath.
// If there is no DOM node this layout-router level is skipped. It'll be handled higher-up in the tree.
if (!(domNode instanceof Element)) {
return
}

// Verify if the element is a HTMLElement and if it's visible on screen (e.g. not display: none).
// If the element is not a HTMLElement or not visible we try to select the next sibling and try again.
while (!(domNode instanceof HTMLElement) || elementCanScroll(domNode)) {
// TODO-APP: Handle the case where we couldn't select any DOM node, even higher up in the layout-router above the current segmentPath.
// No siblings found that are visible so we handle scroll higher up in the tree instead.
// Verify if the element is a HTMLElement and if we want to consider it for scroll behavior.
// If the element is skipped, try to select the next sibling and try again.
while (!(domNode instanceof HTMLElement) || shouldSkipElement(domNode)) {
// No siblings found that match the criteria are found, so handle scroll higher up in the tree instead.
if (domNode.nextElementSibling === null) {
return
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,21 +1,23 @@
export default function Page() {
return (
<div
style={{
position: 'fixed',
top: 0,
left: 0,
width: '100%',
height: '100%',
backgroundColor: 'rgba(0, 0, 0, 0.5)',
display: 'flex',
justifyContent: 'center',
alignItems: 'center',
zIndex: 1000,
}}
id="modal"
>
MODAL
<div>
<div
style={{
position: 'fixed',
top: 0,
left: 0,
width: '100%',
height: '100%',
backgroundColor: 'rgba(0, 0, 0, 0.5)',
display: 'flex',
justifyContent: 'center',
alignItems: 'center',
zIndex: 1000,
}}
id="modal"
>
MODAL
</div>
</div>
)
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
export default function FixedFirstElementPage() {
return (
<>
<nav style={{ position: 'fixed' }}>Fixed nav bar</nav>
<div id="content-that-is-visible" style={{ paddingTop: 40 }}>
Content which is not hidden.
</div>
{
// Repeat 500 elements
Array.from({ length: 500 }, (_, i) => (
<div key={i}>{i}</div>
))
}
</>
)
}
12 changes: 12 additions & 0 deletions test/e2e/app-dir/router-autoscroll/app/page.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,18 @@ export default function Page() {
To invisible first element
</Link>
</div>

<div>
<Link href="/fixed-first-element" id="to-fixed-first-element">
To fixed first element
</Link>
</div>

<div>
<Link href="/sticky-first-element" id="to-sticky-first-element">
To sticky first element
</Link>
</div>
</>
)
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
export default function FixedFirstElementPage() {
return (
<>
<nav style={{ position: 'sticky', top: 0 }}>Sticky nav bar</nav>
<div id="content-that-is-visible" style={{ paddingTop: 40 }}>
Content which is not hidden.
</div>
{
// Repeat 500 elements
Array.from({ length: 500 }, (_, i) => (
<div key={i}>{i}</div>
))
}
</>
)
}
50 changes: 49 additions & 1 deletion test/e2e/app-dir/router-autoscroll/router-autoscroll.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ createNextDescribe(
{
files: __dirname,
},
({ next }) => {
({ next, isNextDev }) => {
type BrowserInterface = Awaited<ReturnType<typeof webdriver>>

const getTopScroll = async (browser: BrowserInterface) =>
Expand Down Expand Up @@ -171,6 +171,54 @@ createNextDescribe(
await check(() => browser.eval('window.scrollY'), 0)
})

it('Should scroll to the top of the layout when the first child is position fixed', async () => {
const browser = await webdriver(next.url, '/')
await browser.eval('window.scrollTo(0, 500)')
await browser
.elementByCss('#to-fixed-first-element')
.click()
.waitForElementByCss('#content-that-is-visible')
await check(() => browser.eval('window.scrollY'), 0)

if (isNextDev) {
// Check that we've logged a warning
await check(async () => {
const logs = await browser.log()
return logs.some((log) =>
log.message.includes(
'Skipping auto-scroll behavior due to `position: sticky` or `position: fixed` on element:'
)
)
? 'success'
: undefined
}, 'success')
}
})

it('Should scroll to the top of the layout when the first child is position sticky', async () => {
const browser = await webdriver(next.url, '/')
await browser.eval('window.scrollTo(0, 500)')
await browser
.elementByCss('#to-sticky-first-element')
.click()
.waitForElementByCss('#content-that-is-visible')
await check(() => browser.eval('window.scrollY'), 0)

if (isNextDev) {
// Check that we've logged a warning
await check(async () => {
const logs = await browser.log()
return logs.some((log) =>
log.message.includes(
'Skipping auto-scroll behavior due to `position: sticky` or `position: fixed` on element:'
)
)
? 'success'
: undefined
}, 'success')
}
})

it('Should apply scroll when loading.js is used', async () => {
const browser = await webdriver(next.url, '/')
await browser.eval('window.scrollTo(0, 500)')
Expand Down

0 comments on commit cb432eb

Please sign in to comment.