Stephen to provide UX spec.

bug 1352164 comment 1 may belong to this bug

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.

(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.

(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.

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

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

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)

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

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.

(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.

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

(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 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 :)

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?

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

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

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.

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

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 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

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

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

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

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]

Thanks.

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.

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.

(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.