The order of items in the url bar should be (from right-to-left) bookmarks, page action menu

VERIFIED FIXED in Firefox 57

Status

()

P1
normal
VERIFIED FIXED
2 years ago
5 months ago

People

(Reporter: abenson, Assigned: adw)

Tracking

(Blocks: 2 bugs)

unspecified
Firefox 57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 verified)

Details

(Whiteboard: [photon-structure], URL)

Attachments

(1 attachment)

(Reporter)

Description

2 years ago
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

Updated

2 years ago
Blocks: 1352697, 1352120
Whiteboard: [photon-structure] → [photon-structure] [triage]

Comment 1

2 years ago
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!
Flags: needinfo?(abenson)
Flags: qe-verify+
Priority: -- → P2
QA Contact: gwimberly
Whiteboard: [photon-structure] [triage] → [photon-structure]
(Reporter)

Comment 2

a year ago
Spec for icon placement inside the URL bar can be found here: https://mozilla.invisionapp.com/share/94CLGV8GN#/243839921_Explainer_-_URL_Bar_Icon_Placement
Flags: needinfo?(abenson)

Updated

a year ago
See Also: → bug 1381025
Component: Menus → Location Bar

Updated

a year ago
See Also: → bug 1386627
Assignee: nobody → adw
Status: NEW → ASSIGNED
Iteration: --- → 57.1 - Aug 15
Priority: P2 → P1
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.

Updated

a year ago
Duplicate of this bug: 1386627

Updated

a year ago
See Also: bug 1386627
See Also: → bug 1378458
(Reporter)

Comment 5

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

Updated

a year ago
See Also: → bug 1389554
Duplicate of this bug: 1389563

Updated

a year ago
Duplicate of this bug: 1390041
Iteration: 57.1 - Aug 15 → 57.2 - Aug 29
Comment hidden (mozreview-request)
Start of a WIP that gets the ordering down.  Shouldn't be too hard to finish this.

Updated

a year ago
Duplicate of this bug: 1392651
Comment hidden (mozreview-request)
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
(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 17

a year ago
mozreview-review
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.)
Attachment #8899051 - Flags: review?(gijskruitbosch+bugs)

Comment 18

a year ago
(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 hidden (mozreview-request)

Comment 20

a year ago
mozreview-review
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 hidden (mozreview-request)

Comment 22

a year ago
mozreview-review
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?
Attachment #8899051 - Flags: review?(gijskruitbosch+bugs) → review+
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?
Flags: needinfo?(gijskruitbosch+bugs)

Comment 24

a year ago
(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.
Flags: needinfo?(gijskruitbosch+bugs)
Comment hidden (mozreview-request)
This adds a comment to the CSS about the border and also consolidates the min-height rule into the main selector.

Comment 27

a year ago
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
Comment hidden (mozreview-request)
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.
Flags: needinfo?(adw)

Comment 31

a year ago
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

Comment 33

a year ago
Backout by kwierso@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/0b8672f85f06
Backed out changeset 5885fd0c53a7 for various mochitest-plain failures a=backout
Comment hidden (mozreview-request)
Try looks OK, landing again.

Comment 37

a year ago
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

Comment 38

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/12052e2c9fe6
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Iteration: 57.2 - Aug 29 → ---
Depends on: 1395743
Thanks for noticing, Grover.  I filed a new bug, bug 1395743.
Status: REOPENED → RESOLVED
Last Resolved: a year agoa year ago
Resolution: --- → FIXED
Since the issue in Comment 39 is a separate bug, the rest is Verified on Windows, Mac, and Ubuntu.
Status: RESOLVED → VERIFIED
status-firefox57: fixed → verified
Flags: qe-verify+
Iteration: --- → 57.2 - Aug 29
Depends on: 1479426
You need to log in before you can comment on or make changes to this bug.