Closed Bug 1367385 Opened 7 years ago Closed 7 years ago

Color the title bar by default on Mac

Categories

(Firefox :: Theme, defect, P1)

Product:
Firefox
Component:
Theme
Platform:
All
macOS
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 56
Iteration:
56.1 - Jun 26
Tracking Flags:
Tracking Status
firefox56 --- fixed
firefox57 --- verified

People

(Reporter: dao, Assigned: nhnt11)

References

(Blocks 1 open bug)

Details

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

Attachments

(1 file, 1 obsolete file)

Bug 1367385 - Color the title bar by default on Mac.
59 bytes, text/x-review-board-request
dao
: review+
Details 
+++ This bug was initially created as a clone of Bug #1367384 +++

Mockup: https://people-mozilla.org/~shorlander/projects/photon/Mockups/macOS.html
Whiteboard: [photon-visual][p1][57] → [photon-visual][p1][57] [triage]
Flags: qe-verify?
Whiteboard: [photon-visual][p1][57] [triage] → [photon-visual][p1][57]
Flags: qe-verify? → qe-verify+
QA Contact: brindusa.tot
Vibrancy was added to macOS' titlebar in bug 1052466, but then it was backed out in bug 1161565 :/
(I'm mentioning this because the mockup seems to use the style of -moz-appearance: -moz-mac-vibrancy-dark;)
Assignee: nobody → nhnt11
Status: NEW → ASSIGNED
Iteration: --- → 56.1 - Jun 26
Priority: P2 → P1
The patch colors the tabs toolbar, which has the effect of coloring the titlebar on Mac.

- This does not at present seem to affect double-click-to-maximize functionality in the empty areas of the tabs toolbar.

- I can't seem to color the actual title bar when it's enabled, it seems to use the "OS provided titlebar" for want of a more technical term.

- Interestingly the "Title bar" checkbox in customization mode and the browser.tabs.drawInTitlebar pref seem to have different effects when toggled, though their states are coupled. Toggling the checkbox results in an "OS provided titlebar" for me, and toggling the pref in about:config results in a titlebar whose styling we appear to have control of. I'm investigating, but maybe we should address this in a follow-up bug.
Comment on attachment 8878039 [details]
Bug 1367385 - Use vibrancy background for titlebar in compact themes.

https://reviewboard.mozilla.org/r/149454/#review154070

::: commit-message-d9315:1
(Diff revision 1)
> +Bug 1367385 - Use vibrancy background for titlebar in lwthemes. r=dao

It's not clear to me what this patch is actually about. If it's about lwthemes as claimed in the summary, then this doesn't belong in compacttheme.css but browser.css.
Attachment #8878039 - Flags: review?(dao+bmo)
(In reply to Nihanth Subramanya [:nhnt11] from comment #5)
> The patch colors the tabs toolbar, which has the effect of coloring the
> titlebar on Mac.

Why is this better than using #titlebar?
Comment on attachment 8878039 [details]
Bug 1367385 - Use vibrancy background for titlebar in compact themes.

https://reviewboard.mozilla.org/r/149454/#review154070

> It's not clear to me what this patch is actually about. If it's about lwthemes as claimed in the summary, then this doesn't belong in compacttheme.css but browser.css.

Eh, it's for the compact themes. Hasty commit message. Sorry.
(In reply to Dão Gottwald [::dao] from comment #7)
> (In reply to Nihanth Subramanya [:nhnt11] from comment #5)
> > The patch colors the tabs toolbar, which has the effect of coloring the
> > titlebar on Mac.
> 
> Why is this better than using #titlebar?

Hmm, seems like it's not; double-click-to-maximize still works. I updated the patch.

----------------------------------------------

So, there seems to be a bug: toggling the title bar from customization mode behaves differently from flipping the pref in about:config. Gijs pointed me to a possible place to start investigating this. I'm going to file another bug for it, and meanwhile the patches in this bug only have effect for [tabsintitlebar].
Comment on attachment 8878038 [details]
Bug 1367385 - Color the title bar by default on Mac.

https://reviewboard.mozilla.org/r/149452/#review154392
Attachment #8878038 - Flags: review?(dao+bmo) → review+
Comment on attachment 8878039 [details]
Bug 1367385 - Use vibrancy background for titlebar in compact themes.

https://reviewboard.mozilla.org/r/149454/#review154394

::: browser/themes/osx/compacttheme.css:38
(Diff revisions 1 - 2)
>  }
>  
> -#tabbrowser-tabs,
> -#TabsToolbar {
> +#main-window[tabsintitlebar] > #titlebar {
> +  -moz-appearance: -moz-mac-vibrancy-dark;
> -  background: none;
>  }

The order of the above two rules is backwards. The one overriding the other's -moz-appearance with the more specific selector should come second to make this code more easily comprehensible.
Attachment #8878039 - Flags: review?(dao+bmo) → review+
Comment on attachment 8878038 [details]
Bug 1367385 - Color the title bar by default on Mac.

https://reviewboard.mozilla.org/r/149452/#review154462

::: browser/themes/osx/browser.css:124
(Diff revision 2)
>  #main-window:not(:-moz-lwtheme) > #titlebar {
> +%ifdef MOZ_PHOTON_THEME
> +  -moz-appearance: -moz-mac-vibrancy-dark;
> +%else
>    -moz-appearance: -moz-window-titlebar;
> +%endif

-moz-mac-vibrancy-dark doesn't work on OSX versions lower than 10.10 (Yosemite).
Attachment #8878038 - Flags: review+ → review?(dao+bmo)
Attachment #8878039 - Flags: review+ → review?(dao+bmo)
Comment on attachment 8878038 [details]
Bug 1367385 - Color the title bar by default on Mac.

https://reviewboard.mozilla.org/r/149452/#review155074
Attachment #8878038 - Flags: review?(dao+bmo) → review+
Comment on attachment 8878039 [details]
Bug 1367385 - Use vibrancy background for titlebar in compact themes.

https://reviewboard.mozilla.org/r/149454/#review155076
Attachment #8878039 - Flags: review?(dao+bmo) → review+
Pushed by nhnt11@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/2f7a963edf85
Color the title bar by default on Mac. r=dao
https://hg.mozilla.org/integration/autoland/rev/d521c1d8d605
Use vibrancy background for titlebar in compact themes. r=dao
Backed out at dao's request:

https://hg.mozilla.org/integration/autoland/rev/138b6b411f4d
Flags: needinfo?(nhnt11)
Attachment #8878039 - Attachment is obsolete: true
Comment on attachment 8878038 [details]
Bug 1367385 - Color the title bar by default on Mac.

https://reviewboard.mozilla.org/r/149452/#review156702

::: browser/themes/osx/browser.css:1537
(Diff revision 4)
>    border: none;
>  }
>  
>  .tabbrowser-tab[visuallyselected=true]:not(:-moz-lwtheme) {
>    /* overriding tabbox.css */
> -  color: inherit;
> +  color: var(--tabs-toolbar-selected-tab-color);

This should remain inherit. You need to set the text color on #TabsToolbar, which will also fix icon colors.
Attachment #8878038 - Flags: review+ → review-
Comment on attachment 8878038 [details]
Bug 1367385 - Color the title bar by default on Mac.

https://reviewboard.mozilla.org/r/149452/#review156716

::: browser/themes/osx/browser.css:1537
(Diff revision 4)
>    border: none;
>  }
>  
>  .tabbrowser-tab[visuallyselected=true]:not(:-moz-lwtheme) {
>    /* overriding tabbox.css */
> -  color: inherit;
> +  color: var(--tabs-toolbar-selected-tab-color);

<nhnt11> dao: I'm only explicitly changing the text color of the *selected* tab
The icons are already fixed
because I changed the --tabs-toolbar-color value or whatever
re. your review some minutes ago
is there a better way to do this
<dao> oh, --tabs-toolbar-color
<dao> let's get rid of this, it's only used once
<nhnt11> sure
<dao> and let's not add --tabs-toolbar-selected-tab-color either
Comment on attachment 8878038 [details]
Bug 1367385 - Color the title bar by default on Mac.

https://reviewboard.mozilla.org/r/149452/#review156732

::: browser/themes/osx/browser.css
(Diff revision 5)
>  @namespace svg url("http://www.w3.org/2000/svg");
>  
>  %include ../shared/browser.inc.css
>  
>  :root {
> -  --tabs-toolbar-color: #333;

Please remove this too: http://searchfox.org/mozilla-central/rev/ae94cfb36d804727dfb38f2750bfcaab4621df6e/browser/themes/shared/compacttheme.inc.css#35

(I believe that line didn't do anything since we only used --tabs-toolbar-color for :not(:-moz-lwtheme), which excludes compact themes.)

::: browser/themes/osx/browser.css:124
(Diff revision 5)
>    margin-left: 7px;
>  }
>  
>  #main-window:not(:-moz-lwtheme) > #titlebar {
> +%ifdef MOZ_PHOTON_THEME
> +  -moz-appearance: none;

You can drop -moz-appearance: none, it's the default.

And please drop this rule from photon: http://searchfox.org/mozilla-central/rev/ae94cfb36d804727dfb38f2750bfcaab4621df6e/browser/themes/osx/browser.css#165-167

::: browser/themes/osx/browser.css:125
(Diff revision 5)
>  }
>  
>  #main-window:not(:-moz-lwtheme) > #titlebar {
> +%ifdef MOZ_PHOTON_THEME
> +  -moz-appearance: none;
> +  background: rgba(0, 0, 0, .85);

nit: use background-color
Flags: needinfo?(nhnt11)
Comment on attachment 8878038 [details]
Bug 1367385 - Color the title bar by default on Mac.

https://reviewboard.mozilla.org/r/149452/#review156782

::: browser/themes/osx/browser.css:1572
(Diff revision 6)
> -  color: var(--tabs-toolbar-color);
> +  color: #333;
>    text-shadow: @loweredShadow@;
>  }
>  
> +%ifdef MOZ_PHOTON_THEME
> +#main-window[tabsintitlebar] #TabsToolbar:not(:-moz-lwtheme) {

please use :root instead of #main-window
Attachment #8878038 - Flags: review?(dao+bmo) → review+
Comment on attachment 8878038 [details]
Bug 1367385 - Color the title bar by default on Mac.

https://reviewboard.mozilla.org/r/149452/#review156788

::: browser/themes/osx/browser.css:124
(Diff revision 6)
>    margin-left: 7px;
>  }
>  
>  #main-window:not(:-moz-lwtheme) > #titlebar {
> +%ifdef MOZ_PHOTON_THEME
> +  background-color: rgba(0, 0, 0, .85);

Not sure why this is an alpha color, transparency doesn't even work (you can't see what's behind the window).

Even if it did, -moz-mac-vibrancy-dark also includes a blur effect, and a few extra native filters.
(In reply to Tim Nguyen :ntim from comment #29)
> Comment on attachment 8878038 [details]
> Bug 1367385 - Color the title bar by default on Mac.
> 
> https://reviewboard.mozilla.org/r/149452/#review156788
> 
> ::: browser/themes/osx/browser.css:124
> (Diff revision 6)
> >    margin-left: 7px;
> >  }
> >  
> >  #main-window:not(:-moz-lwtheme) > #titlebar {
> > +%ifdef MOZ_PHOTON_THEME
> > +  background-color: rgba(0, 0, 0, .85);
> 
> Not sure why this is an alpha color, transparency doesn't even work (you
> can't see what's behind the window).

It lets through part of the window background. Could reach the same result with a lighter opaque color.
Pushed by nhnt11@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/2bc059a92b04
Color the title bar by default on Mac. r=dao
https://hg.mozilla.org/mozilla-central/rev/2bc059a92b04
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
(In reply to Nihanth Subramanya [:nhnt11] from comment #22)
> Comment on attachment 8878038 [details]
> Bug 1367385 - Color the title bar by default on Mac.
> 
> Review request updated; see interdiff: https://reviewboard.mozilla.org/r/149452/diff/3-4/

In this update to the patch you silently removed the vibrancy style. I'm curious about the reasons for this change. Was it because the requirements from UX changed, or because you were hitting bugs, or because of something else?
Flags: needinfo?(nhnt11)
(In reply to Markus Stange [:mstange] from comment #34)
> (In reply to Nihanth Subramanya [:nhnt11] from comment #22)
> > Comment on attachment 8878038 [details]
> > Bug 1367385 - Color the title bar by default on Mac.
> > 
> > Review request updated; see interdiff: https://reviewboard.mozilla.org/r/149452/diff/3-4/
> 
> In this update to the patch you silently removed the vibrancy style. I'm
> curious about the reasons for this change. Was it because the requirements
> from UX changed, or because you were hitting bugs, or because of something
> else?

The original patch was backed out due to talos regressions (see bug 1374725) and we decided to land a solid colour for the time being. I want to do some digging/talking to find out if we can land vibrancy eventually.
Flags: needinfo?(nhnt11)
Depends on: 1375975
Depends on: 1376313
Depends on: 1376413
Depends on: 1377284
Depends on: 1379981
QA Contact: brindusa.tot → ovidiu.boca
Tested on latest Nightly 57.0a1 (2017-08-16) (64-bit) on Mac OXS 10.10 and 10.12.5 
When the Title bar box is checked, on some Themes (Dark and Space Fantasy) the title bar is barely visible. See also issue  1390885 for this matter, as I understood that the Default Color of Title Bar is directly connected with the Theme used. Thank you
Flags: needinfo?(nhnt11)
(In reply to Deac Alin from comment #36)
> Tested on latest Nightly 57.0a1 (2017-08-16) (64-bit) on Mac OXS 10.10 and
> 10.12.5 
> When the Title bar box is checked, on some Themes (Dark and Space Fantasy)
> the title bar is barely visible. See also issue  1390885 for this matter, as
> I understood that the Default Color of Title Bar is directly connected with
> the Theme used. Thank you

Filed bug 1392219 for this.
Flags: needinfo?(nhnt11)
Considering that the issue I detected it's addressed in another bug (1392219), we can mark this as verified fixed. Thank you
Status: RESOLVED → VERIFIED
status-firefox57: --- → verified
Flags: qe-verify+
Depends on: 1428547
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: