Conversation
🦋 Changeset detectedLatest commit: e00bc95 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
| // There's not a good way to record scroll position before a back button. | ||
| // So the way we do it is by listening to scroll and just continuously recording it. | ||
| addEventListener('scroll', () => { | ||
| const state: State | undefined = history.state; | ||
| if(state) { | ||
| state.scrollY = scrollY; | ||
| history.replaceState(state, ''); | ||
| } | ||
| }, { passive: true }); |
There was a problem hiding this comment.
Even though with passive, I wonder if there's a perf penalty calling replaceState repeatedly. Maybe it can use a throttle.
Could keep an eye on this and leave it as is too now though.
There was a problem hiding this comment.
I agree with Bjorn, but I feel more strongly on this one. I think we have to add a bouncing mechanism.
There was a problem hiding this comment.
If it were a regular Map would you still feel this way? I would assume that replaceState is just a map under the hood. I don't want to add extra code, increasing the bundle size, unless we know that we need to.
There was a problem hiding this comment.
Given that this is an experimental feature I think it's ok to be less safe in this case. If it is a problem we'll hear about it and fix before it becomes stable.
There was a problem hiding this comment.
Did some investigation and Safari considers calling replaceState too many times a security issue. I think I understand why; malicious sites could call pushState a lot to prevent you from backing out of the site. But I don't see why that would be the case for replaceState.
In any event, will need to look for another solution here.
There was a problem hiding this comment.
I remember very vaguely hitting this issue, and was able to fix it with a throttle helper with 300ms timeout (i think). This SO post seems to explain it: https://stackoverflow.com/questions/38419867/os-x-ios-safari-history-replacestate-limit-throws-securityerror-dom-exception
There was a problem hiding this comment.
Yeah, that's what I wound up doing. I did 500ms to be safe, but can bump it down. Otherwise ready for another review.
|
This has a 2 bugs: In prod - back animation no longer plays
In dev (and probably in prod once the previous bug is fixed) -
|
|
From what I can see in prod, if I follow a link from the footer, the page opens at the footer instead of the top. Is this expected? Can it be overriden? |
|
@millette I do not see that, but going back and forth using navigation does seem to break the page... sometimes... this is worrying. |
|
For example, if you click |
Changes
history.statescrollToscrollToTesting
Docs
N/A, transparent to developer