1367439 - Update toolbar background colors on OS X and Windows, update customize mode background to match

Status: VERIFIED FIXED

(Firefox :: Theme, enhancement, P1)

Description

[:Gijs (he/him)]

The interactive mockups have a fixed colour for both the OS X and Windows toolbar background. We should use this (except on Windows hcm, which I guess will end up meaning 'except on Windows non-default themes') for:

- the navbar
- the bookmarks toolbar
- the tabs
- customize mode toolbox / background
- customize mode footer

Right now the customize mode footer uses -moz-dialog as a background colour, which on Windows (but not OS X) is the same as what we use for the toolbars.

Stephen: what would you expect to happen on Linux?

Comment 1

[u428464]

Shouldn't the find bar get the same treatment ?

Comment 2

[Dão Gottwald [:dao]]

On Mac we're currently using hsl(0,0%,83%):
http://searchfox.org/mozilla-central/rev/b318c7dca7392bd16c0b11929f55b1be133f0b31/browser/themes/osx/browser.css#169

On Windows and Linux we're using rgba(255,255,255,.4) on top of -moz-dialog:
http://searchfox.org/mozilla-central/rev/b318c7dca7392bd16c0b11929f55b1be133f0b31/browser/themes/windows/browser.css#139
http://searchfox.org/mozilla-central/rev/b318c7dca7392bd16c0b11929f55b1be133f0b31/browser/themes/linux/browser.css#74

Not sure how far these colors are off from the mockups (I think it's pretty subtle?) and what they should be changed to. Stephen, please specify precisely.

Comment 3

[Dale Harvey (:daleharvey)]

Attachment: Bug 1367439 - Update toolbar background.

Review commit: https://reviewboard.mozilla.org/r/149010/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/149010/

Comment 4

[Dão Gottwald [:dao]]

Comment on attachment 8877545 [details]
Bug 1367439 - Update toolbar background.

https://reviewboard.mozilla.org/r/149010/#review153466

::: browser/themes/windows/browser.css:141
(Diff revision 1)
>    border-bottom-color: rgba(0,0,0,.3);
>  }
>  
> +%ifdef MOZ_PHOTON_THEME
> +#nav-bar {
> +  background-color: -moz-Dialog;

We need to consistently use the same color for the selected tab, the navigation toolbar and any following toolbars (i.e. basically the bookmarks toolbar). It doesn't look like this patch will do that.

Comment 5

[Dale Harvey (:daleharvey)]

Comment on attachment 8877545 [details]
Bug 1367439 - Update toolbar background.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/149010/diff/1-2/

Comment 6

[Dale Harvey (:daleharvey)]

As discussed on IRC didnt change the selected tab colour as it looks obviously broken until the tab curves etc are also changed (https://bugzilla.mozilla.org/show_bug.cgi?id=1349555). I also didnt change the customise mode as we dont have any specs here so seems best to put that in a new seperate bug.

Comment 7

[Stephen Horlander [:shorlander]]

(In reply to :Gijs from comment #0) 
> Stephen: what would you expect to happen on Linux?

I believe this is all documented in the InVision spec. Do we still need any clarification here? Thanks!

Comment 8

[Dão Gottwald [:dao]]

Comment on attachment 8877545 [details]
Bug 1367439 - Update toolbar background.

https://reviewboard.mozilla.org/r/149010/#review155568

::: browser/themes/osx/browser.css:213
(Diff revision 2)
>  }
>  
> +%ifndef MOZ_PHOTON_THEME
>  #nav-bar {
>    -moz-appearance: none;
>    background: url(chrome://browser/skin/Toolbar-background-noise.png),

This removes Toolbar-background-noise.png except for lightweight themes: http://searchfox.org/mozilla-central/rev/2bcd258281da848311769281daf735601685de2d/browser/themes/osx/browser.css#278

Do we still need this?

::: browser/themes/windows/browser.css:141
(Diff revision 2)
>    border-bottom-color: rgba(0,0,0,.3);
>  }
>  
> +%ifdef MOZ_PHOTON_THEME
> +#navigator-toolbox > toolbar:not(#toolbar-menubar):not(#TabsToolbar) {
> +  background-color: -moz-Dialog;

Does this work for lightweight themes? The non-photon rule uses :not(:-moz-lwtheme).

Comment 9

[Dale Harvey (:daleharvey)]

> This removes Toolbar-background-noise.png except for lightweight 
> themes: 

> Do we still need this?

Nope missed this

> Does this work for lightweight themes? The non-photon rule uses :not(:-moz-lwtheme).

Was a little confused about what we should do for lightweight themes, the plain background seemed like a suitable default? but either way this is overridden by http://searchfox.org/mozilla-central/source/toolkit/themes/osx/global/toolbar.css#20 so keeping the :not is probably for the best

Comment 10

[Dale Harvey (:daleharvey)]

Comment on attachment 8877545 [details]
Bug 1367439 - Update toolbar background.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/149010/diff/2-3/

Comment 11

[Dão Gottwald [:dao]]

Comment on attachment 8877545 [details]
Bug 1367439 - Update toolbar background.

https://reviewboard.mozilla.org/r/149010/#review158120

::: browser/themes/osx/browser.css:288
(Diff revision 3)
>  }
>  
> +%ifndef MOZ_PHOTON_THEME
>  #navigator-toolbox > toolbar:not(#TabsToolbar):-moz-lwtheme {
>    background-color: @toolbarColorLWT@;
>    background-image: url(chrome://browser/skin/Toolbar-background-noise.png);

Since Toolbar-background-noise.png is unused now, you'll probably get a failure in browser_all_files_referenced.js. Please package the file conditional on #ifndef MOZ_PHOTON_THEME.

Comment 12

[Dale Harvey (:daleharvey)]

Attachment: toolbar-bg.patch

Updated to use Gij's --toolbar-background variable and to not package noise.png on non photon

Comment 13

[Dão Gottwald [:dao]]

Comment on attachment 8882287 [details] [diff] [review]
toolbar-bg.patch

>--- a/browser/themes/windows/browser.css
>+++ b/browser/themes/windows/browser.css
>@@ -131,16 +131,21 @@ toolbar[brighttext] {
>     }
>   }
> }
> 
> #navigator-toolbox:-moz-lwtheme::after {
>   border-bottom-color: rgba(0,0,0,.3);
> }
> 
>+%ifdef MOZ_PHOTON_THEME
>+#navigator-toolbox > toolbar:not(#toolbar-menubar):not(#TabsToolbar):not(:-moz-lwtheme) {
>+  background-color: var(--toolbar-background-color);
>+}
>+%elseif

%else

Comment 14

[Dale Harvey (:daleharvey)]

Attachment: toolbar-bg.patch

Yup noticed try go in flames, thanks

Comment 15

[Dale Harvey (:daleharvey)]

Could sheriffs land this please as I forgot my laptop and am not setup to push, thank you

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4bad6a20f428
Update toolbar background. r=dao

Comment 17

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

Backed out for failing browser_all_files_referenced.js:

https://hg.mozilla.org/integration/mozilla-inbound/rev/b2efbed661d39531a1c15bde1aad5a933aff51d2

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=4bad6a20f428ef145ad21554c11450f2b15e6c48&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=110996684&repo=mozilla-inbound

17:56:49     INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/static/browser_all_files_referenced.js | there should be no unreferenced files - Got 1, expected 0
17:56:49     INFO - Stack trace:
17:56:49     INFO - chrome://mochikit/content/browser-test.js:test_is:967
17:56:49     INFO - chrome://mochitests/content/browser/browser/base/content/test/static/browser_all_files_referenced.js:checkAllTheFiles:609
17:56:49     INFO - Not taking screenshot here: see the one that was previously logged
17:56:49     INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/static/browser_all_files_referenced.js | unreferenced file: chrome://browser/skin/Toolbar-background-noise.png -

Comment 18

[Dale Harvey (:daleharvey)]

Attachment: toolbar-bg.patch

Fixed unreferenced file

Comment 19

[Dale Harvey (:daleharvey)]

Try run @ https://treeherder.mozilla.org/#/jobs?repo=try&revision=bf755acce0e856ea75647e32fd10b34aa85023ff&selectedJob=111149818 (failures unrelated)

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/eb62dc6dcf5f
Update toolbar background. r=dao

Comment 21

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/eb62dc6dcf5f

Comment 22

[ovidiu boca[:Ovidiu]]

Hi Dale,

Please tell us what do we need to verify here, thanks.

Comment 23

[Dale Harvey (:daleharvey)]

Hi Ovidiu

In this bug we changed the toolback background color (bookmarks toolbar and the area behind the buttons + url bar), we should verify that it matches the spec @ http://design.firefox.com/people/shorlander/photon/Mockups/macOS.html

Cheers

Comment 24

[ovidiu boca[:Ovidiu]]

Thanks Dale, I verified this on Mac OS X 10.10 and I compared the latest Nightly 57.0a1(2017-08-16) with the specs and the colors don't match here are some examples:

- background color from URL bar- Spec color: #FEFEFE        Nightly color: #FFFFFF
- the color behind buttons-      Spec color: #FAFAFB        Nightly color: #F9F9FA
- Bookmark toolbar background-   Spec color: #FAFAFB        Nightly color: #F9F9FA 

Please tell me your opinion, and also 1 more question, this bug tracks changes that were made only on Mac or do I need to verify on all systems?

Comment 25

[Dale Harvey (:daleharvey)]

> - background color from URL bar- Spec color: #FEFEFE        Nightly color: #FFFFFF

The URL bar uses an inherited OS colour (moz-Field) here so may not match the spec entirely, the url bar in the spec uses a white with transparency over an almost white so computers to pretty much white anyway

> - the color behind buttons-      Spec color: #FAFAFB        Nightly color: #F9F9FA
> - Bookmark toolbar background-   Spec color: #FAFAFB        Nightly color: #F9F9FA 

Not sure how you are taking the spec colour here, the spec is hsl(240, 9%, 98%) which is rgb #F9F9FA

Comment 26

[ovidiu boca[:Ovidiu]]

I verified this on Windows 10 and Mac OS 10.10 and I can confirm the fix. On Windows 7 when I select the Dark, Default or Light theme the tab bar doesn't change its color, but if I select the "Recommended" themes the tab bar changes. This is intended?

Comment 27

[ovidiu boca[:Ovidiu]]

Attachment: Windows 7 tab bar.png

Comment 28

[Dale Harvey (:daleharvey)]

I dont think thats intended, I vaguelly remember seeing a problem with windows theme switching updates but cant find it now, and certainly not related to this bug

Comment 29

[ovidiu boca[:Ovidiu]]

Thanks for clearing this out, I will mark this as verified fixed.