Page action panel ordering can get messed up

RESOLVED FIXED in Firefox 57

Status

()

defect
P1
normal
RESOLVED FIXED
2 years ago
2 years 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])

Attachments

(1 attachment)

(Assignee)

Description

2 years 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.
Iteration: --- → 57.2 - Aug 29
Flags: qe-verify?
Whiteboard: [photon-structure] → [reserve-photon-structure]
Comment hidden (mozreview-request)
(Assignee)

Comment 2

2 years ago
Work in progress.  Main part's done, working on a test.
Comment hidden (mozreview-request)
(Assignee)

Comment 5

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

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

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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a08bb8b25b9b
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Flags: qe-verify? → qe-verify-
(Assignee)

Updated

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