Closed
Bug 1378560
Opened 7 years ago
Closed 7 years ago
The order of items in the url bar should be (from right-to-left) bookmarks, page action menu
Categories
(Firefox :: Address Bar, enhancement, P1)
Firefox
Address Bar
Tracking
()
Tracking | Status | |
---|---|---|
firefox57 | --- | verified |
People
(Reporter: abenson, Assigned: adw)
References
(Blocks 1 open bug, )
Details
(Whiteboard: [photon-structure])
Attachments
(1 file)
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•7 years ago
|
Updated•7 years ago
|
Whiteboard: [photon-structure] → [photon-structure] [triage]
Comment 1•7 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)
Updated•7 years ago
|
Flags: qe-verify+
Priority: -- → P2
QA Contact: gwimberly
Whiteboard: [photon-structure] [triage] → [photon-structure]
Reporter | ||
Comment 2•7 years 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•7 years ago
|
Component: Menus → Location Bar
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → adw
Status: NEW → ASSIGNED
Updated•7 years ago
|
Iteration: --- → 57.1 - Aug 15
Priority: P2 → P1
Assignee | ||
Comment 3•7 years ago
|
||
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•7 years ago
|
Reporter | ||
Comment 5•7 years 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.
Updated•7 years ago
|
Iteration: 57.1 - Aug 15 → 57.2 - Aug 29
Comment hidden (mozreview-request) |
Assignee | ||
Comment 9•7 years ago
|
||
Start of a WIP that gets the ordering down. Shouldn't be too hard to finish this.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 12•7 years ago
|
||
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.)
Assignee | ||
Comment 13•7 years ago
|
||
Comment 14•7 years ago
|
||
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 15•7 years ago
|
||
comment 14 was on Windows 10 default theme normal density.
Comment 16•7 years ago
|
||
(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•7 years 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•7 years 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•7 years 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•7 years 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+
Assignee | ||
Comment 23•7 years ago
|
||
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•7 years 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) |
Assignee | ||
Comment 26•7 years ago
|
||
This adds a comment to the CSS about the border and also consolidates the min-height rule into the main selector.
Comment 27•7 years 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
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.
Flags: needinfo?(adw)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 30•7 years ago
|
||
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•7 years 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
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
Flags: needinfo?(adw)
Comment 33•7 years 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) |
Assignee | ||
Comment 35•7 years ago
|
||
Flags: needinfo?(adw)
Assignee | ||
Comment 36•7 years ago
|
||
Try looks OK, landing again.
Comment 37•7 years 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•7 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Comment 39•7 years ago
|
||
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 → ---
Updated•7 years ago
|
Iteration: 57.2 - Aug 29 → ---
Assignee | ||
Comment 40•7 years ago
|
||
Thanks for noticing, Grover. I filed a new bug, bug 1395743.
Status: REOPENED → RESOLVED
Closed: 7 years ago → 7 years ago
Resolution: --- → FIXED
Comment 41•7 years ago
|
||
Since the issue in Comment 39 is a separate bug, the rest is Verified on Windows, Mac, and Ubuntu.
Updated•7 years ago
|
Iteration: --- → 57.2 - Aug 29
You need to log in
before you can comment on or make changes to this bug.
Description
•