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

Status: VERIFIED FIXED

(Firefox :: Menus, defect, P1)

Description

[Drew Willcoxon :adw]

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.

Comment 1

[Drew Willcoxon :adw]

Attachment: Bug 1396053 - Page action urlbar button ordering can get messed up for new windows.

Review commit: https://reviewboard.mozilla.org/r/175540/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/175540/

Comment 2

[Drew Willcoxon :adw]

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

[Drew Willcoxon :adw]

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

Comment 4

[Drew Willcoxon :adw]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e5502463bfa1

Comment 5

[:Gijs (he/him)]

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?

Comment 6

[Drew Willcoxon :adw]

Thanks, that was left over and should have been removed.

Comment 7

[Drew Willcoxon :adw]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/175540/diff/1-2/

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2efc7d0accca
Page action urlbar button ordering can get messed up for new windows. r=Gijs

Comment 9

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/2efc7d0accca

Comment 10

[u554753]

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?

Comment 11

[Drew Willcoxon :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., 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

Comment 12

[u554753]

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

[u554753]

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