Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix scroll bailout logic when targeting fixed/sticky elements #53873

Merged
merged 5 commits into from Aug 15, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
27 changes: 19 additions & 8 deletions packages/next/src/client/components/layout-router.tsx
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
@@ -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>
)
}
@@ -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
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>
</>
)
}
@@ -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
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