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

popstate/hashchange dispatching doesn't match what browsers do. #1792

Closed
smaug---- opened this issue Sep 16, 2016 · 12 comments · Fixed by #7815
Closed

popstate/hashchange dispatching doesn't match what browsers do. #1792

smaug---- opened this issue Sep 16, 2016 · 12 comments · Fixed by #7815
Labels
interop Implementations are not interoperable with each other topic: events topic: history

Comments

@smaug----
Copy link

When doing fragment navigation Gecko and Blink dispatch popstate sync but hashchange async. Per current spec both events should fire async using same task.

@freesamael
Copy link

@annevk I'm willing to push this forward. I found that Webkit raises compatibility concerns and decided not to implement popstate async:
https://bugs.webkit.org/show_bug.cgi?id=153686#c2

Chrome seems to keep the same behavior as Webkit at this part - sync popstate & async hashchange:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp?type=cs&sq=package:chromium&l=405-415

Could you loop some people from webkit / chrome / edge to see if they have some comments? I think if major browsers are all behave the same, we should update the spec accordingly.

@annevk
Copy link
Member

annevk commented Sep 7, 2017

The proposal is to align popstate/hashchange with Chrome and Safari and fire the first directly and the second from a task. Anyone any comments on that?

cc @cdumez @majido @foolip @hober @travisleithead

@foolip
Copy link
Member

foolip commented Sep 7, 2017

@tkent-google, can you figure out who the Blink contact for this would be?

@tkent-google
Copy link
Contributor

@tkent-google, can you figure out who the Blink contact for this would be?

I don't think so. Maybe japhet@chromium or kinuko@chromium knows it.

@foolip
Copy link
Member

foolip commented Sep 19, 2017

@kinu, do you know? (I can't find japhet on GitHub.)

@kinu
Copy link

kinu commented Sep 20, 2017

@natechapin you probably know more about this?

@domenic domenic added the interop Implementations are not interoperable with each other label Jun 6, 2018
@domenic
Copy link
Member

domenic commented Nov 19, 2021

Fun fact: Chrome does not synchronously fire popstate for fragment navigations. It waits until "document close", i.e. load event fire time. You can see this by opening your console on https://boom-bath.glitch.me/popstate-location-href.html and observing different results in Firefox vs. Chrome.

Combined with the fact that we queue hashchange events (per the original post in this issue), this bit us recently in https://bugs.chromium.org/p/chromium/issues/detail?id=1254926. The basic situation is

<!doctype html>
<script>
window.onload = () => console.log("load");
window.onpopstate = () => console.log("popstate");
window.onhashchange = () => console.log("hashchange");
location.hash = "#1";
</script>

Chrome is trying to introduce a HTML parser that yields to the event loop more often before the load event, and doing so causes the ordering of these events to flip:

  • Without yielding (current Chrome), we queue hashchange, fire to the load event, fire popstate, then yield and the queued hashchange fires. So: load, popstate, hashchange.
  • With yielding, we queue hashchange, yield, fire hashchange, fire the load event, fire popstate. So: hashchange, load, popstate.

I'm not sure what the right solution is here, but I don't like how this sort of stuff makes the yield points observable. I suspect the blame lies with queuing hashchange since that makes it run at the next yield point. Maybe we can hold both hashchange and popstate until the load event, and either queue or synchronously fire hashchange then.

@domenic domenic added the agenda+ To be discussed at a triage meeting label Nov 23, 2021
@past past removed the agenda+ To be discussed at a triage meeting label Dec 3, 2021
@domenic
Copy link
Member

domenic commented Jan 14, 2022

I have written some tests at web-platform-tests/wpt#32392. I have not yet found a Safari computer to run them against.

The problem cases really seem to be about changing location.hash. Recall that pushState/replaceState don't fire popstate. Traversal seems to work fine since generally everything is async; the popstate-then-hashchange order seems to be preserved with no difficulties.

Notably Chrome's behavior differs between Canary and stable. And per the above it might depend on parser yield points and thus not be deterministic. I think this gives us more freedom to match Firefox-like behavior.

However I think Firefox might still do task-queuing, which I worry about because it could cause the non-determinism Chrome is currently exhibiting.

So this might be one proposal:

  • Fire popstate sync
    • This is like Firefox, AFAICT
    • Chrome usually fires it sync but will delay it until after the load event if the load event hasn't fired yet. In that case if there are multiple state changes only one will generate an event in Chrome. (Seems bad.)
  • Before the load event fires, if the hash changes: queue up hashchange events and fire them right before load
    • This requires a separate queue of hash changes as you can change the hash multiple times before the load event fires
    • Both Chrome and Firefox appear to fire hashchange before the load event (but not synchronously)

I will investigate Safari behavior next week and see if this plan might work out.

A less-ambituous plan is to avoid the queue of hash changes and allow that level of parser-based nondeterminism. That would still preserve the popstate-then-hashchange order, as well as the every-change-generates-both-events invariant. It would mean hashchange could be either before or after the load event though.

I do worry that firing popstate sync (even if the change happens before load) might be a compat risk for Chrome. So if @smaug---- can confirm that Firefox in all fragment navigation cases fires popstate sync, and that this hasn't been a source of compat bugs, that would help alleviate my concerns.

@smaug----
Copy link
Author

smaug---- commented Feb 3, 2022

Firefox queues hashchange, but not popstate, as mentioned initially
https://searchfox.org/mozilla-central/rev/8b46752d1e583b2f817c451f93ba515fb865554d/docshell/base/nsDocShell.cpp#9154,9164

What causes Chrome fires popstate after load? Does it happen always or only if .hash was modified just before load?

I don't understand what "separate queue". One just queues a task to fire hashchange.

And yes, in FF, location.hash is synchronous, so popstate is sync.
The only relevant bug report about this seems to be for discussing whether to follow the model spec (at least at some point) had to fire popstate async.

@domenic
Copy link
Member

domenic commented Apr 12, 2022

@natechapin and I are planning to try aligning Blink and the spec with Gecko here. Since you have not experienced any compat issues we are encouraged that we try that without compat problems.

@natechapin's analysis of the compat risks for Blink changing this are that it reduces to the following scenarios:

  • If you pushState() before the load event, and you somehow depend on history.state not being equal to e.state inside the popstate event handler.
  • The long tail of absurd ordering dependencies that a website might take on between popstate/hashchange/load.

Both of these are not too risky because (a) Firefox has gotten away with it for many years without problems; (b) ordering is already unreliable and has even changed recently in Chromium.

I will work on a spec PR now where we can see in more detail. And @natechapin and I will collaborate on web platform tests.


Now for a history lesson!! Why is Chromium delaying popstate until after load? Well, apparently the spec used to ask us to do so! See this old working draft.

This changed in 4e57ced#diff-42692259bbea922a55e8e0f8a8b7860df9b59e29b3f955bcfd4d0a3ca852cc01L60686 , as a result of discussions in https://www.w3.org/Bugs/Public/show_bug.cgi?id=11468 and https://www.w3.org/Bugs/Public/show_bug.cgi?id=12277 . The change in turn is based on something Firefox very intentionally led the way on, and even blogged about: https://hacks.mozilla.org/2011/03/history-api-changes-in-firefox-4/ . As far as we can tell, other browsers implemented items 1 and 2 in that blog post, but not 3. And the spec got updated in a weird way that didn't really match any browser.

domenic added a commit that referenced this issue Apr 12, 2022
Closes #1792 by aligning with Gecko. The alternative model involved delaying popstate until after the load event, which is what WebKit and Blink do. But the Gecko model is simpler and has fewer edge cases around multiple state changes and such.
@miketaylr
Copy link
Member

Both of these are not too risky because (a) Firefox has gotten away with it for many years without problems

There's definitely a pile of "back button works in Chrome but not Firefox" bugs out there though, see https://bugzilla.mozilla.org/show_bug.cgi?id=1643554 and https://github.com/webcompat/web-bugs/search?q=%22back+button%22+return&type=issues (maybe under-reporting, but https://github.com/webcompat/web-bugs/search?q=%22back+button%22&type=issues is over-reporting).

cc @wisniewskit, who has diagnosed many of these bugs.

@domenic
Copy link
Member

domenic commented Apr 18, 2022

There's definitely a pile of "back button works in Chrome but not Firefox" bugs out there though,

I don't think any of those are related to popstate/hashchange ordering, though. Instead I believe most are due to inconsistent mapping of the back button to the joint session history, per WICG/interventions#21.

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Apr 26, 2022
Interop discussion: whatwg/html#1792
Intent to ship: https://groups.google.com/a/chromium.org/g/blink-dev/c/HXRHWirKarU

Bug: 1254926
Change-Id: I7e41ab603a15a14bf9df5000edca2724766a20e9
aarongable pushed a commit to chromium/chromium that referenced this issue Apr 27, 2022
Interop discussion: whatwg/html#1792
Intent to ship: https://groups.google.com/a/chromium.org/g/blink-dev/c/HXRHWirKarU

Bug: 1254926
Change-Id: I7e41ab603a15a14bf9df5000edca2724766a20e9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3580022
Reviewed-by: Domenic Denicola <domenic@chromium.org>
Commit-Queue: Nate Chapin <japhet@chromium.org>
Cr-Commit-Position: refs/heads/main@{#996748}
domenic added a commit that referenced this issue Apr 27, 2022
Closes #1792 by aligning with Gecko. The alternative model involved delaying popstate until after the load event, which is what WebKit and Blink do. But the Gecko model is simpler, more deterministic (since the load event depends on network timing), and has fewer edge cases around multiple consecutive state changes.
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Apr 27, 2022
Interop discussion: whatwg/html#1792
Intent to ship: https://groups.google.com/a/chromium.org/g/blink-dev/c/HXRHWirKarU

Bug: 1254926
Change-Id: I7e41ab603a15a14bf9df5000edca2724766a20e9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3580022
Reviewed-by: Domenic Denicola <domenic@chromium.org>
Commit-Queue: Nate Chapin <japhet@chromium.org>
Cr-Commit-Position: refs/heads/main@{#996748}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Apr 27, 2022
Interop discussion: whatwg/html#1792
Intent to ship: https://groups.google.com/a/chromium.org/g/blink-dev/c/HXRHWirKarU

Bug: 1254926
Change-Id: I7e41ab603a15a14bf9df5000edca2724766a20e9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3580022
Reviewed-by: Domenic Denicola <domenic@chromium.org>
Commit-Queue: Nate Chapin <japhet@chromium.org>
Cr-Commit-Position: refs/heads/main@{#996748}
DanielRyanSmith pushed a commit to DanielRyanSmith/wpt that referenced this issue Apr 27, 2022
Interop discussion: whatwg/html#1792
Intent to ship: https://groups.google.com/a/chromium.org/g/blink-dev/c/HXRHWirKarU

Bug: 1254926
Change-Id: I7e41ab603a15a14bf9df5000edca2724766a20e9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3580022
Reviewed-by: Domenic Denicola <domenic@chromium.org>
Commit-Queue: Nate Chapin <japhet@chromium.org>
Cr-Commit-Position: refs/heads/main@{#996748}
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue May 16, 2022
… a=testonly

Automatic update from web-platform-tests
Make popstate always fire synchronously

Interop discussion: whatwg/html#1792
Intent to ship: https://groups.google.com/a/chromium.org/g/blink-dev/c/HXRHWirKarU

Bug: 1254926
Change-Id: I7e41ab603a15a14bf9df5000edca2724766a20e9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3580022
Reviewed-by: Domenic Denicola <domenic@chromium.org>
Commit-Queue: Nate Chapin <japhet@chromium.org>
Cr-Commit-Position: refs/heads/main@{#996748}

--

wpt-commits: 681dc9c986220956b3ffd0936963211603a0ba06
wpt-pr: 33746
jamienicol pushed a commit to jamienicol/gecko that referenced this issue May 25, 2022
… a=testonly

Automatic update from web-platform-tests
Make popstate always fire synchronously

Interop discussion: whatwg/html#1792
Intent to ship: https://groups.google.com/a/chromium.org/g/blink-dev/c/HXRHWirKarU

Bug: 1254926
Change-Id: I7e41ab603a15a14bf9df5000edca2724766a20e9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3580022
Reviewed-by: Domenic Denicola <domenic@chromium.org>
Commit-Queue: Nate Chapin <japhet@chromium.org>
Cr-Commit-Position: refs/heads/main@{#996748}

--

wpt-commits: 681dc9c986220956b3ffd0936963211603a0ba06
wpt-pr: 33746
mfreed7 pushed a commit to mfreed7/html that referenced this issue Jun 3, 2022
Closes whatwg#1792 by aligning with Gecko. The alternative model involved delaying popstate until after the load event, which is what WebKit and Blink do. But the Gecko model is simpler, more deterministic (since the load event depends on network timing), and has fewer edge cases around multiple consecutive state changes.
webkit-early-warning-system pushed a commit to cdumez/WebKit that referenced this issue Sep 15, 2022
https://bugs.webkit.org/show_bug.cgi?id=245153

Reviewed by Brent Fulgham.

PopState event should be fired synchronously, even before the load event:
- whatwg/html#1792

We used to delay PopState events until the load event has fired but this
doesn't match other Blink or Gecko.

* LayoutTests/imported/w3c/web-platform-tests/html/browsers/browsing-the-web/history-traversal/event-order/before-load-hash-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/html/browsers/browsing-the-web/history-traversal/event-order/before-load-hash-twice-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/html/browsers/browsing-the-web/history-traversal/event-order/pushState-inside-popstate-expected.txt:
Rebaseline WPT tests that are now passing or failing a little further. The ones that are still failing and due to the fact that
we fire the load event synchronously instead of queuing a task. As a result, it may fire before hashchange events that were
scheduled before the load has completed. I plan to look into this in a follow-up.

* Source/WebCore/dom/Document.cpp:
(WebCore::Document::implicitClose):
(WebCore::Document::statePopped):
* Source/WebCore/dom/Document.h:

Canonical link: https://commits.webkit.org/254519@main
aperezdc pushed a commit to WebKit/WebKit that referenced this issue Sep 21, 2022
…n before the load event https://bugs.webkit.org/show_bug.cgi?id=245153

Reviewed by Brent Fulgham.

PopState event should be fired synchronously, even before the load event:
- whatwg/html#1792

We used to delay PopState events until the load event has fired but this
doesn't match other Blink or Gecko.

* LayoutTests/imported/w3c/web-platform-tests/html/browsers/browsing-the-web/history-traversal/event-order/before-load-hash-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/html/browsers/browsing-the-web/history-traversal/event-order/before-load-hash-twice-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/html/browsers/browsing-the-web/history-traversal/event-order/pushState-inside-popstate-expected.txt:
Rebaseline WPT tests that are now passing or failing a little further. The ones that are still failing and due to the fact that
we fire the load event synchronously instead of queuing a task. As a result, it may fire before hashchange events that were
scheduled before the load has completed. I plan to look into this in a follow-up.

* Source/WebCore/dom/Document.cpp:
(WebCore::Document::implicitClose):
(WebCore::Document::statePopped):
* Source/WebCore/dom/Document.h:

Canonical link: https://commits.webkit.org/254519@main

(cherry picked from commit 9497f1b)
mjfroman pushed a commit to mjfroman/moz-libwebrtc-third-party that referenced this issue Oct 14, 2022
Interop discussion: whatwg/html#1792
Intent to ship: https://groups.google.com/a/chromium.org/g/blink-dev/c/HXRHWirKarU

Bug: 1254926
Change-Id: I7e41ab603a15a14bf9df5000edca2724766a20e9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3580022
Reviewed-by: Domenic Denicola <domenic@chromium.org>
Commit-Queue: Nate Chapin <japhet@chromium.org>
Cr-Commit-Position: refs/heads/main@{#996748}
NOKEYCHECK=True
GitOrigin-RevId: 425a4f723fdc2b4118713b0aed8203621129f01a
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
interop Implementations are not interoperable with each other topic: events topic: history
Development

Successfully merging a pull request may close this issue.

9 participants