The bookmark star should be the right-most item and the page action menu should be to its left. New items (such as Pocket) get placed left of the bookmark star. See the mockup and prototypes linked below: https://mozilla.invisionapp.com/share/PYC5LJJXG#/screens/229940647_Toolbars https://people-mozilla.org/~shorlander/projects/photon/Mockups/windows-10.html

OK. So, the current order of things here is as follows (in photon), left to right in an English (ltr) Firefox: popup blocker icon reader mode icon bookmarks star url bar zoom indicator any webextension page actions the user adds "containers" indicator page action button From discussion here and elsewhere, it sounds like you want: "containers" indicator page action button popup blocker icon reader mode icon bookmarks star any webextension page actions the user adds url bar zoom indicator maybe? I'm not 100% sure. Can you reorder as appropriate? Thanks!

Spec for icon placement inside the URL bar can be found here: https://mozilla.invisionapp.com/share/94CLGV8GN#/243839921_Explainer_-_URL_Bar_Icon_Placement

See also bug 1386627: (In reply to :Gijs from bug 1386627 comment #2) > TBH, I suspect it's not worth fixing this separately from bug 1378560.

(In reply to Aaron Benson from comment #2) > Spec for icon placement inside the URL bar can be found here: > https://mozilla.invisionapp.com/share/94CLGV8GN#/243839921_Explainer_- > _URL_Bar_Icon_Placement Just want to point out that I added details for the "go" button (arrow) to this spec. It should be the right-most item when the URL bar is in focus and being typed in.

Start of a WIP that gets the ordering down. Shouldn't be too hard to finish this.

This does several things: * Separates page action buttons from other icons in the urlbar (#page-action-buttons vs. #urlbar-icons) * Orders everything according to spec * Draws a separator between those two groups * Polishes the spacing between all icons in the urlbar Gijs, I'm using a mutation observer to determine when the separator needs to be shown. I can't think of a better, simpler way than that. Please let me know if you can. (I don't want to add yet another (pseudo-)API for that, or add logic to each site that hides/unhides these elements.)

The reader mode icon is stretched vertically with the patch you have on mozreview (I used hg pull and built using the changeset that your patch is based on): https://www.screencast.com/t/xlMXyyeNTwA

comment 14 was on Windows 10 default theme normal density.

(In reply to Drew Willcoxon :adw from comment #12) > This does several things: > > * Separates page action buttons from other icons in the urlbar > (#page-action-buttons vs. #urlbar-icons) This distinction isn't really clear. It doesn't help that urlbar-icons contains non-urlbar-icon things.

Comment on attachment 8899051 [details] Bug 1378560 - The order of items in the url bar should be (from right-to-left) bookmarks, page action menu. https://reviewboard.mozilla.org/r/170374/#review176720 Going to clear review for now given the feedback on the bug and the suggestion below, which I hope helps simplify this a bit. :-) ::: browser/base/content/browser-pageActions.js:69 (Diff revision 2) > + * #page-action-buttons when all the elements in #urlbar-icons are hidden. > + */ > + _addSeparatorObserver() { > + let icons = document.getElementById("urlbar-icons"); > + let actionButtons = this.mainButtonNode.parentNode; > + this._separatorObserver = new MutationObserver(mutations => { Yeah, I'm worried about the performance implications of doing this. Mutation observers are... not great for that. One thing you could do is add a node to the urlbar-icons container that is styled as the separator (as a sibling to the actual reader mode icon etc.), style it with display: none by default, and then use: ```css #urlbar-icons > :-moz-any(image,toolbarbutton,hbox):not([hidden]) ~ #separator-element { display: -moz-box; } ``` To unhide it whenever any of the other kids doesn't have the `hidden` attribute. (The -moz-any is kind of ugly, you could add a class and use that, maybe.)

(In reply to :Gijs from comment #17) > ```css > #urlbar-icons > :-moz-any(image,toolbarbutton,hbox):not([hidden]) ~ > #separator-element { > display: -moz-box; > } > ``` > > (The -moz-any is kind of ugly, you could add a class and use that, maybe.) Err, and the child selector is unnecessary. But hey, otherwise...

Comment on attachment 8899051 [details] Bug 1378560 - The order of items in the url bar should be (from right-to-left) bookmarks, page action menu. https://reviewboard.mozilla.org/r/170374/#review176934 ::: browser/base/content/browser.xul:858 (Diff revision 3) > </box> > <box id="urlbar-display-box" align="center"> > <label id="switchtab" class="urlbar-display urlbar-display-switchtab" value="&urlbar.switchToTab.label;"/> > <label id="extension" class="urlbar-display urlbar-display-extension" value="&urlbar.extension.label;"/> > </box> > - <hbox id="page-action-buttons"> > + <hbox id="urlbar-icons"> As far as I can see, you can undo this and just keep page-action-buttons as the only container. Alternatively, find a better id for this. ::: browser/themes/shared/urlbar-searchbar.inc.css:19 (Diff revision 3) > padding: 0; > margin: 0 5px; > min-height: 30px; > } > > +#urlbar-icons-separator { Please don't put this in the middle of the #urlbar, .searchbar-textbox rules. ::: browser/themes/shared/urlbar-searchbar.inc.css:160 (Diff revision 3) > + border-image: linear-gradient(transparent 15%, var(--urlbar-separator-color) 15%, var(--urlbar-separator-color) 85%, transparent 85%); > + border-image-slice: 1; > + display: none; > +} > + > +#urlbar-icons > *:not(#urlbar-icons-separator):not([hidden]) ~ #urlbar-icons-separator { Remove #urlbar-icons > * ::: browser/themes/shared/urlbar-searchbar.inc.css:166 (Diff revision 3) > + /* Show the separator between the urlbar icons and page actions when at least > + one urlbar icon is shown. */ > + display: -moz-box; > +} > + > +#urlbar-icons > *:not(.urlbar-icon) { Instead of this inefficient selector, be explicit about the elements you're targetting here.

Comment on attachment 8899051 [details] Bug 1378560 - The order of items in the url bar should be (from right-to-left) bookmarks, page action menu. https://reviewboard.mozilla.org/r/170374/#review177084 Looks great, thanks! ::: browser/themes/shared/urlbar-searchbar.inc.css:150 (Diff revision 4) > + border-inline-start: 1px solid var(--urlbar-separator-color); > + border-image: linear-gradient(transparent 15%, var(--urlbar-separator-color) 15%, var(--urlbar-separator-color) 85%, transparent 85%); > + border-image-slice: 1; I'm sure this is a dumb question, but why can't we just give this a single solid background/border color and then use margins to give it the right visual height? This seems like it's more complex then necessary off-hand. It might be worth adding a comment here about why we're doing that?

Not dumb -- the reason is to match exactly how #urlbar-display-box draws its two borders: https://dxr.mozilla.org/mozilla-central/source/browser/themes/shared/identity-block/identity-block.inc.css#62 https://dxr.mozilla.org/mozilla-central/source/browser/themes/shared/identity-block/identity-block.inc.css#75 It seems like a good idea to do the same thing so we're more likely to get the same results. It is possible for #urlbar-display-box and at least its right border to be visible at the same time as this new separator. It would be ugly for the separator to have a slightly different height or vertical position, for example. Do you think that's OK?

(In reply to Drew Willcoxon :adw from comment #23) > Not dumb -- the reason is to match exactly how #urlbar-display-box draws its > two borders: > > https://dxr.mozilla.org/mozilla-central/source/browser/themes/shared/ > identity-block/identity-block.inc.css#62 > > https://dxr.mozilla.org/mozilla-central/source/browser/themes/shared/ > identity-block/identity-block.inc.css#75 > > It seems like a good idea to do the same thing so we're more likely to get > the same results. It is possible for #urlbar-display-box and at least its > right border to be visible at the same time as this new separator. It would > be ugly for the separator to have a slightly different height or vertical > position, for example. > > Do you think that's OK? Ah, yes, this makes sense. Maybe file a followup to share this styling and/or simplify this (class .urlbar-separator or something)? I dunno off-hand if the folks who wrote that CSS had their own reasons for doing it this way rather than a fixed single-colour bg that didn't go all the way to the edge, but we don't need to worry about that here, let's just get this in considering the number of people running into issues with the current state.

This adds a comment to the CSS about the border and also consolidates the min-height rule into the main selector.

Pushed by dwillcoxon@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/535fefe4cc5e The order of items in the url bar should be (from right-to-left) bookmarks, page action menu. r=Gijs

This seems to have caused a couple very frequent (but not permafailing) mochitest failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&fromchange=40aafceb5e48a7139e934bf698c32c491b0936a1&noautoclassify&filter-searchStr=linux%20mochi%20s(6)&tochange=f4509989f87c9d2db5e33db25a0dc8c260017774&selectedJob=125358795 https://treeherder.mozilla.org/#/jobs?repo=autoland&fromchange=40aafceb5e48a7139e934bf698c32c491b0936a1&noautoclassify&filter-searchStr=linux%20mochi%20s(9)&tochange=f4509989f87c9d2db5e33db25a0dc8c260017774&selectedJob=125358714 Backed out in https://hg.mozilla.org/integration/autoland/rev/f4509989f87c9d2db5e33db25a0dc8c260017774 to see if the failures go away.

The cause of the hang is here: https://dxr.mozilla.org/mozilla-central/rev/1867d7931c0a70ab90edf4aa84876525773a7139/dom/tests/mochitest/pointerlock/pointerlock_utils.js#63 Which is called by addFullscreenChangeContinuation, right below it. This test runs a bunch of html subtests. Each runs in a new 500x500 browser window that only has a navbar. In the navbar is: flexible spacer, urlbar, flexible spacer, menu button. The particular subtest that hangs happens to trigger the reader mode icon. That triggers the separator. That somehow causes the window's width to go from 500 to 498. That screws up the test because it's waiting on the window to return to its previous size after exiting full screen, or something. I don't know why the window's width changes. The flexible spacers that are now on either side of the urlbar? Is the width of the urlbar changing, and why's that happening in the first place? I fixed it by using `visibility` instead of `display` to show/hide the separator. Now it's `visibility: visible` when the separator is shown, and `visibility: hidden` when it's not.

Pushed by dwillcoxon@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/5885fd0c53a7 The order of items in the url bar should be (from right-to-left) bookmarks, page action menu. r=Gijs

I had to back this out again for various mochitest failures like: https://treeherder.mozilla.org/logviewer.html#?job_id=125410107&repo=autoland https://treeherder.mozilla.org/logviewer.html#?job_id=125410111&repo=autoland https://treeherder.mozilla.org/logviewer.html#?job_id=125410025&repo=autoland

Backout by kwierso@gmail.com: https://hg.mozilla.org/integration/autoland/rev/0b8672f85f06 Backed out changeset 5885fd0c53a7 for various mochitest-plain failures a=backout

Try looks OK, landing again.

Pushed by dwillcoxon@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/12052e2c9fe6 The order of items in the url bar should be (from right-to-left) bookmarks, page action menu. r=Gijs

The behavior I am seeing is that the pocket item is being placed to the right of the bookmark star instead of the left as shown in the spec as of today's Nightly. This behavior is seen in Windows, Mac, and Ubuntu.

Thanks for noticing, Grover. I filed a new bug, bug 1395743.

Since the issue in Comment 39 is a separate bug, the rest is Verified on Windows, Mac, and Ubuntu.