Fire the popstate event asynchronously

NEW
Unassigned

Status

()

Core
Document Navigation
P3
normal
2 years ago
4 months ago

People

(Reporter: khuey, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(URL)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(4 attachments, 1 obsolete attachment)

At http://searchfox.org/mozilla-central/rev/970569ad57ac4436ff31aa2ac63f38ed3ee2932d/docshell/base/nsDocShell.cpp#10308 we synchronously fire popstate during a navigation.  The spec does not appear to require this.  All history traversals are either done off of an event or have the non-blocking events flag set, in which case the popstate event is going to be dispatched asynchronously.  And in my simple testcase, Chrome dispatches the event asynchronously: http://people.mozilla.org/~khuey/popstate-test/container.html

I think we can do what FireAsyncHashChange does here without any problem.

Comment 1

2 years ago
This part of the spec changed couple of times, IIRC.

One thing to remember is that we must do scrolling after firing popstate.
That is a recent change from bug 1186774.
(Note that it would be ok, for my purposes, to do this only for cross origin frames)
Priority: -- → P3
Created attachment 8778367 [details]
container.html

I'll attach the two files in khuey's directory here in case his people account goes pop (hey-o). You probably have to download locally for them to do whatever they are supposed to do.
This doesn't sound too tricky, so I'll take a crack at it. I need to read up on session history first, though.
Assignee: nobody → continuation
I looked through the spec. 7.8.10 History traversal says that this can be done with a "non-blocking events flag" set. If that flag is set, then the hash change and popstate events are to be done async. Otherwise, they are supposed to be done sync. There seem to be two cases for history traversal: "Navigating across documents" and "Navigating to a fragment". Only the latter mentions having the flag set, which I guess implies it is not set for the former?

Of course, our implementation does something else entirely: AFAICT hash change is always async, and popstate is always sync, so maybe I'm not reading the spec correctly.

Bug 515190 made hashchange sync, but that broke Domino's pizza website (bug 615061) so it was reverted. This change and backout corresponded to some spec changes, so I'll try to figure out how the spec got to the current state which seems to imply sync hashchange under some conditions.
Comment hidden (mozreview-request)
I had to change docshell/test/navigation/file_scrollRestoration.html to make it pass. I had to basically split up case 6 so that after each navigation we return and wait for a popstate event, like the current state waits for pageshow. Does that sound reasonable, Olli?  The test for bug 1186774 passes. (Don't look too closely at the C++, as that part of the patch is still hacky.)
Flags: needinfo?(bugs)

Comment 9

2 years ago
Sounds reasonable. Scrolling happens still after popstate, right?
Flags: needinfo?(bugs)
It would be good to test a bit what other browsers do here. At least blink supports scroll restoration stuff too.
(In reply to Olli Pettay [:smaug] from comment #9)
> Sounds reasonable. Scrolling happens still after popstate, right?

Yes. Before I implemented that part of it, the test in bug 1186774 failed on with my patch, as you would expect. With my modifications to your test, we still check all the scrolling stuff, just inside the event handler for popstate, instead of immediately after.

I should probably write some tests for how this interacts with hashchange, too, if we don't have them already.

(In reply to Olli Pettay [:smaug] from comment #10)
> It would be good to test a bit what other browsers do here. At least blink
> supports scroll restoration stuff too.

Alright. I should also write a test that checks that we are actually doing the popstate asynchronously.
It looks like Safari behaves the same as Chrome here: the popstate is not run immediately after either history.back() call. I'll try to check Edge when I get home.
FWIW, often testing webkit and blink might be enough, but their session history implementation happens to be the most broken one so testing also Edge it good.
Edge looks similar to Firefox, running the popstate synchronously. However, it seems to restore the scroll state before the popstate ran.
aha, its scroll state restoration is still based on the older spec.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Attachment #8780635 - Attachment is obsolete: true
I cleaned up my code a bit, fixed some docshell tests, and removed failures for a number of web platform tests that pass now. There's another docshell test I need to fix, and three tests in toolkit/mozapps/extensions/test/browser/ (browser_bug562797.js, browser_types.js and browser_uninstalling.js) fail. The addons manager code itself seems to involve navigation, so I guess I have to figure out how that works.
Depends on: 1298456
I manually checked that the navigation in the addons manager, as described in bug 562797 comment 0, still works with my patch.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
The second patch fixes addon manager tests. I did get one failure in one of these tests in about 15 retriggers. I'm not sure why. https://treeherder.mozilla.org/#/jobs?repo=try&revision=64b8aae384b5 I'll ask mossop for review on those once my retriggers finish.
Attachment #8786496 - Flags: review?(dtownsend)

Comment 24

2 years ago
mozreview-review
Comment on attachment 8780311 [details]
Bug 1281952 - Fire the popstate event asynchronously.

https://reviewboard.mozilla.org/r/71030/#review73538

::: docshell/base/nsDocShell.cpp:10362
(Diff revision 4)
> +            thisHolder->SetCurScrollPosEx(bx, by);
> +          };
> +        }
> +
>          if (historyNavBetweenSameDoc || doHashchange) {
> -          win->DispatchSyncPopState();
> +          win->DispatchAsyncPopState(Move(scrollCallback));

In fragment navigations popstate should happen sync per spec, and that is what blink seems to do too.
(but weirdly enough, it does async hashchange in that case. It is possible that we should always do async hashchange as we do now and just fix the spec)

::: docshell/base/nsDocShell.cpp:10372
(Diff revision 4)
>          }
>  
>          if (doHashchange) {
>            // Note that currentURI hasn't changed because it's on the
>            // stack, so we can just use it directly as the old URI.
>            win->DispatchAsyncHashchange(currentURI, aURI);

Hmm, so now we end up dispatching two separate tasks. Per spec popstate and hashchange should happen within the same task.
So could you merge the popstate and hashchange dispatching runnables and fire whatever events are needed in different cases.

::: docshell/test/navigation/file_popstate_sync.html:4
(Diff revision 4)
> +<html>
> +  <head>
> +    <script>
> +      let correctOrder = ["start-onload", "end-onload", "onpopstate", "onhashchange"];

nit, the events are "popstate" and "hashchange". not on***

::: dom/base/nsGlobalWindow.cpp:10261
(Diff revision 4)
> +  }
> +
> +private:
> +  JS::PersistentRooted<JS::Value> mStateJSValue;
> +  RefPtr<nsGlobalWindow> mWindow;
> +  mozilla::function<void ()> mRunAfter;

Ok, so you use the function stuff to avoid having to have ->SetCurScrollPosEx(bx, by); call in two places. Not sure the function makes this any easier to read, but not so it makes it harder to read it either, so fine.
Attachment #8780311 - Flags: review?(bugs) → review-
Attachment #8786496 - Flags: review?(dtownsend)
I'll just wait until the main CPP changes are fixed before I request review on the addon manager test changes, in case something ends up breaking.
(In reply to Olli Pettay [:smaug] from comment #24)
> In fragment navigations popstate should happen sync per spec
And I was so wrong with that.
Olli and I looked at the spec a bit. For fragment navigation, popstate and hashchange should be run within the same task, but async. For regular forward and backward navigation, the spec says that the navigation itself should happen async ("To traverse the history by a delta delta, the user agent must append a task to this top-level browsing context's session history traversal queue"), and then the popstate and hashchange should run sync within that same task.

Right now in Gecko, it looks like we run the navigation itself sync (whether it is fragment or otherwise), but then dispatch a new task for the hashchange. (With the popstate always run sync, as per this bug.)
Olli filed a spec bug, because Blink apparently also always does sync popstate on fragment navigation and async hashchange, like Gecko: https://github.com/whatwg/html/issues/1792
I'm not working on this. I got bogged down in trying to figure out what the spec actually says, and what people actually do, and it hasn't come up as a particular issue for Quantum DOM yet so I think it is okay to leave it alone.
Assignee: continuation → nobody
Maybe this is something that is of interest to Samael?
Flags: needinfo?(sawang)
Flags: needinfo?(htsai)
There doesn't seem to be a conclusion in the spec issue, but I'm happy to take a look and re-initiate the discussion to find out what we want to do, either at the spec or the implementation.
Assignee: nobody → sawang
Flags: needinfo?(sawang)
Flags: needinfo?(htsai)
Sorry I can no longer work on this. I still think we should just change the spec, tho.
Assignee: freesamael → nobody
You need to log in before you can comment on or make changes to this bug.