1352366 - Implement new location and search bar appearance

Status: VERIFIED FIXED

(Firefox :: Theme, enhancement, P1)

Description

[Dão Gottwald [:dao]]

Stephen to provide UX spec.

Comment 1

[Darkspirit]

bug 1352164 comment 1 may belong to this bug

Comment 2

[Dão Gottwald [:dao]]

Dale, FYI, we'll need to implement this behind an ifdef such as PHOTON_THEME so that we don't change how Firefox 55 and 56 look. For future reference, the [57] whiteboard tag is meant to mark these kind of bugs.

Comment 3

[Darkspirit]

(In reply to Dão Gottwald [::dao] from comment #2)
> ifdef such as PHOTON_THEME
Please consider a (maybe Nightly-only) pref like browser.photon.structure.enabled to make it unnecessary to compile oneself.

Comment 4

[Dão Gottwald [:dao]]

(In reply to Darkspirit from comment #3)
> (In reply to Dão Gottwald [::dao] from comment #2)
> > ifdef such as PHOTON_THEME
> Please consider a (maybe Nightly-only) pref like
> browser.photon.structure.enabled to make it unnecessary to compile oneself.

Prefs are more cumbersome to use in theme code. I think we'll just enable PHOTON_THEME by default for Nightly sometime down the road, probably in the 56 cycle.

Comment 5

[Dale Harvey (:daleharvey)]

Attachment: Implement photon location bar style changes

Getting my linux and windows builds ready for testing, only tested on osx so far but works there and enough to check that this is the right direction / bikeshed about names etc

Comment 6

[Dale Harvey (:daleharvey)]

Attachment: Implement photon location bar style changes

Apologies for being a .patch rather than mozreview, will fix my hg setup when home

Comment 7

[Dale Harvey (:daleharvey)]

Attachment: Implement photon location bar style changes

After feedback from dao, use system provided border colours and backgrounds

Minor differences from the spec

We only show transparency in the url bar when using lightweight themes, by default it was impossible to notice

Linux only shows the box-shadow on url bar hover and not a changed border color as no border color is provided by the system (and we cant tell default themes in linux)

Comment 8

[Dale Harvey (:daleharvey)]

Attachment: Implement photon location bar style changes

Refactored a little from feedback, restored font: icon and split out focused border style

Comment 9

[Dale Harvey (:daleharvey)]

One issue I have noticed is that by removing the padding on osx the url bar is a little cramped and a lot smaller than the mockups, however even with the padding on osx the url bar has a larger margin than other platforms so I wasnt not sure if that was to be fixed in this bug or a wider toolbar bug (the background etc also needs changed)

Comment 10

[Darkspirit]

It would be cool if I could place the home button between (or after) the forward and reload button, for example.

Comment 11

[Dão Gottwald [:dao]]

(In reply to Dale Harvey (:daleharvey) from comment #9)
> One issue I have noticed is that by removing the padding on osx the url bar
> is a little cramped and a lot smaller than the mockups, however even with
> the padding on osx the url bar has a larger margin than other platforms so I
> wasnt not sure if that was to be fixed in this bug or a wider toolbar bug
> (the background etc also needs changed)

Could you attach a screenshot of how this looks with your patch on Mac?

(In reply to Darkspirit from comment #10)
> It would be cool if I could place the home button between (or after) the
> forward and reload button, for example.

Bug 1363485 will make this possible.

Comment 12

[Dale Harvey (:daleharvey)]

Attachment: Screen Shot 2017-05-12 at 16.58.28.png

Windows and Linux both have padding: 0 here, OSX had padding: 1, even with padding the url bar is much smaller on osx than linux/windows

Comment 13

[Dão Gottwald [:dao]]

(In reply to Dale Harvey (:daleharvey) from comment #12)
> Created attachment 8867351 [details]
> Screen Shot 2017-05-12 at 16.58.28.png
> 
> Windows and Linux both have padding: 0 here, OSX had padding: 1, even with
> padding the url bar is much smaller on osx than linux/windows

Seems okay for now, we can further tweak this later.

Comment 14

[Dão Gottwald [:dao]]

Comment on attachment 8867309 [details] [diff] [review]
Implement photon location bar style changes

>+++ b/browser/themes/linux/browser.css

>+#urlbar[focused="true"],
>+.searchbar-textbox[focused="true"] {
>+  border: 1px solid Highlight;

Please use border-color instead.

>--- a/browser/themes/osx/browser.css
>+++ b/browser/themes/osx/browser.css

>+#main-window {
>+  --color-chrome-border-values: 240, 5%, 5%;
>+  --urlbar-border-color: hsla(var(--color-chrome-border-values), .25);
>+  --urlbar-border-color-hover: hsla(var(--color-chrome-border-values), .35);

Since CSS variables have some runtime overhead and --color-chrome-border-values isn't used anywhere else, please just put 240, 5%, 5% directly in the latter two variables.

>+#urlbar[focused="true"],
>+.searchbar-textbox[focused="true"] {
>+  border: 1px solid -moz-mac-focusring;

border-color

>--- a/browser/themes/windows/browser.css
>+++ b/browser/themes/windows/browser.css

>+@media (-moz-windows-default-theme) {
>+  #main-window:not(:-moz-lwtheme) {
>+    --color-chrome-border-values: 240, 5%, 5%;
>+    --urlbar-border-color: hsla(var(--color-chrome-border-values), .25);
>+    --urlbar-border-color-hover: hsla(var(--color-chrome-border-values), .35);

Again, let's get rid of --color-chrome-border-values here.

>+#urlbar[focused="true"],
>+.searchbar-textbox[focused="true"] {
>+  border: 1px solid Highlight;

border-color :)

Comment 15

[Darkspirit]

Attachment: different-locationbar-heights.png

A locationbar with "Nightly" or "(padlock) EV cert owner Corp." at the beginning has 1px more height than a normal one.

Comment 16

[Darkspirit]

(In reply to Darkspirit from comment #15)
+ Shouldn't those three dots at the end of the locationbar ("This space intentionally left blank.") only get shown when browser.photon.structure.enabled=true for the moment?

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9d98f8c36399
Implement photon location bar style changes. r=dao

Comment 18

[Dale Harvey (:daleharvey)]

Landed, taking a look at the height issues, is there already a bug we can address the height issues in or should I make one / take it?

Comment 19

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

Backed out for failing browser_overflowScroll.js on OS X:

https://hg.mozilla.org/integration/mozilla-inbound/rev/7499b66a2f94391bd6a26053659a212c95328f37

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=9d98f8c363992ade3c600bd83932ee7ad5bf9076&filter-searchStr=98b7e26b3d04d3074c6aa0dbd7d9fee078960e23
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=98927453&repo=mozilla-inbound

14:28:57     INFO - TEST-START | browser/base/content/test/general/browser_overflowScroll.js
14:28:57     INFO - TEST-INFO | started process screencapture
14:28:57     INFO - TEST-INFO | screencapture: exit 0
14:28:57     INFO - Buffered messages logged at 14:28:57
14:28:57     INFO - TEST-PASS | browser/base/content/test/general/browser_overflowScroll.js | Selecting the first tab scrolls it into view (143 <= 158) - 
14:28:57     INFO - TEST-PASS | browser/base/content/test/general/browser_overflowScroll.js | Scrolled one tab to the right with a single click - 
14:28:57     INFO - TEST-PASS | browser/base/content/test/general/browser_overflowScroll.js | Selecting the last tab scrolls it into view (1173 <= 1188) - 
14:28:57     INFO - Buffered messages finished
14:28:57     INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/general/browser_overflowScroll.js | Scrolled one tab to the left with a single click - Got 58, expected 143
14:28:57     INFO - Stack trace:
14:28:57     INFO - chrome://mochikit/content/browser-test.js:test_is:928
14:28:57     INFO - chrome://mochitests/content/browser/browser/base/content/test/general/browser_overflowScroll.js:isLeft:10
14:28:57     INFO - chrome://mochitests/content/browser/browser/base/content/test/general/browser_overflowScroll.js:runOverflowTests:64
14:28:57     INFO - Not taking screenshot here: see the one that was previously logged
14:28:57     INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/general/browser_overflowScroll.js | Scrolled one page of tabs with a double click - Got -942, expected 143
14:28:57     INFO - Stack trace:
14:28:57     INFO - chrome://mochikit/content/browser-test.js:test_is:928
14:28:57     INFO - chrome://mochitests/content/browser/browser/base/content/test/general/browser_overflowScroll.js:isLeft:10
14:28:57     INFO - chrome://mochitests/content/browser/browser/base/content/test/general/browser_overflowScroll.js:runOverflowTests:72
14:28:57     INFO - Not taking screenshot here: see the one that was previously logged
14:28:57     INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/general/browser_overflowScroll.js | Scrolled to the start with a triple click (143 <= -2327) - 
14:28:57     INFO - Stack trace:
14:28:57     INFO - chrome://mochitests/content/browser/browser/base/content/test/general/browser_overflowScroll.js:runOverflowTests:76
14:28:58     INFO - GECKO(1765) | MEMORY STAT | vsize 4802MB | residentFast 839MB | heapAllocated 274MB

Comment 20

[Gary [:streetwolf52]]

With this patch the location and search box height is too small.  Don't know if you backed out this patch because of that.  On Windows 10 x86-64.

Comment 21

[Dale Harvey (:daleharvey)]

Ugh sorry, I did a try run @ https://treeherder.mozilla.org/#/jobs?repo=try&revision=022223d4ab9be107bdec556923672952095ba8ca&selectedJob=98800866 but for some reason that didnt bother with mochitests on osx, looking at now

Comment 22

[Dale Harvey (:daleharvey)]

Attachment: Implement photon location bar style changes

So the extra radius (--toolbarbutton-border-radius: 4px;) makes these synthetic clicks http://searchfox.org/mozilla-central/source/browser/base/content/test/general/browser_overflowScroll.js#63 miss the target as the original syth event was |EventUtils.synthesizeMouse(upButton, 1, 1, {})| Changed the events to aim for the center of the button

Going to take a look at the height issues now before relanding

Comment 23

[Dale Harvey (:daleharvey)]

Comment on attachment 8867627 [details] [diff] [review]
Implement photon location bar style changes

Ok didnt touch the height in this version, only changes are the test fixes

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2d66ad57d5c0
Implement photon location bar style changes. r=dao

Comment 25

[Dale Harvey (:daleharvey)]

Gave it a try run, only failure looked unrelated and passed on restart https://treeherder.mozilla.org/#/jobs?repo=try&revision=5d4c0a4660e02c897ffffa3ac3811d44d589555d, landed, cheers

Comment 26

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 27

[Olli Pettay [:smaug][[e-mail]]]

Attachment: graylocationbar.png

FWIW, locationbar and searchbar look now a bit odd in linux / Fedora 25.

Comment 28

[Almas Hossain[:mAlmas]]

I have seen the feature being implemented with latest Nightly 55.0a1 on Windows 10 (64 bit).

This bug's fix is now verified in Latest Nightly.

Build ID   :   20170518030213
User Agent :   Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:55.0) Gecko/20100101 Firefox/55.0

[bugday 20170517]

Comment 29

[Dão Gottwald [:dao]]

Thanks.

Comment 30

[Johann Hofmann [:johannh]]

Screenshots:

https://screenshots.mattn.ca/compare/?oldProject=mozilla-central&oldRev=a4235c4be96edaf90b5d6d7c20272a8761ca2339&newProject=mozilla-central&newRev=3e166b6838931b3933ca274331f9e0e115af5cc0

I can identify the problems from bug 1366278 and bug 1366492, but otherwise this looks fine to me.

Comment 31

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Compared to the font size as used in the bookmarks toolbar and the tab labels, the font in the location and search bar looks oversized now. Is that something we want to fix? It looks kinda awkward.

Comment 32

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Henrik Skupin (:whimboo) from comment #31)
> Compared to the font size as used in the bookmarks toolbar and the tab
> labels, the font in the location and search bar looks oversized now. Is that
> something we want to fix? It looks kinda awkward.

I moved that comment out to bug 1372218.