1287649 - pageAction should not be hidden when only the reference fragment changes

Status: VERIFIED FIXED

(WebExtensions :: Untriaged, defect, P1)

Description

[Rob Wu [:robwu]]

Attachment: pageAction-webNavigation.zip

1. Load the attached extension. It will use the onCommitted (and onHistoryStateUpdated) webNavigation events to show the pageAction, unless the URL (including fragment) contains "hideme".
2. Visit example.com.
3. Confirm that the page action is shown.
4. Navigate to example.com/hideme
5. Confirm that the page action is hidden.
6. Open the JS console for the tab, and run history.pushState(null, null, '/');
7. Confirm that the page action is shown.
8. Change the reference fragment to #hideme, e.g. by running location.hash = '#hideme';

Expected (Chromium 51):
- After step 8, the page action should still be visible, because the webNavigation.onCommitted/onHistoryStateUpdated events are not fired, and the page action state should therefore not change.

Actual (Firefox 47, Firefox Nightly 50):
- After step 8, the page action is hidden.

Comment 1

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

I've done some debugging about the above scenario, to check if the issue is generated from the webNavigation events or from the behavior of the pageAction.

From my debugging session, it looks like no onCommitte/onHistoryStateUpdated events have been fired from the webNavigation API on the step 8, and so it is not the callback registered by the attached addon that hides the pageAction.

On the contrary, by looking at the pageAction implementation, it looks like that pageAction implementation hides the pageAction because it doesn't recognize that the location change wasn't generated by a navigation to another page (document), but it is generated from a hash change (without any change to the loaded document). 

Follow links to the related pieces of code:

- pageAction register a listener to the "location-change" event emitted by the TabContext: https://dxr.mozilla.org/mozilla-central/source/browser/components/extensions/ext-pageAction.js#39

- pageAction handle the "location-change" event and clear the pageAction if the fromBrowse parameter is true: https://dxr.mozilla.org/mozilla-central/source/browser/components/extensions/ext-pageAction.js#173

- TabContext's onLocationChange emit the "location-change" event with the third parameter set to true: https://dxr.mozilla.org/mozilla-central/source/browser/components/extensions/ext-utils.js#334

Comment 2

[Kris Maglione [:kmag]]

Attachment: Bug 1287649: Don't hide pageAction when only hash fragment changes.

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

Comment 3

[Andrew Swan [:aswan]]

Comment on attachment 8774958 [details]
Bug 1287649: Don't hide pageAction when only hash fragment changes.

https://reviewboard.mozilla.org/r/67304/#review64582

This seems okay, but onLocationChange also has a flag called SAME_DOCUMENT that I think would be simpler and ought to achieve the same result?

Comment 4

[Kris Maglione [:kmag]]

https://reviewboard.mozilla.org/r/67304/#review64582

I thought about that, but the problem is that it's also set for URL changes via `history.pushState`, which I think that we probably want to treat as if it were an actual navigation. It might be worth checking the flag in addition to the previous URL, though, I guess.

Comment 5

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/fx-team/rev/b46427537871aa6f56560076a82eea144674c8d9
Bug 1287649: Don't hide pageAction when only hash fragment changes. r=aswan

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b46427537871

Comment 7

[Vasilica Tamas, QA [:vtamas]]

I was able to reproduce the initial issue on Firefox 50.0a1 (2016-07-17) under Windows 10 64-bit.

Tested on Firefox 50.0a2 (2016-09-05) and Firefox 51.0a1 (2016-09-05) under Windows 10 64-bit and noticed that the page action is no longer hidden while running location.hash = '#hideme' but after refreshing the page it disappears.

Rob, can you please confirm that the page action disappearance after refreshing the page is also an expected behaviour?

Comment 8

[Rob Wu [:robwu]]

Yes - that is correct. The test case checks whether the URL contains "hideme" and if it does, hides the page action.
The check is done at page load and after history state change (e.g. using history.pushState).

Neither of these events are (supposed to be) triggered when you use location.hash = '#hideme';
But when you reload the page, the check is done and the correct action is to hide the page action if the URL contains "#hideme".

Comment 9

[Vasilica Tamas, QA [:vtamas]]

Thanks Rob! 

Based on the above comments (Comment 7 and Comment 8) I am marking this bug as verified.