The default bug view has changed. See this FAQ.

Implement webNavigation.onHistoryStateUpdated

RESOLVED FIXED in Firefox 47

Status

()

Toolkit
WebExtensions: Untriaged
P2
normal
RESOLVED FIXED
a year ago
10 months ago

People

(Reporter: rpl, Assigned: rpl)

Tracking

(Blocks: 1 bug, {dev-doc-complete})

unspecified
mozilla47
dev-doc-complete
Points:
---
Bug Flags:
blocking-webextensions +

Firefox Tracking Flags

(firefox47 fixed)

Details

(Whiteboard: [webNavigation] triaged)

MozReview Requests

()

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

Attachments

(1 attachment)

(Assignee)

Description

a year ago
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".
(Assignee)

Updated

a year ago
Whiteboard: [webNavigation]

Updated

a year ago
Flags: blocking-webextensions+
Priority: -- → P2
Whiteboard: [webNavigation] → [webNavigation] triaged

Updated

a year ago
Iteration: --- → 47.1 - Feb 8

Updated

a year ago
Assignee: nobody → lgreco
(Assignee)

Comment 1

a year ago
Created attachment 8717563 [details]
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/
Attachment #8717563 - Flags: review?(kmaglione+bmo)
(Assignee)

Comment 2

a year ago
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).
(Assignee)

Updated

a year ago
Iteration: 47.1 - Feb 8 → 47.2 - Feb 22
(Assignee)

Updated

a year ago
Status: NEW → ASSIGNED
Attachment #8717563 - Flags: review?(kmaglione+bmo)
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?
(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?
(Assignee)

Updated

a year ago
Attachment #8717563 - Flags: review?(kmaglione+bmo)
(Assignee)

Comment 5

a year ago
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 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`
Attachment #8717563 - Flags: review?(kmaglione+bmo)
(Assignee)

Updated

a year ago
Attachment #8717563 - Flags: review?(kmaglione+bmo)
(Assignee)

Comment 7

a year ago
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/
(Assignee)

Comment 8

a year ago
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.
(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.
(Assignee)

Comment 10

a year ago
(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 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`.
Attachment #8717563 - Flags: review?(kmaglione+bmo)
(Assignee)

Updated

a year ago
Attachment #8717563 - Flags: review?(kmaglione+bmo)
(Assignee)

Comment 12

a year ago
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/
(Assignee)

Comment 13

a year ago
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 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?
Attachment #8717563 - Flags: review?(kmaglione+bmo) → review+
(Assignee)

Comment 15

a year ago
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/
(Assignee)

Comment 16

a year ago
last patch revision pushed to try:

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

Updated

a year ago
Keywords: checkin-needed
(Assignee)

Updated

a year ago
Blocks: 1213478
Iteration: 47.2 - Feb 22 → 47.3 - Mar 7

Comment 17

a year ago
https://hg.mozilla.org/integration/fx-team/rev/f3dcf982a76f
Keywords: checkin-needed
backed out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=7514363&repo=fx-team
Flags: needinfo?(lgreco)
(Assignee)

Comment 19

a year ago
(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.
Flags: needinfo?(lgreco)

Comment 20

a year ago
Backout:
https://hg.mozilla.org/integration/fx-team/rev/7a39bf96ab31
(Assignee)

Comment 21

a year ago
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/
Attachment #8717563 - Attachment description: MozReview Request: Bug 1239349 - Implement webNavigation.onHistoryStateUpdated. r?kmag → MozReview Request: Bug 1239349 - Implement webNavigation.onHistoryStateUpdated. r=kmag
(Assignee)

Comment 22

a year ago
Pushed updated patch to fix eslint errors.

Re-applied checkin-needed keyword.
Keywords: checkin-needed

Comment 23

a year ago
https://hg.mozilla.org/integration/fx-team/rev/9b5fd0f8dbf1
Keywords: checkin-needed

Comment 24

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/9b5fd0f8dbf1
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Keywords: dev-doc-needed
-> https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/WebNavigation/onHistoryStateUpdated
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.