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

VERIFIED FIXED in Firefox 57

Status

()

P1
normal
VERIFIED FIXED
a year ago
a year ago

People

(Reporter: adw, Assigned: adw)

Tracking

unspecified
Firefox 57
Points:
---

Firefox Tracking Flags

(firefox57 verified, firefox58 verified)

Details

(Whiteboard: [reserve-photon-structure])

Attachments

(1 attachment)

(Assignee)

Description

a year 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)
(Assignee)

Comment 2

a year 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.
(Assignee)

Comment 3

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

Comment 5

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

https://reviewboard.mozilla.org/r/175540/#review180974

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 => a.id == 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+
(Assignee)

Comment 6

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

Comment 8

a year ago
Pushed by dwillcoxon@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2efc7d0accca
Page action urlbar button ordering can get messed up for new windows. r=Gijs
https://hg.mozilla.org/mozilla-central/rev/2efc7d0accca
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox57: --- → fixed
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)
(Assignee)

Comment 11

a year 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., mozilla.com)

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

Actual with the 9/4 Nightly (http://ftp.mozilla.org/pub/firefox/nightly/2017/09/2017-09-04-22-00-27-mozilla-central/): 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.
status-firefox58: --- → verified
Verified on Windows 10 64bit, Mac OS X 10.13, Ubuntu 16.04 64bit using Beta 57.0b11 (64-bit).
Status: RESOLVED → VERIFIED
status-firefox57: fixed → verified
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.