Closed Bug 1396053 Opened 6 years ago Closed 6 years ago

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


(Firefox :: Menus, defect, P1)




Firefox 57
57.3 - Sep 19
Tracking Status
firefox57 --- verified
firefox58 --- verified


(Reporter: adw, Assigned: adw)



(Whiteboard: [reserve-photon-structure])


(1 file)

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
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.
This is built on the patches in bug 1395154 and bug 1395410 by the way, in case it matters.
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+
Thanks, that was left over and should have been removed.
Pushed by
Page action urlbar button ordering can get messed up for new windows. r=Gijs
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Flags: qe-verify? → qe-verify+
QA Contact: gwimberly
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)
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)
Verified on Windows 10 64bit, Mac OS X 10.13, Ubuntu 16.04 64bit using Nightly 58.0a1 (64-bit) with today's build.
Verified on Windows 10 64bit, Mac OS X 10.13, Ubuntu 16.04 64bit using Beta 57.0b11 (64-bit).
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.