History needs to visit with a restoration identifier to restore scroll position #295
Conversation
…l position We don't need to go through proposeVisit, because the visit already passed that check to get into the history.
| if (this.enabled) { | ||
| this.navigator.proposeVisit(location, { action: "restore", historyChanged: true }) |
There was a problem hiding this comment.
I wonder if calling proposeVisit might be needed for the native adapters?
I think this issue was previously solved by passing the restorationIdentifier along in the options to proposeVisit.
There was a problem hiding this comment.
I checked that with the Android adapter, and it did not seem to be the case: https://github.com/hotwired/turbo-android/blob/556cfae61408811fe488bd877416b27e53cb35ca/turbo/src/main/assets/js/turbo_bridge.js. But will test before rolling further. We also have a test that's explicitly asserting that we're not going through proposeVisit (since that implies the right to cancel): https://github.com/hotwired/turbo/blob/main/src/tests/functional/visit_tests.ts#L74-L81
There was a problem hiding this comment.
I'm not sure it'd work just to pass the restorationIdentifier as part of the option set either. Where this problem was triggered was in BrowserAdapter#visitPropoposedToLocation, which just always generates a new uuid for the restorationIdentifier: https://github.com/hotwired/turbo/blob/main/src/core/native/browser_adapter.ts#L18. And passing the restorationIdentifier all the way from history popped meant passing it through like 3-4 times.
There was a problem hiding this comment.
Ahh, I see that this other PR added restorationIdentifier to the VisitOptions. Yeah, that would also be a way to go.
There was a problem hiding this comment.
Verified that historyPoppedToLocationWithRestorationIdentifier is not used by the native adapters. So fine to proceed with this simple fix to the problem as it appears in browsers.
We don't need to go through proposeVisit, because the visit already passed that check to get into the history.