Page action panel ordering can get messed up

RESOLVED FIXED in Firefox 57

Status

()

Firefox
Menus
P1
normal
RESOLVED FIXED
4 months ago
3 months ago

People

(Reporter: adw, Assigned: adw)

Tracking

unspecified
Firefox 57
Points:
---
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox57 fixed)

Details

(Whiteboard: [reserve-photon-structure])

MozReview Requests

()

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

Attachments

(1 attachment)

(Assignee)

Description

4 months ago
I noticed that when I'm restoring all my windows on startup, in some of the windows, the web compat item is first in the panel, followed by a separator and then all the built-in items.  Also, in new windows, Pocket is now getting placed above the bookmark item, and I'm not sure when that started happening but I don't think it was always broken.

At least part of the problem is that BrowserPageActions.init incorrectly calls placeAction with only two arguments instead of three, and the second argument is wrong: https://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser-pageActions.js#51

I'll have a patch soon.

Updated

4 months ago
Iteration: --- → 57.2 - Aug 29
Flags: qe-verify?
Whiteboard: [photon-structure] → [reserve-photon-structure]
Comment hidden (mozreview-request)
(Assignee)

Comment 2

4 months ago
Work in progress.  Main part's done, working on a test.
(Assignee)

Comment 3

4 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e0171e3c0f64
Comment hidden (mozreview-request)
(Assignee)

Comment 5

4 months ago
I think there are a couple of problems:

* BrowserPageActions.init incorrectly calls placeAction with only two arguments instead of three, and the second argument is wrong.

* An action added by an extension can get placed in a browser window before that window's BrowserPageActions.init is called -- before that window places all the built-in actions.  Which the code does not expect and I think ends up placing the extension's action before all the built-in actions in the panel.  I think that's what I'm seeing both when restoring all my windows on startup and with Pocket.  I'm not 100% sure, but it's plausible.

The fix for the first problem is pretty easy.  I added another method to PageActions to get the appropriate place to insert actions in the panel.

The second problem points to the fact that PageActions isn't handling the ordering of actions and insertion and removal as robustly as it should be.  So most of the changes in this patch address that, and so does the new test.  Those changes include:

* Stop handling separators specially -- don't assume they should always be appended, don't allow them not to have IDs, etc.

* Don't assume there are built-in actions.  (Shouldn't happen normally of course, but the test checks this.)

Comment 6

4 months ago
mozreview-review
Comment on attachment 8898068 [details]
Bug 1391082 - Page action panel ordering can get messed up.

https://reviewboard.mozilla.org/r/169370/#review175230

Very nice. Thanks!
Attachment #8898068 - Flags: review?(gijskruitbosch+bugs) → review+

Comment 7

4 months ago
Pushed by dwillcoxon@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a08bb8b25b9b
Page action panel ordering can get messed up. r=Gijs

Comment 8

4 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a08bb8b25b9b
Status: ASSIGNED → RESOLVED
Last Resolved: 4 months ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57

Updated

4 months ago
Flags: qe-verify? → qe-verify-
(Assignee)

Updated

3 months ago
See Also: → bug 1396053
You need to log in before you can comment on or make changes to this bug.