Closed
Bug 1396053
Opened 6 years ago
Closed 6 years ago
Page action urlbar button ordering can get messed up for new windows
Categories
(Firefox :: Menus, defect, P1)
Firefox
Menus
Tracking
()
People
(Reporter: adw, Assigned: adw)
References
Details
(Whiteboard: [reserve-photon-structure])
Attachments
(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.
Updated•6 years ago
|
Iteration: --- → 57.3 - Sep 19
Flags: qe-verify?
Priority: -- → P1
Comment hidden (mozreview-request) |
Assignee | ||
Comment 2•6 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.
Assignee | ||
Comment 3•6 years ago
|
||
This is built on the patches in bug 1395154 and bug 1395410 by the way, in case it matters.
Assignee | ||
Comment 4•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e5502463bfa1
Comment 5•6 years 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•6 years ago
|
||
Thanks, that was left over and should have been removed.
Comment hidden (mozreview-request) |
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
![]() |
||
Comment 9•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/2efc7d0accca
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Updated•6 years ago
|
Flags: qe-verify? → qe-verify+
QA Contact: gwimberly
Comment 10•6 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)
Assignee | ||
Comment 11•6 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., 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)
Comment 12•6 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.
status-firefox58:
--- → verified
Comment 13•6 years ago
|
||
Verified on Windows 10 64bit, Mac OS X 10.13, Ubuntu 16.04 64bit using Beta 57.0b11 (64-bit).
Status: RESOLVED → VERIFIED
You need to log in
before you can comment on or make changes to this bug.
Description
•