Implement new location and search bar appearance

VERIFIED FIXED in Firefox 55

Status

()

enhancement
P1
normal
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: dao, Assigned: daleharvey)

Tracking

(Depends on 1 bug, Blocks 1 bug)

Trunk
Firefox 55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 verified)

Details

(Whiteboard: [photon-visual][p1][57])

Attachments

(4 attachments, 4 obsolete attachments)

(Reporter)

Description

2 years ago
Stephen to provide UX spec.
Flags: needinfo?(shorlander)
(Reporter)

Updated

2 years ago
Keywords: uiwanted
(Reporter)

Updated

2 years ago
Priority: -- → P1
(Reporter)

Updated

2 years ago
Depends on: 1355386
(Reporter)

Updated

2 years ago
Flags: needinfo?(shorlander)
(Reporter)

Updated

2 years ago
Whiteboard: [photon] → [photon][57]
(Reporter)

Updated

2 years ago
Whiteboard: [photon][57] → [photon-visual][57]
Flags: qe-verify+
Priority: P1 → P2
QA Contact: ovidiu.boca
(Reporter)

Updated

2 years ago
Whiteboard: [photon-visual][57] → [photon-visual][p1][57]
QA Contact: ovidiu.boca → brindusa.tot
(Assignee)

Updated

2 years ago
Assignee: nobody → dale
(Reporter)

Comment 2

2 years ago
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.
Status: NEW → ASSIGNED
Iteration: --- → 55.4 - May 1
Priority: P2 → P1
(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.
(Reporter)

Comment 4

2 years ago
(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.
Iteration: 55.4 - May 1 → 55.5 - May 15
(Reporter)

Updated

2 years ago
Depends on: 1363358
(Assignee)

Comment 5

2 years ago
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
Attachment #8866345 - Flags: feedback?(dao+bmo)
(Assignee)

Comment 6

2 years ago
Apologies for being a .patch rather than mozreview, will fix my hg setup when home
Attachment #8866345 - Attachment is obsolete: true
Attachment #8866345 - Flags: feedback?(dao+bmo)
Attachment #8866865 - Flags: review?(dao+bmo)
(Assignee)

Comment 7

2 years ago
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)
Attachment #8867295 - Flags: review?(dao+bmo)
(Reporter)

Updated

2 years ago
Attachment #8866865 - Attachment is obsolete: true
Attachment #8866865 - Flags: review?(dao+bmo)
(Assignee)

Comment 8

2 years ago
Refactored a little from feedback, restored font: icon and split out focused border style
Attachment #8867295 - Attachment is obsolete: true
Attachment #8867295 - Flags: review?(dao+bmo)
Attachment #8867309 - Flags: review?(dao+bmo)
(Assignee)

Comment 9

2 years ago
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)
It would be cool if I could place the home button between (or after) the forward and reload button, for example.
(Reporter)

Comment 11

2 years ago
(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.
(Assignee)

Comment 12

2 years ago
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
(Reporter)

Comment 13

2 years ago
(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.
(Reporter)

Comment 14

2 years ago
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 :)
Attachment #8867309 - Flags: review?(dao+bmo) → review+
A locationbar with "Nightly" or "(padlock) EV cert owner Corp." at the beginning has 1px more height than a normal one.
(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

2 years ago
Pushed by dharvey@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9d98f8c36399
Implement photon location bar style changes. r=dao
(Assignee)

Comment 18

2 years ago
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?
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
Flags: needinfo?(dale)
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.
(Assignee)

Comment 21

2 years ago
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
Flags: needinfo?(dale)
(Assignee)

Comment 22

2 years ago
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
Attachment #8867309 - Attachment is obsolete: true
(Assignee)

Comment 23

2 years ago
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
Attachment #8867627 - Flags: review?(dao+bmo)
(Reporter)

Updated

2 years ago
Attachment #8867627 - Flags: review?(dao+bmo) → review+

Comment 24

2 years ago
Pushed by dharvey@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2d66ad57d5c0
Implement photon location bar style changes. r=dao
(Assignee)

Comment 25

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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/2d66ad57d5c0
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Posted image graylocationbar.png
FWIW, locationbar and searchbar look now a bit odd in linux / Fedora 25.
Depends on: 1365275

Updated

2 years ago
No longer depends on: 1365509
Depends on: 1365789
(Reporter)

Updated

2 years ago
No longer depends on: 1365789
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]
(Reporter)

Comment 29

2 years ago
Thanks.
Status: RESOLVED → VERIFIED
Keywords: uiwanted
Flags: qe-verify+
(Reporter)

Updated

2 years ago
Depends on: 1366278

Updated

2 years ago
Depends on: 1366276
(Reporter)

Updated

2 years ago
No longer depends on: 1366276
(Reporter)

Updated

2 years ago
Depends on: 1367046
(Reporter)

Updated

2 years ago
Depends on: 1365906
(Reporter)

Updated

2 years ago
Depends on: 1365213
(Reporter)

Updated

2 years ago
Depends on: 1366492
(Reporter)

Updated

2 years ago
Depends on: 1363502
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.
Flags: needinfo?(shorlander)
(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.
Flags: needinfo?(shorlander)
(Reporter)

Updated

2 years ago
Depends on: 1382157
(Reporter)

Updated

2 years ago
Depends on: 1370401
Depends on: 1404355
You need to log in before you can comment on or make changes to this bug.