Fix turbo-refresh-scroll ignored on error responses#1462
Fix turbo-refresh-scroll ignored on error responses#1462yujiteshima wants to merge 5 commits intohotwired:mainfrom
Conversation
seanpdoyle
left a comment
seanpdoyle
left a comment
There was a problem hiding this comment.
The introduction of 404.html and 500.html as dedicated routes introduced in the src/tests/server.mjs changes deviate from how page refreshes behave in real world applications.
Since the <a> elements drive the page to a URL that differs from the one initiating the request, it does not reproduce the Page Refresh behavior outlined in the original issue (#1449).
Have you explored alternatives that drives to the same URL that's initiating the navigation?
| assert.notOk(await isScrolledToTop(page), "page is scrolled down") | ||
|
|
||
| await page.click("#link-404") | ||
| await nextBody(page) | ||
| await nextBeat() | ||
|
|
||
| assert.ok(await isScrolledToTop(page), "page is scrolled to the top after 404 error") | ||
| assert.equal(await page.locator("h1").textContent(), "Not Found") |
There was a problem hiding this comment.
If these assert calls were replaced with async expect-based expectations, and the h1 assertions were first, the nextBody and nextBeat calls might not be necessary:
Could these calls to assert be replaced with expect calls?
| assert.notOk(await isScrolledToTop(page), "page is scrolled down") | |
| await page.click("#link-404") | |
| await nextBody(page) | |
| await nextBeat() | |
| assert.ok(await isScrolledToTop(page), "page is scrolled to the top after 404 error") | |
| assert.equal(await page.locator("h1").textContent(), "Not Found") | |
| expect(await isScrolledToTop(page), "page is scrolled down").toEqual(false) | |
| await page.click("#link-404") | |
| await expect(page.locator("h1")).toHaveText("Not Found") | |
| expect(await isScrolledToTop(page), "page is scrolled to the top after 404 error").toEqual(true) | |
| }) |
| assert.notOk(await isScrolledToTop(page), "page is scrolled down") | ||
|
|
||
| await page.click("#link-500") | ||
| await nextBody(page) | ||
|
|
||
| assert.ok(await isScrolledToTop(page), "page is scrolled to the top after 500 error") | ||
| assert.equal(await page.locator("h1").textContent(), "Internal Server Error") |
There was a problem hiding this comment.
| assert.notOk(await isScrolledToTop(page), "page is scrolled down") | |
| await page.click("#link-500") | |
| await nextBody(page) | |
| assert.ok(await isScrolledToTop(page), "page is scrolled to the top after 500 error") | |
| assert.equal(await page.locator("h1").textContent(), "Internal Server Error") | |
| expect(await isScrolledToTop(page), "page is scrolled down").toEqual(false) | |
| await page.click("#link-500") | |
| await expect(page.locator("h1")).toHaveText("Internal Server Error") | |
| expect(await isScrolledToTop(page), "page is scrolled to the top after 500 error").toEqual(true) |
| assert.notOk(await isScrolledToTop(page), "page is scrolled down") | ||
|
|
||
| await page.click("#link-success") | ||
| await nextBody(page) | ||
|
|
||
| assert.ok(await isScrolledToTop(page), "page is scrolled to the top after success") | ||
| assert.equal(await page.locator("h1").textContent(), "One") |
There was a problem hiding this comment.
| assert.notOk(await isScrolledToTop(page), "page is scrolled down") | |
| await page.click("#link-success") | |
| await nextBody(page) | |
| assert.ok(await isScrolledToTop(page), "page is scrolled to the top after success") | |
| assert.equal(await page.locator("h1").textContent(), "One") | |
| expect(await isScrolledToTop(page), "page is scrolled down").toEqual(false) | |
| await page.click("#link-success") | |
| await expect(page.locator("h1")).toHaveText("One") | |
| expect(await isScrolledToTop(page), "page is scrolled to the top after success").toEqual(true) |
| assert.notOk(await isScrolledToTop(page), "page is scrolled down") | ||
|
|
||
| await page.click("#link-404") | ||
| await nextBody(page) | ||
|
|
||
| const newScrollY = await page.evaluate(() => window.scrollY) | ||
| assert.equal(newScrollY, scrollY, "scroll position should be preserved after 404 error") | ||
| assert.equal(await page.locator("h1").textContent(), "Not Found") |
There was a problem hiding this comment.
| assert.notOk(await isScrolledToTop(page), "page is scrolled down") | |
| await page.click("#link-404") | |
| await nextBody(page) | |
| const newScrollY = await page.evaluate(() => window.scrollY) | |
| assert.equal(newScrollY, scrollY, "scroll position should be preserved after 404 error") | |
| assert.equal(await page.locator("h1").textContent(), "Not Found") | |
| expect(await isScrolledToTop(page), "page is scrolled down").toEqual(false) | |
| await page.click("#link-404") | |
| await expect(page.locator("h1")).toHaveText("Not Found") | |
| const newScrollY = await page.evaluate(() => window.scrollY) | |
| expect(newScrollY, "scroll position should be preserved after 404 error").toEqual(scrollY) |
src/core/drive/visit.js
Outdated
| const currentSnapshot = this.view.snapshot | ||
| const preserveScroll = currentSnapshot.refreshScroll === "preserve" | ||
| const scrollPosition = preserveScroll ? { x: window.scrollX, y: window.scrollY } : null | ||
|
|
||
| await this.view.renderError(PageSnapshot.fromHTMLString(responseHTML), this) | ||
|
|
||
| // Handle scroll position after error rendering | ||
| if (preserveScroll && scrollPosition) { | ||
| this.view.scrollToPosition(scrollPosition) | ||
| } else if (!this.scrollToAnchor()) { | ||
| this.view.scrollToTop() | ||
| } | ||
| this.scrolled = true | ||
|
|
There was a problem hiding this comment.
Rather than adding these lines to account for the scrolling behavior, have you explored invoking this.performScroll()?
| const currentSnapshot = this.view.snapshot | |
| const preserveScroll = currentSnapshot.refreshScroll === "preserve" | |
| const scrollPosition = preserveScroll ? { x: window.scrollX, y: window.scrollY } : null | |
| await this.view.renderError(PageSnapshot.fromHTMLString(responseHTML), this) | |
| // Handle scroll position after error rendering | |
| if (preserveScroll && scrollPosition) { | |
| this.view.scrollToPosition(scrollPosition) | |
| } else if (!this.scrollToAnchor()) { | |
| this.view.scrollToTop() | |
| } | |
| this.scrolled = true | |
| await this.view.renderError(PageSnapshot.fromHTMLString(responseHTML), this) | |
| this.performScroll() | |
It would mirror this branch's counterpart (the this.renderPageSnapshot(snapshot, false))
Lines 423 to 428 in fea2ebd
Update turbo-fixed.js with the latest build that includes the performScroll() refactoring based on code review feedback. Changes: - Update public/turbo-fixed.js to latest build - Update README.md to reflect current fix implementation - Clarify that the fix uses performScroll() for consistency - Add code example showing before/after comparison The fix ensures scroll behavior is handled consistently for all response types (2XX and non-2XX) by calling performScroll() after rendering error responses. Related: hotwired/turbo#1449, hotwired/turbo#1462
|
Thank you for the thorough review! I've addressed all the feedback points. Here's a summary of the changes: Changes Made1. Refactored scroll handling in visit.jsAs suggested, I've replaced the manual scroll position handling with a call to the existing Before: const currentSnapshot = this.view.snapshot
const preserveScroll = currentSnapshot.refreshScroll === "preserve"
const scrollPosition = preserveScroll ? { x: window.scrollX, y: window.scrollY } : null
await this.view.renderError(PageSnapshot.fromHTMLString(responseHTML), this)
if (preserveScroll && scrollPosition) {
this.view.scrollToPosition(scrollPosition)
} else if (!this.scrollToAnchor()) {
this.view.scrollToTop()
}
this.scrolled = trueAfter: await this.view.renderError(PageSnapshot.fromHTMLString(responseHTML), this)
this.performScroll()2. Updated test assertions to use expect styleConverted all test assertions from
Example: // Before
assert.notOk(await isScrolledToTop(page), "page is scrolled down")
await page.click("#link-404")
await nextBody(page)
await nextBeat()
assert.ok(await isScrolledToTop(page), "page is scrolled to the top after 404 error")
assert.equal(await page.locator("h1").textContent(), "Not Found")
// After
expect(await isScrolledToTop(page), "page is scrolled down").toEqual(false)
await page.click("#link-404")
await expect(page.locator("h1")).toHaveText("Not Found")
expect(await isScrolledToTop(page)).toEqual(true)3. Clarified test scope and namingI've updated the tests to better reflect what's actually being tested:
4. Expanded test coverage to include form submissionsWhile Issue #1449 specifically mentions "clicking any link", the fix ( Added tests in page_refresh_tests.js:
Why form submission tests were added:
Additional fixtures and infrastructure:
Response to Test Approach ConcernRegarding your question about alternative test approaches:
Thank you for raising this important point. I've considered several alternative approaches: Alternative Approaches ConsideredOption 1: Same-URL with query parameters Option 2: Dynamic fixture behavior router.get("/dynamic-fixture.html", (req, res) => {
if (req.session.visitCount > 0) {
res.status(404).sendFile('404.html')
}
})Option 3: Dedicated error page paths <a href="/nonexistent-page.html">404</a>Why I Chose the Current ApproachAfter investigation, I found the $ grep -r "/__turbo/" src/tests/fixtures/*.html | wc -l
98Examples include:
The server mounts these via ✅ Consistency with 98 existing test usages Issue #1449 Scope ClarificationThe original issue specifically describes link-based navigation:
The core problem is:
Going ForwardI'm open to implementing a more realistic test approach if you feel it's important. However, I'd suggest:
This would allow:
Does this sound reasonable? I'm happy to adjust the approach if you feel the alternative patterns would be significantly better for this specific case. Summary of Current StateFiles changed:
Tests coverage:
All tests pass on Chrome. Please let me know if you'd like any adjustments! |
|
@seanpdoyle Thank you for your patience. I now understand the issue correctly. My Previous MisunderstandingIn my previous comment, I thought updating the test names and explaining the The Real IssueYou were right - those tests in What I've Done NowRemoved:
Added:
Test CoverageAll tests now correctly test same-URL Page Refresh behavior:
Each test uses form submissions that return error responses on the same URL, correctly reproducing the scenario in #1449. Thank you for helping me understand this critical distinction! |
Add test coverage for verifying that turbo-refresh-scroll meta tag behavior works with error responses (404, 500) in addition to successful responses. - Add test fixtures for reset and preserve scroll modes - Make error page fixtures (404.html, 500.html) scrollable - Add GET /error endpoint to test server for returning error responses - Add 4 test cases covering both reset and preserve modes on errors
Honor the turbo-refresh-scroll meta tag for non-2XX status codes. Previously, scroll behavior (reset/preserve) was only applied to successful (2XX) responses. Error responses now correctly respect the scroll mode configuration. When rendering error responses, capture the current scroll mode before rendering, then apply the appropriate scroll behavior: - "preserve": restore the previous scroll position - "reset" (or default): scroll to top Fixes hotwired#1449
Replace manual scroll position handling with the existing performScroll() method to maintain consistency with the successful response handling pattern. - Remove manual scroll capture and restoration logic - Call this.performScroll() after renderError() - Leverage existing scroll behavior handling Addresses review feedback from @seanpdoyle
- Convert assertions from assert to expect() style - Remove explicit timing helpers (nextBody/nextBeat) in favor of await expect(locator).toHaveText() automatic waiting - Clarify test scope: remove turbo-refresh-scroll references from navigation tests (meta tag only applies to same-URL page refreshes) - Rename fixture: navigation_scroll_reset.html → navigation_error.html - Add form submission error tests for comprehensive coverage - Use assertPageScroll() helper in page_refresh_tests.js Test files: - Update navigation_tests.js with expect style and clarified naming - Add 422 error tests to page_refresh_tests.js Fixtures: - Create navigation_error.html (renamed, removed misleading meta tag) - Delete navigation_scroll_preserve.html (out of scope) - Make 422_morph.html scrollable - Create 422_morph_reset.html for reset mode testing - Update page_refresh.html with reset mode form Server: - Add /error endpoint for error response testing - Add /reject/morph/reset endpoint for reset mode testing Addresses review feedback from @seanpdoyle
Addresses @seanpdoyle's review feedback. The navigation tests were testing cross-page navigation instead of Page Refresh behavior. The turbo-refresh-scroll meta tag only applies to same-URL page refreshes, not navigation. Changes: - Remove 3 navigation tests that navigated to different URLs - Remove unused navigation_error.html and /__turbo/error endpoint - Add Page Refresh tests for 404/500 errors with turbo-refresh-scroll=reset - Add 404_reset.html and 500_reset.html fixtures - Add /__turbo/reject/reset endpoint and update page_refresh.html All tests now correctly test same-URL Page Refresh behavior. Related: hotwired#1449
yujiteshima
force-pushed
the
fix-1449-turbo-refresh-scroll-errors
branch
from
3f1cb51 to
423d697
Compare
2 participants
Description
Fixes #1449
The
turbo-refresh-scrollmeta tag was being ignored for non-2XX HTTP responses. This PR ensures that both"reset"and"preserve"scroll modes work correctly with error responses (404, 500, etc.), not just successful responses.Problem
Previously, scroll behavior was only applied to successful (2XX) responses. When navigating to error pages, the scroll position would remain unchanged regardless of the
turbo-refresh-scrollsetting:Before:
loadResponse()→renderPageSnapshot()→performScroll()✓loadResponse()→renderError()→ (no scroll handling) ✗This created inconsistent behavior where error pages would leave users scrolled down, potentially hiding the error message.
Solution
Added scroll handling to the error rendering path in
src/core/drive/visit.js:turbo-refresh-scrollsetting from the current snapshot before rendering"preserve": Restore the previous scroll position"reset"(or default): Scroll to topAfter:
loadResponse()→renderError()→ apply scroll behavior ✓Changes
src/core/drive/visit.js: Add scroll handling for error responsessrc/tests/fixtures/navigation_scroll_reset.html: New test fixture for reset modesrc/tests/fixtures/navigation_scroll_preserve.html: New test fixture for preserve modesrc/tests/fixtures/404.html: Make scrollable for testingsrc/tests/fixtures/500.html: Make scrollable for testingsrc/tests/server.mjs: AddGET /errorendpoint for test casessrc/tests/functional/navigation_tests.js: Add 4 comprehensive test casesTest Coverage
Added tests covering:
All tests pass in both Chrome and Firefox.
Related