Color the title bar by default on Mac

VERIFIED FIXED in Firefox 56

Status

()

defect
P1
normal
VERIFIED FIXED
2 years ago
Last year

People

(Reporter: dao, Assigned: nhnt11)

Tracking

(Blocks 1 bug)

Trunk
Firefox 56
All
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed, firefox57 verified)

Details

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

Attachments

(1 attachment, 1 obsolete attachment)

Reporter

Description

2 years ago
+++ 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]
Reporter

Updated

2 years ago
Flags: qe-verify? → qe-verify+
QA Contact: brindusa.tot

Comment 1

2 years ago
Vibrancy was added to macOS' titlebar in bug 1052466, but then it was backed out in bug 1161565 :/

Comment 2

2 years ago
(I'm mentioning this because the mockup seems to use the style of -moz-appearance: -moz-mac-vibrancy-dark;)
Comment hidden (mozreview-request)
Assignee: nobody → nhnt11
Status: NEW → ASSIGNED
Iteration: --- → 56.1 - Jun 26
Priority: P2 → P1
Assignee

Comment 5

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

Comment 6

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

Comment 7

2 years ago
(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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Comment 10

2 years ago
mozreview-review-reply
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.
Assignee

Comment 11

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

Comment 12

2 years ago
mozreview-review
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+
Reporter

Comment 13

2 years ago
mozreview-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 hidden (mozreview-request)

Comment 15

2 years ago
mozreview-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).
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Updated

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

Updated

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

Comment 18

2 years ago
mozreview-review
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+
Reporter

Comment 19

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

Comment 20

2 years ago
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)
Comment hidden (mozreview-request)
Assignee

Updated

2 years ago
Attachment #8878039 - Attachment is obsolete: true
Reporter

Comment 23

2 years ago
mozreview-review
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-
Reporter

Comment 24

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

Comment 26

2 years ago
mozreview-review
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
Comment hidden (mozreview-request)
Assignee

Updated

2 years ago
Flags: needinfo?(nhnt11)
Reporter

Comment 28

2 years ago
mozreview-review
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 29

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

Comment 30

2 years ago
(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.
Comment hidden (mozreview-request)

Comment 32

2 years ago
Pushed by nhnt11@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/2bc059a92b04
Color the title bar by default on Mac. r=dao

Comment 33

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/2bc059a92b04
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
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)
Assignee

Comment 35

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

Updated

2 years ago
Depends on: 1376313

Updated

2 years ago
Depends on: 1376413
Assignee

Updated

2 years ago
Depends on: 1377284
Reporter

Updated

2 years ago
Depends on: 1379981
QA Contact: brindusa.tot → ovidiu.boca

Comment 36

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

Comment 37

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

Comment 38

2 years ago
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
Flags: qe-verify+
Depends on: 1428547
You need to log in before you can comment on or make changes to this bug.