pageAction should not be hidden when only the reference fragment changes

VERIFIED FIXED in Firefox 50

Status

WebExtensions
Untriaged
P1
normal
VERIFIED FIXED
2 years ago
3 days ago

People

(Reporter: robwu, Assigned: kmag)

Tracking

(Blocks: 1 bug)

Trunk
mozilla50
Dependency tree / graph

Firefox Tracking Flags

(firefox50 verified)

Details

(Whiteboard: [pageAction] triaged)

MozReview Requests

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

Attachments

(2 attachments)

(Reporter)

Description

2 years ago
Created attachment 8772223 [details]
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

2 years ago
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

Updated

2 years ago
Priority: -- → P1
Whiteboard: [pageAction] triaged

Updated

2 years ago
Assignee: nobody → kmaglione+bmo
(Assignee)

Comment 2

2 years ago
Created attachment 8774958 [details]
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/
Attachment #8774958 - Flags: review?(aswan)
Attachment #8774958 - Flags: review?(aswan) → review+
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?
(Assignee)

Comment 4

2 years ago
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.
(Assignee)

Comment 5

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

Comment 6

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b46427537871
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox50: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
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?
Flags: needinfo?(rob)
(Reporter)

Comment 8

2 years ago
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".
Flags: needinfo?(rob)
Thanks Rob! 

Based on the above comments (Comment 7 and Comment 8) I am marking this bug as verified.
Status: RESOLVED → VERIFIED
status-firefox50: fixed → verified

Updated

4 months ago
Depends on: 1438274

Updated

3 days ago
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.