Color the title bar by default on Mac

RESOLVED FIXED in Firefox 56

Status

()

Firefox
Theme
P1
normal
RESOLVED FIXED
3 months ago
3 days ago

People

(Reporter: dao, Assigned: nhnt11)

Tracking

(Blocks: 1 bug)

Trunk
Firefox 56
All
Mac OS X
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify +

Firefox Tracking Flags

(firefox56 fixed)

Details

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

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

3 months ago
+++ This bug was initially created as a clone of Bug #1367384 +++

Mockup: https://people-mozilla.org/~shorlander/projects/photon/Mockups/macOS.html

Updated

3 months ago
Whiteboard: [photon-visual][p1][57] → [photon-visual][p1][57] [triage]

Updated

3 months ago
Flags: qe-verify?
Whiteboard: [photon-visual][p1][57] [triage] → [photon-visual][p1][57]
(Reporter)

Updated

3 months ago
Flags: qe-verify? → qe-verify+

Updated

3 months ago
QA Contact: brindusa.tot

Comment 1

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

Comment 2

3 months ago
(I'm mentioning this because the mockup seems to use the style of -moz-appearance: -moz-mac-vibrancy-dark;)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Updated

2 months ago
Assignee: nobody → nhnt11
Status: NEW → ASSIGNED
Iteration: --- → 56.1 - Jun 26
Priority: P2 → P1
(Assignee)

Comment 5

2 months 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 months 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 months 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 months 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 months 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 months 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 months 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 months 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 months ago
Attachment #8878038 - Flags: review+ → review?(dao+bmo)
(Assignee)

Updated

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

Comment 18

2 months 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 months 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 months 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
Depends on: 1374725
Backed out at dao's request:

https://hg.mozilla.org/integration/autoland/rev/138b6b411f4d
Flags: needinfo?(nhnt11)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 months ago
Attachment #8878039 - Attachment is obsolete: true
(Reporter)

Comment 23

2 months 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 months 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 months 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 months ago
Flags: needinfo?(nhnt11)
(Reporter)

Comment 28

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

Comment 35

2 months 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: 1375973

Updated

2 months ago
Depends on: 1375975

Updated

2 months ago
Depends on: 1376313

Updated

2 months ago
Depends on: 1376413
(Assignee)

Updated

2 months ago
Depends on: 1377284
(Reporter)

Updated

a month ago
Depends on: 1379981
QA Contact: brindusa.tot → ovidiu.boca

Comment 36

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

3 days 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)
You need to log in before you can comment on or make changes to this bug.