1239349 - Implement webNavigation.onHistoryStateUpdated

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect, P2)

Description

[Luca Greco [:rpl] [:luca] [:lgreco]]

https://developer.chrome.com/extensions/webNavigation#event-onHistoryStateUpdated

This event should be fired every time the history API is used to modify the state of a frame
(e.g. using "history.pushState()"), and it can be fired at any time after "onDOMContentLoaded".

Comment 1

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: MozReview Request: Bug 1239349 - Implement webNavigation.onHistoryStateUpdated. r=kmag

Review commit: https://reviewboard.mozilla.org/r/34215/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/34215/

Comment 2

[Luca Greco [:rpl] [:luca] [:lgreco]]

https://reviewboard.mozilla.org/r/34213/#review30899

::: toolkit/modules/addons/WebNavigation.jsm:104
(Diff revision 1)
> +    let isHistoryPushState = (data) => (data.loadType & Ci.nsIDocShell.LOAD_CMD_PUSHSTATE);

|history.pushState| can be easily recognized by checking the |loadType| using the |Ci.nsIDocShell.LOAD_CMD_PUSHSTATE| constant.

::: toolkit/modules/addons/WebNavigation.jsm:105
(Diff revision 1)
> +    let isHistoryPopState = (data) => (data.loadType & Ci.nsIDocShell.LOAD_CMD_HISTORY) &&

Things are bit more tricky for the pop state:

checking |loadType| using |Ci.nsIDocShell.LOAD_CMD_HISTORY| and |flags| using |Ci.nsIWebProgressListener.LOCATION_CHANGE_SAME_DOCUMENT| seems to be coherent with how |window.onpopstate| works on Firefox.

Unfortunately, the above "popState detection" strategy is slightly different from how Chromium browsers behave:

Chrome send an onReferenceFragmentUpdated event when a window goes back in the history entries to an entry which was created by an hash change

On the contrary, using the above "popState detection" strategy, Firefox will send onHistoryStateUpdate for any navigation event which is related to an history back/forward which doesn't change the document (basically history push/replace/popState and location hash changes which are already in the history, but new location hash changes will be still detected and sent as onReferenceFragmentUpdated events).

Comment 3

[Kris Maglione [:kmag]]

Comment on attachment 8717563 [details]
MozReview Request: Bug 1239349 - Implement webNavigation.onHistoryStateUpdated. r=kmag

https://reviewboard.mozilla.org/r/34215/#review31083

::: toolkit/modules/addons/WebNavigation.jsm:106
(Diff revision 1)
> +          isSameDocument(data);

Can we just have these be variables rather than functions?

Comment 4

[Kris Maglione [:kmag]]

(In reply to Luca Greco [:rpl] from comment #2)
> Unfortunately, the above "popState detection" strategy is slightly different
> from how Chromium browsers behave:
> 
> Chrome send an onReferenceFragmentUpdated event when a window goes back in
> the history entries to an entry which was created by an hash change
> 
> On the contrary, using the above "popState detection" strategy, Firefox will
> send onHistoryStateUpdate for any navigation event which is related to an
> history back/forward which doesn't change the document (basically history
> push/replace/popState and location hash changes which are already in the
> history, but new location hash changes will be still detected and sent as
> onReferenceFragmentUpdated events).

When we're navigating within the same page, the docshell checks if the only
change to the URL is in the hash, and if so sends a hashchange event. We
should probably do the same thing here to decide whether to send
onReferenceFragmentUpdated events.

Does chrome also send onHistoryStateUpdate events for hash changes?

Comment 5

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8717563 [details]
MozReview Request: Bug 1239349 - Implement webNavigation.onHistoryStateUpdated. r=kmag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34215/diff/1-2/

Comment 6

[Kris Maglione [:kmag]]

Comment on attachment 8717563 [details]
MozReview Request: Bug 1239349 - Implement webNavigation.onHistoryStateUpdated. r=kmag

https://reviewboard.mozilla.org/r/34215/#review31483

::: toolkit/modules/addons/WebNavigationContent.js:77
(Diff revisions 1 - 2)
> +      let location = webProgress.DOMWindow.location;

Please use the `location` argument rather than going through the DOM window here. Also, `pathname` isn't the right property here, since it won't change when things like the query parameters do.

::: toolkit/modules/addons/WebNavigationContent.js:79
(Diff revisions 1 - 2)
> +      let hashChanged = !previousLocation ? false : (previousLocation.hash != location.hash);

`hashChanged = !(previousLocation && previousLocation.hash == location.hash);

::: toolkit/modules/addons/WebNavigationFrames.jsm:101
(Diff revision 2)
> +var docShellPreviousLocationMap = new WeakMap();

It's unfortunate that we have to do this... Ideally we should update the docshell to add an extra flag for hash-only changes, or something like that. But I guess this is fine for now.

::: toolkit/modules/addons/WebNavigationFrames.jsm:103
(Diff revision 2)
> +function updatePreviousLocationInfo(docShell) {

This function isn't shareable. It depends on having only one call site, so let's move the map to the progress listener, and just update it there rather than adding a helper function.

::: toolkit/modules/addons/WebNavigationFrames.jsm:106
(Diff revision 2)
> +                    .DOMWindow;

We can just use the `currentURI` of the docshell here. `docShell.QueryInterface(Ci.nsIWebNavigation).currentURI`

Comment 7

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8717563 [details]
MozReview Request: Bug 1239349 - Implement webNavigation.onHistoryStateUpdated. r=kmag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34215/diff/2-3/

Comment 8

[Luca Greco [:rpl] [:luca] [:lgreco]]

https://reviewboard.mozilla.org/r/34215/#review31483

> Please use the `location` argument rather than going through the DOM window here. Also, `pathname` isn't the right property here, since it won't change when things like the query parameters do.

It shouldn't be a problem because a change in the query parameter (or location.search) generates a new document load and it should not be wrongly detected as a hash change or an history.pushState/replaceState, because it would be filtered out by the isSameDocument check.

But using the URI instead of the location property is ok too and in this last revision of the patch I changed it to use the URI (and to include the query parameters in the tracked info)

> It's unfortunate that we have to do this... Ideally we should update the docshell to add an extra flag for hash-only changes, or something like that. But I guess this is fine for now.

It is very unfortunate, and I felt tempted to restrict it to the history.pushState tracking in a first place, but I'm not convinced that, in this case, "half of the feature" is still useful (and correct or at least "not misleading").

My plan is to use this workaround (which seems to be simple and reasonable enough) until a platform change is ready and we can remove it in favor of directly checking an new flag.

Comment 9

[Kris Maglione [:kmag]]

(In reply to Luca Greco [:rpl] from comment #8)
> It shouldn't be a problem because a change in the query parameter (or
> location.search) generates a new document load and it should not be wrongly
> detected as a hash change or an history.pushState/replaceState, because it
> would be filtered out by the isSameDocument check.

Not when using `history.pushState`. That can change the query parameter
without navigating away from the document. Changing '/foo#bar' to
'/foo?baz=bar' in that case would be detected as a hash change if you're only
checking the pathname.

Comment 10

[Luca Greco [:rpl] [:luca] [:lgreco]]

(In reply to Kris Maglione [:kmag] from comment #9)
> Not when using `history.pushState`. That can change the query parameter
> without navigating away from the document. Changing '/foo#bar' to
> '/foo?baz=bar' in that case would be detected as a hash change if you're only
> checking the pathname.

That's true! 

This version should not be affected, because the query parameters are included in |currentURI.specIgnoringRef| (which is what I'm using to retrieve the path)

The hash is now retrieved from |currentURI.ref.

Comment 11

[Kris Maglione [:kmag]]

Comment on attachment 8717563 [details]
MozReview Request: Bug 1239349 - Implement webNavigation.onHistoryStateUpdated. r=kmag

https://reviewboard.mozilla.org/r/34215/#review31565

::: toolkit/modules/addons/WebNavigationContent.js:54
(Diff revisions 2 - 3)
> +  updatePreviousLocationInfo(docShell) {

Please just inline this function. It takes a lot more effort to track it down and figure out what it does than it would to just read the code in place.

And since it depends on having exactly one call site, making it a shared function is just asking for trouble.

::: toolkit/modules/addons/WebNavigationContent.js:57
(Diff revisions 2 - 3)
> +                      .DOMWindow;

`let win = docShell.QueryInterface(Ci.nsIInterfaceRequestor).getInterface(Ci.nsIDOMWindow)`

::: toolkit/modules/addons/WebNavigationContent.js:69
(Diff revisions 2 - 3)
> +    this.docShellPreviousLocationMap.set(win, updatedInfo);

Let's just store the URI. You can check the path equality using `equalsIgnoringRef`.

Comment 12

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8717563 [details]
MozReview Request: Bug 1239349 - Implement webNavigation.onHistoryStateUpdated. r=kmag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34215/diff/3-4/

Comment 13

[Luca Greco [:rpl] [:luca] [:lgreco]]

https://reviewboard.mozilla.org/r/34215/#review31565

> Please just inline this function. It takes a lot more effort to track it down and figure out what it does than it would to just read the code in place.
> 
> And since it depends on having exactly one call site, making it a shared function is just asking for trouble.

it's defintely better to "do not ask for trouble"! ;-)

This helper method has been inlined in the last version of this patch (and by tracking the URI instead of pre-extracting the URI pieces it looks even nicer)

> Let's just store the URI. You can check the path equality using `equalsIgnoringRef`.

Thanks Kris, the nsIURI's equalsExceptRef was definitely useful.

Keeping track of the entire URI object instead of pre-extracting the URI pieces makes the code nicer, thanks again for pointing out it.

Comment 14

[Kris Maglione [:kmag]]

Comment on attachment 8717563 [details]
MozReview Request: Bug 1239349 - Implement webNavigation.onHistoryStateUpdated. r=kmag

https://reviewboard.mozilla.org/r/34215/#review31771

Looks good. Thanks.

Can you just add one more test that if we replace a hash with a query parameter we get the right event?

Comment 15

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8717563 [details]
MozReview Request: Bug 1239349 - Implement webNavigation.onHistoryStateUpdated. r=kmag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34215/diff/4-5/

Comment 16

[Luca Greco [:rpl] [:luca] [:lgreco]]

last patch revision pushed to try:

- https://treeherder.mozilla.org/#/jobs?repo=try&revision=ab254d321b7f

Comment 17

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/f3dcf982a76f

Comment 18

[Carsten Book [:Tomcat]]

backed out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=7514363&repo=fx-team

Comment 19

[Luca Greco [:rpl] [:luca] [:lgreco]]

(In reply to Carsten Book [:Tomcat] from comment #18)
> backed out for test failures like
> https://treeherder.mozilla.org/logviewer.html#?job_id=7514363&repo=fx-team

They seems to be related to changes in the eslint config which happened after my last eslint run.

I'm going to fix it and push to mozreview the new eslint-proof version.

Comment 20

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/fx-team/rev/7a39bf96ab31

Comment 21

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8717563 [details]
MozReview Request: Bug 1239349 - Implement webNavigation.onHistoryStateUpdated. r=kmag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34215/diff/5-6/

Comment 22

[Luca Greco [:rpl] [:luca] [:lgreco]]

Pushed updated patch to fix eslint errors.

Re-applied checkin-needed keyword.

Comment 23

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/9b5fd0f8dbf1

Comment 24

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/9b5fd0f8dbf1

Comment 25

[Will Bamberg [:wbamberg]]

-> https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/WebNavigation/onHistoryStateUpdated