Closed
Bug 1287649
Opened 8 years ago
Closed 8 years ago
pageAction should not be hidden when only the reference fragment changes
Categories
(WebExtensions :: Untriaged, defect, P1)
WebExtensions
Untriaged
Tracking
(firefox50 verified)
VERIFIED
FIXED
mozilla50
Tracking | Status | |
---|---|---|
firefox50 | --- | verified |
People
(Reporter: robwu, Assigned: kmag)
References
(Blocks 1 open bug)
Details
(Whiteboard: [pageAction] triaged)
Attachments
(2 files)
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•8 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•8 years ago
|
Priority: -- → P1
Whiteboard: [pageAction] triaged
Updated•8 years ago
|
Assignee: nobody → kmaglione+bmo
Assignee | ||
Comment 2•8 years ago
|
||
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)
Updated•8 years ago
|
Attachment #8774958 -
Flags: review?(aswan) → review+
Comment 3•8 years ago
|
||
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•8 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•8 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•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/b46427537871
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Comment 7•8 years ago
|
||
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•8 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)
Comment 9•8 years ago
|
||
Thanks Rob! Based on the above comments (Comment 7 and Comment 8) I am marking this bug as verified.
Status: RESOLVED → VERIFIED
Updated•6 years ago
|
Product: Toolkit → WebExtensions
You need to log in
before you can comment on or make changes to this bug.
Description
•