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

VERIFIED FIXED in Firefox 56

Status

()

enhancement
P1
normal
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: Gijs, Assigned: daleharvey)

Tracking

(Blocks 1 bug)

53 Branch
Firefox 56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed, firefox57 verified)

Details

(Whiteboard: [photon-visual][p3])

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

2 years ago
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?
Flags: qe-verify+
Flags: needinfo?(shorlander)
Whiteboard: [photon-visual] → [photon-visual] [triage]
Priority: -- → P2
Whiteboard: [photon-visual] [triage] → [photon-visual]

Comment 1

2 years ago
Shouldn't the find bar get the same treatment ?
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.
Whiteboard: [photon-visual] → [photon-visual][p3]
(Assignee)

Updated

2 years ago
Assignee: nobody → dale
Status: NEW → ASSIGNED
Iteration: --- → 55.7 - Jun 12
Priority: P2 → P1
Iteration: 55.7 - Jun 12 → 56.1 - Jun 26
Comment hidden (mozreview-request)

Comment 4

2 years ago
mozreview-review
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.
Attachment #8877545 - Flags: review?(dao+bmo) → review-
(Assignee)

Updated

2 years ago
Depends on: 1349555
Comment hidden (mozreview-request)
(Assignee)

Comment 6

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

Comment 8

2 years ago
mozreview-review
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).
Attachment #8877545 - Flags: review?(dao+bmo)
(Assignee)

Comment 9

2 years ago
> 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 hidden (mozreview-request)
Iteration: 56.1 - Jun 26 → 56.2 - Jul 10

Comment 11

2 years ago
mozreview-review
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.
Attachment #8877545 - Flags: review?(dao+bmo) → review+
(Assignee)

Comment 12

2 years ago
Posted patch toolbar-bg.patch (obsolete) — Splinter Review
Updated to use Gij's --toolbar-background variable and to not package noise.png on non photon
Attachment #8877545 - Attachment is obsolete: true
Attachment #8882287 - Flags: review?(dao+bmo)
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
Attachment #8882287 - Flags: review?(dao+bmo) → review+
(Assignee)

Comment 14

2 years ago
Posted patch toolbar-bg.patch (obsolete) — Splinter Review
Yup noticed try go in flames, thanks
Attachment #8882287 - Attachment is obsolete: true
Attachment #8882346 - Flags: review+
(Assignee)

Comment 15

2 years ago
Could sheriffs land this please as I forgot my laptop and am not setup to push, thank you
Keywords: checkin-needed
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 -
Flags: needinfo?(dale)
(Assignee)

Comment 18

2 years ago
Fixed unreferenced file
Attachment #8882346 - Attachment is obsolete: true
Flags: needinfo?(dale)

Comment 21

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/eb62dc6dcf5f
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56

Updated

2 years ago
Depends on: 1378843
No longer depends on: 1378843
Depends on: 1379230
Hi Dale,

Please tell us what do we need to verify here, thanks.
Flags: needinfo?(dharvey)
(Assignee)

Comment 23

2 years ago
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
Flags: needinfo?(dharvey)
QA Contact: brindusa.tot → ovidiu.boca
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?
Flags: needinfo?(dharvey)
(Assignee)

Comment 25

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

Comment 28

2 years ago
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
Flags: needinfo?(dharvey)
Thanks for clearing this out, I will mark this as verified fixed.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.