-
Notifications
You must be signed in to change notification settings - Fork 27.5k
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
Ensure metadata doesn't break scroll-to-top on navigation #74748
Ensure metadata doesn't break scroll-to-top on navigation #74748
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
Failing test suitesCommit: 3fee0b4
Expand output● Static Image Component Tests › production mode › Should allow an image with a static src to omit height and width
● Static Image Component Tests › production mode › Should use immutable cache-control header for static import
● Static Image Component Tests › production mode › Should use immutable cache-control header even when unoptimized
● Static Image Component Tests › production mode › should have containing followed by for priority image
● Static Image Component Tests › production mode › Should automatically provide an image height and width
● Static Image Component Tests › production mode › should use width and height prop to override import
● Static Image Component Tests › production mode › should use height prop to adjust both width and height
● Static Image Component Tests › production mode › should use width prop to adjust both width and height
● Static Image Component Tests › production mode › should add a data URL placeholder to an image
● Static Image Component Tests › production mode › should add a blur placeholder a statically imported jpg
● Static Image Component Tests › production mode › should add a blur placeholder a statically imported png
● Static Image Component Tests › production mode › should add a blur placeholder a statically imported png with fill
● Static Image Component Tests › production mode › should add placeholder with blurDataURL and fill
● Static Image Component Tests › production mode › should add placeholder even when blurDataURL aspect ratio does not match width/height ratio
● Static Image Component Tests › production mode › should load direct imported image
● Static Image Component Tests › production mode › should load staticprops imported image
Read more about building and testing Next.js in contributing.md.
Expand output● app dir - navigation › hash-link-back-to-same-page › should scroll to the specified hash
Read more about building and testing Next.js in contributing.md. |
Stats from current PRDefault Build (Increase detected
|
vercel/next.js canary | vercel/next.js sebbie/01-10-ensure_metadata_doesn_t_break_scroll-to-top_on_navigation | Change | |
---|---|---|---|
buildDuration | 36.4s | 33.7s | N/A |
buildDurationCached | 30.4s | 27.3s | N/A |
nodeModulesSize | 417 MB | 417 MB | |
nextStartRea..uration (ms) | 896ms | 957ms | N/A |
Client Bundles (main, webpack)
vercel/next.js canary | vercel/next.js sebbie/01-10-ensure_metadata_doesn_t_break_scroll-to-top_on_navigation | Change | |
---|---|---|---|
5306-HASH.js gzip | 53.3 kB | 53.3 kB | N/A |
8276.HASH.js gzip | 169 B | 168 B | N/A |
8377-HASH.js gzip | 5.44 kB | 5.44 kB | N/A |
bccd1874-HASH.js gzip | 53 kB | 53 kB | ✓ |
framework-HASH.js gzip | 57.5 kB | 57.5 kB | N/A |
main-app-HASH.js gzip | 240 B | 242 B | N/A |
main-HASH.js gzip | 34.1 kB | 34.1 kB | N/A |
webpack-HASH.js gzip | 1.71 kB | 1.71 kB | N/A |
Overall change | 53 kB | 53 kB | ✓ |
Legacy Client Bundles (polyfills)
vercel/next.js canary | vercel/next.js sebbie/01-10-ensure_metadata_doesn_t_break_scroll-to-top_on_navigation | Change | |
---|---|---|---|
polyfills-HASH.js gzip | 39.4 kB | 39.4 kB | ✓ |
Overall change | 39.4 kB | 39.4 kB | ✓ |
Client Pages
vercel/next.js canary | vercel/next.js sebbie/01-10-ensure_metadata_doesn_t_break_scroll-to-top_on_navigation | Change | |
---|---|---|---|
_app-HASH.js gzip | 193 B | 193 B | ✓ |
_error-HASH.js gzip | 193 B | 193 B | ✓ |
amp-HASH.js gzip | 512 B | 510 B | N/A |
css-HASH.js gzip | 343 B | 342 B | N/A |
dynamic-HASH.js gzip | 1.84 kB | 1.84 kB | ✓ |
edge-ssr-HASH.js gzip | 265 B | 265 B | ✓ |
head-HASH.js gzip | 363 B | 362 B | N/A |
hooks-HASH.js gzip | 393 B | 392 B | N/A |
image-HASH.js gzip | 4.57 kB | 4.57 kB | N/A |
index-HASH.js gzip | 268 B | 268 B | ✓ |
link-HASH.js gzip | 2.35 kB | 2.34 kB | N/A |
routerDirect..HASH.js gzip | 328 B | 328 B | ✓ |
script-HASH.js gzip | 397 B | 397 B | ✓ |
withRouter-HASH.js gzip | 323 B | 326 B | N/A |
1afbb74e6ecf..834.css gzip | 106 B | 106 B | ✓ |
Overall change | 3.59 kB | 3.59 kB | ✓ |
Client Build Manifests
vercel/next.js canary | vercel/next.js sebbie/01-10-ensure_metadata_doesn_t_break_scroll-to-top_on_navigation | Change | |
---|---|---|---|
_buildManifest.js gzip | 749 B | 747 B | N/A |
Overall change | 0 B | 0 B | ✓ |
Rendered Page Sizes
vercel/next.js canary | vercel/next.js sebbie/01-10-ensure_metadata_doesn_t_break_scroll-to-top_on_navigation | Change | |
---|---|---|---|
index.html gzip | 522 B | 521 B | N/A |
link.html gzip | 538 B | 535 B | N/A |
withRouter.html gzip | 518 B | 518 B | ✓ |
Overall change | 518 B | 518 B | ✓ |
Edge SSR bundle Size
vercel/next.js canary | vercel/next.js sebbie/01-10-ensure_metadata_doesn_t_break_scroll-to-top_on_navigation | Change | |
---|---|---|---|
edge-ssr.js gzip | 128 kB | 128 kB | N/A |
page.js gzip | 207 kB | 207 kB | N/A |
Overall change | 0 B | 0 B | ✓ |
Middleware size
vercel/next.js canary | vercel/next.js sebbie/01-10-ensure_metadata_doesn_t_break_scroll-to-top_on_navigation | Change | |
---|---|---|---|
middleware-b..fest.js gzip | 670 B | 669 B | N/A |
middleware-r..fest.js gzip | 155 B | 156 B | N/A |
middleware.js gzip | 31.2 kB | 31.2 kB | N/A |
edge-runtime..pack.js gzip | 844 B | 844 B | ✓ |
Overall change | 844 B | 844 B | ✓ |
Next Runtimes
vercel/next.js canary | vercel/next.js sebbie/01-10-ensure_metadata_doesn_t_break_scroll-to-top_on_navigation | Change | |
---|---|---|---|
274-experime...dev.js gzip | 322 B | 322 B | ✓ |
274.runtime.dev.js gzip | 314 B | 314 B | ✓ |
app-page-exp...dev.js gzip | 368 kB | 368 kB | ✓ |
app-page-exp..prod.js gzip | 129 kB | 129 kB | N/A |
app-page-tur..prod.js gzip | 142 kB | 142 kB | N/A |
app-page-tur..prod.js gzip | 138 kB | 138 kB | N/A |
app-page.run...dev.js gzip | 356 kB | 356 kB | N/A |
app-page.run..prod.js gzip | 126 kB | 126 kB | N/A |
app-route-ex...dev.js gzip | 37.6 kB | 37.6 kB | ✓ |
app-route-ex..prod.js gzip | 25.6 kB | 25.6 kB | ✓ |
app-route-tu..prod.js gzip | 25.6 kB | 25.6 kB | ✓ |
app-route-tu..prod.js gzip | 25.4 kB | 25.4 kB | ✓ |
app-route.ru...dev.js gzip | 39.2 kB | 39.2 kB | ✓ |
app-route.ru..prod.js gzip | 25.4 kB | 25.4 kB | ✓ |
pages-api-tu..prod.js gzip | 9.69 kB | 9.69 kB | ✓ |
pages-api.ru...dev.js gzip | 11.6 kB | 11.6 kB | ✓ |
pages-api.ru..prod.js gzip | 9.68 kB | 9.68 kB | ✓ |
pages-turbo...prod.js gzip | 21.7 kB | 21.7 kB | ✓ |
pages.runtim...dev.js gzip | 27.5 kB | 27.5 kB | ✓ |
pages.runtim..prod.js gzip | 21.7 kB | 21.7 kB | ✓ |
server.runti..prod.js gzip | 916 kB | 916 kB | ✓ |
Overall change | 1.57 MB | 1.57 MB | ✓ |
build cache Overall increase ⚠️
vercel/next.js canary | vercel/next.js sebbie/01-10-ensure_metadata_doesn_t_break_scroll-to-top_on_navigation | Change | |
---|---|---|---|
0.pack gzip | 2.09 MB | 2.09 MB | |
index.pack gzip | 76.2 kB | 75.8 kB | N/A |
Overall change | 2.09 MB | 2.09 MB |
Diff details
Diff for 5306-HASH.js
Diff too large to display
Diff for main-HASH.js
Diff too large to display
Diff for app-page-exp..ntime.dev.js
Diff too large to display
Diff for app-page-exp..time.prod.js
Diff too large to display
Diff for app-page-tur..time.prod.js
Diff too large to display
Diff for app-page-tur..time.prod.js
Diff too large to display
Diff for app-page.runtime.dev.js
Diff too large to display
Diff for app-page.runtime.prod.js
Diff too large to display
{/* | ||
* The order here matters since a parent might call findDOMNode(). | ||
* findDOMNode() will return the first child if multiple children are rendered. | ||
* But React will hoist metadata into <head> which breaks scroll handling. | ||
*/} | ||
{metadata} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With Fragment Refs, we wouldn't have this problem. We'd just iterate over the children and skip over metadata.
Or rather skip over any hoistable
0093173
to
503fc37
Compare
503fc37
to
e8835a4
Compare
e8835a4
to
eb1661f
Compare
@@ -153,7 +168,7 @@ describe('router autoscrolling on navigation', () => { | |||
return ( | |||
content + | |||
` | |||
\\\\ Add this meaningless comment to force refresh | |||
// Add this meaningless comment to force refresh |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This wasn't a comment and just triggered a build error polluting test assertions i.e. fixes
Error: × Expected unicode escape
╭─[/Users/sebbie/repos/next.js/test/e2e/app-dir/router-autoscroll/app/[layoutPaddingWidth]/[layoutPaddingHeight]/[pageWidth]/[pageHeight]/[param]/page.tsx:14:1]
11 │ )
12 │ }
13 │
14 │ \\\\ Add this meaningless comment to force refresh
· ▲
╰────
e821c8b
to
9409864
Compare
test/lib/next-test-utils.ts
Outdated
log.message.startsWith( | ||
'Failed to load resource: the server responded with a status of 404' | ||
)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Message format is slightly different on deploy tests: https://github.com/vercel/next.js/actions/runs/12712892279/job/35440856951?pr=74748#step:29:105
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just reverts the snapshot changes in #74262 which introduced this bug.
9409864
to
3fee0b4
Compare
expect(await getRedboxTotalErrorCount(browser)).toBe(2) | ||
await retry(async () => { | ||
expect(await getRedboxTotalErrorCount(browser)).toBe(2) | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We already did that in some other instances where we assert on multiple errors.
findDOMNode()
returns the DOM element of the first child. However, this is not what we want inInnerScrollAndFocusHandler
if metadata is the first child since React will have hoisted those into<head>
by the time e.g.componentDidMount
runs (or any other after mutation phase handlers). The fix is to ensure the relevant element comes before metadata in the tree. This shouldn't break rendering prio since these nodes are still siblings and we have sibling pre-warming now.The alternative would be to move the handling of
InnerScrollAndFocusHandler
handler intocreate-component-tree
to ensure it's wrapped around the designated element. But that might result in missing to addInnerScrollAndFocusHandler
which you probably don't think about when working oncreate-component-tree
.Added a dev-only warning to
ScrollAndFocus
iffindDOMNode(this)
returns an element that renders in<head>
. Might be better to throw in our tests. Usually not a good idea to have different flow between test and prod but better chances it is flushed out in tests. There were many more tests that hit this warning without failing the test: #74751Closes https://linear.app/vercel/issue/NDX-651