Page action urlbar button ordering can get messed up for new windows

VERIFIED FIXED in Firefox 57



2 years ago
2 years ago


(Reporter: adw, Assigned: adw)


Firefox 57
Dependency tree / graph

Firefox Tracking Flags

(firefox57 verified, firefox58 verified)


(Whiteboard: [reserve-photon-structure])


(1 attachment)



2 years ago
When you open a new window, BrowserPageActions iterates through all the registered page actions and places them in the window.  The registered actions are ordered according to how they appear in the panel, not the urlbar.  That means that if action A comes before action B in both the panel and the urlbar, placeActionInUrlbar will try to insert A's urlbar node before B's node, but B's node hasn't been created yet.  So B's node is null, so insertBefore() ends up appending A's node to the urlbar.

See also bug 1391082.
Iteration: --- → 57.3 - Sep 19
Flags: qe-verify?
Priority: -- → P1
Comment hidden (mozreview-request)

Comment 2

2 years ago
This has more changes than I thought I would make, but they're good improvements I think.  Basically, I want all windows to use the same logic when they add DOM for their initial actions, regardless of whether they're opened before PageActions.init is called or after.  (The first window after startup is opened before.)  So now I have a new BrowserPageActions.placeAllActions that does that.

Second, I wanted to remove duplicated or at least similar logic in two places: when windows add their initial actions, and when actions are added after PageActions.init is called (i.e., by extensions via addAction).  addAction is now very simple.  It just tells each BrowserPageActions to place the action and lets its determine where its DOM nodes need to go, which is now the same as what happens for initial actions.

Comment 3

2 years ago
This is built on the patches in bug 1395154 and bug 1395410 by the way, in case it matters.

Comment 5

2 years ago
Comment on attachment 8903791 [details]
Bug 1396053 - Page action urlbar button ordering can get messed up for new windows.

r=me with the (AFAICT?) dead sepNextID bits removed. Thanks!

::: browser/modules/PageActions.jsm:166
(Diff revision 1)
> +    let sepNextID = null;
> +    if (!hadSep) {
> +      sep = this.actions.find(a => == ACTION_ID_BUILT_IN_SEPARATOR);
> +      if (sep) {
> +        sepNextID = this.nextActionID(sep, this.actions);
> +      }
> +    }

Hm, but you don't use `sepNextID` here, it seems?
Attachment #8903791 - Flags: review?(gijskruitbosch+bugs) → review+

Comment 6

2 years ago
Thanks, that was left over and should have been removed.
Comment hidden (mozreview-request)

Comment 8

2 years ago
Pushed by
Page action urlbar button ordering can get messed up for new windows. r=Gijs
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Flags: qe-verify? → qe-verify+
QA Contact: gwimberly

Comment 10

2 years ago
Hi Drew, any suggested STR to be able to verify? 

1. Open a website
2. Open a new window
3. ???

What would be the actions to do past this point to determine this bug is verified?
Flags: needinfo?(adw)

Comment 11

2 years ago
Hi Grover, try this:

1. Start with a new profile
2. Open about:config
3. Search for pref browser.pageActions.persistedActions
4. Change its value to: {"version":1,"ids":["bookmark","bookmarkSeparator","copyURL","emailLink","sendToDevice","pocket","webcompat-reporter-button","screenshots"],"idsInUrlbar":["pocket","copyURL","bookmark"]}
5. Restart
6. Open a page that shows the action buttons (e.g.,

Expected: The action buttons are, in order: the main button (...), Pocket, copy link, and bookmark star

Actual with the 9/4 Nightly ( The action buttons are: the main button, bookmark star, and Pocket

Actual with the current Nightly: Same as expected
Flags: needinfo?(adw)

Comment 12

2 years ago
Verified on Windows 10 64bit, Mac OS X 10.13, Ubuntu 16.04 64bit using Nightly 58.0a1 (64-bit) with today's build.

Comment 13

2 years ago
Verified on Windows 10 64bit, Mac OS X 10.13, Ubuntu 16.04 64bit using Beta 57.0b11 (64-bit).


2 years ago
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.