1424422 - [CSD] window buttons in title bar always black even with a dark LW theme

Status: RESOLVED DUPLICATE of bug 1718846

(Core :: Widget: Gtk, defect, P2)

Description

[:aceman]

Even with a dark LW theme installed, the window buttons drawn in title bar with CSD enabled are always black, thus barely visible at all. They only become visible when hovered because they then get a light background.

They should probably get the menu label color which in a dark theme is tailored to a light color.

Comment 1

[:aceman]

Attachment: screenshot

Comment 2

[Martin Stránský [:stransky] (ni? me)]

Attachment: Bug 1424422 - Use built-in icons on titlebar on lwt themes,

When lwt themes are enabled we need to draw titlebar controls as windows platform does.

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

Comment 3

[Martin Stránský [:stransky] (ni? me)]

We depend on Bug 1408335. 

I also wonder if we want to follow system button sizes when lwt theme is enabled.

Comment 5

[Dão Gottwald [:dao]]

Comment on attachment 8941797 [details]
Bug 1424422 - Use built-in icons on titlebar on lwt themes,

https://reviewboard.mozilla.org/r/212046/#review222658

::: browser/themes/linux/browser.css:750
(Diff revision 1)
> +  .titlebar-button:-moz-lwtheme {
> +    border: none;
> +    margin: 0 !important;
> +    padding: 8px 17px;
> +    -moz-context-properties: stroke;
> +    stroke: var(--titlebar-text-color);

This variable isn't set, and you don't seem to be using context-stroke in the SVGs...

::: browser/themes/linux/browser.css:776
(Diff revision 1)
>    @media (-moz-gtk-csd-maximize-button) {
> -    #titlebar-max {
> +    #titlebar-max:not(:-moz-lwtheme) {
>        -moz-appearance: -moz-window-button-maximize;
>      }
> -    :root[sizemode="maximized"] #titlebar-max {
> +    #titlebar-max:-moz-lwtheme {
> +      list-style-image: url(chrome://browser/skin/window-controls/maximize-themes.svg);

It seems that this makes the title bar taller than it should be. Tested with our Dark and Light themes on Ubuntu.

Comment 6

[Martin Stránský [:stransky] (ni? me)]

Comment on attachment 8941797 [details]
Bug 1424422 - Use built-in icons on titlebar on lwt themes,

https://reviewboard.mozilla.org/r/212046/#review222664

::: browser/themes/linux/browser.css:776
(Diff revision 1)
>    @media (-moz-gtk-csd-maximize-button) {
> -    #titlebar-max {
> +    #titlebar-max:not(:-moz-lwtheme) {
>        -moz-appearance: -moz-window-button-maximize;
>      }
> -    :root[sizemode="maximized"] #titlebar-max {
> +    #titlebar-max:-moz-lwtheme {
> +      list-style-image: url(chrome://browser/skin/window-controls/maximize-themes.svg);

You're right, this is caused by:
https://bugzilla.mozilla.org/show_bug.cgi?id=1419442#c3

which we should fix there. The titlebar height is a quite random right now and depends on actual Gtk+ theme which defines titlebar button size.

Comment 8

[Martin Stránský [:stransky] (ni? me)]

AFAIK There's one thing missing at this patch - when button is selected the button is rendered as standard/themed button. Windows use plain red color in that case and we'd need something similar.

Comment 9

[Martin Stránský [:stransky] (ni? me)]

Comment on attachment 8941797 [details]
Bug 1424422 - Use built-in icons on titlebar on lwt themes,

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

Comment 10

[Martin Stránský [:stransky] (ni? me)]

Comment on attachment 8941797 [details]
Bug 1424422 - Use built-in icons on titlebar on lwt themes,

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

Comment 11

[Martin Stránský [:stransky] (ni? me)]

Comment on attachment 8941797 [details]
Bug 1424422 - Use built-in icons on titlebar on lwt themes,

https://reviewboard.mozilla.org/r/212046/#review222658

> It seems that this makes the title bar taller than it should be. Tested with our Dark and Light themes on Ubuntu.

Should be fixed now.

Comment 13

[Dão Gottwald [:dao]]

Comment on attachment 8941797 [details]
Bug 1424422 - Use built-in icons on titlebar on lwt themes,

https://reviewboard.mozilla.org/r/212046/#review228668

::: browser/themes/linux/browser.css:696
(Diff revision 3)
> +    padding: 8px 17px;
> +  }
> +  .titlebar-button:-moz-lwtheme > .toolbarbutton-icon {
> +    width: 12px;
> +    height: 12px;
> +  }

Looks like we'll need to call TabsInTitlebar.updateAppearance in response to the lightweight-theme-styling-update notification, otherwise the space reserved for the buttons might be off.

Comment 14

[Martin Stránský [:stransky] (ni? me)]

Comment on attachment 8941797 [details]
Bug 1424422 - Use built-in icons on titlebar on lwt themes,

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

Comment 15

[Martin Stránský [:stransky] (ni? me)]

Comment on attachment 8941797 [details]
Bug 1424422 - Use built-in icons on titlebar on lwt themes,

https://reviewboard.mozilla.org/r/212046/#review228668

> Looks like we'll need to call TabsInTitlebar.updateAppearance in response to the lightweight-theme-styling-update notification, otherwise the space reserved for the buttons might be off.

Fixed, thanks.

Comment 16

[Martin Stránský [:stransky] (ni? me)]

Dao, do you think we can get this to Firefox 60 which will have enabled the titlebar rendering?

Comment 18

[Dão Gottwald [:dao]]

(In reply to Martin Stránský [:stransky] from comment #3)
> I also wonder if we want to follow system button sizes when lwt theme is
> enabled.

This would be nice, such that 1) the UI doesn't jiggle when applying a theme and 2) we're more consistent with other applications.

I'm not sure how to proceed here. Do we have a sense of how many Gtk themes are affected? The Ubuntu 18.04 default theme's window controls work just fine with the dark theme and other lightweight themes.

Comment 19

[Ondrej Zoder [:ozoder]]

Attachment: Bug 1424422 - Use built-in icons on titlebar on lwt themes

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

Comment 20

[Ondrej Zoder [:ozoder]]

Updated patch for current trunk.

Comment 21

[Dão Gottwald [:dao]]

Comment on attachment 8992892 [details]
Bug 1424422 - Use built-in icons on titlebar on lwt themes

https://reviewboard.mozilla.org/r/257722/#review264640

This doesn't seem to build:

 0:15.74 make[5]: *** /home/dao/mozilla/central/objdir-frontend/browser/themes/linux/window-controls/: No such file or directory.  Stop.
 0:15.74 /home/dao/mozilla/central/config/faster/rules.mk:74: recipe for target '/home/dao/mozilla/central/objdir-frontend/browser/themes/linux/window-controls/close-themes.svg' failed

Also please see my previous comment.

Comment 22

[Ondrej Zoder [:ozoder]]

Comment on attachment 8992892 [details]
Bug 1424422 - Use built-in icons on titlebar on lwt themes

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

Comment 23

[Dão Gottwald [:dao]]

Comment on attachment 8992892 [details]
Bug 1424422 - Use built-in icons on titlebar on lwt themes

https://reviewboard.mozilla.org/r/257722/#review264668

Have you missed comment 18? I'm still not convinced that this is the right approach.

Comment 24

[Dão Gottwald [:dao]]

Aside from that, I'm getting this error with the patch applied:

JavaScript error: chrome://browser/content/browser-tabsintitlebar.js, line 365: TypeError: TabsInTitlebar.updateAppearance is not a function

Comment 25

[Ondrej Zoder [:ozoder]]

(In reply to Dão Gottwald [::dao] from comment #23)
> Have you missed comment 18? I'm still not convinced that this is the right
> approach.

I'd prefer to deal with button sizes in another bug. Martin, could you please fill the bug, as you know more details? Thanks.

Comment 26

[Dão Gottwald [:dao]]

(In reply to Ondrej Zoder [:ozoder] from comment #25)
> (In reply to Dão Gottwald [::dao] from comment #23)
> > Have you missed comment 18? I'm still not convinced that this is the right
> > approach.
> 
> I'd prefer to deal with button sizes in another bug. Martin, could you
> please fill the bug, as you know more details? Thanks.

The thing is, I'm not convinced that this patch is a net win without further changes, so I think we'll want to hold off landing this even if follow-ups are filed.

Comment 27

[Ondrej Zoder [:ozoder]]

Comment on attachment 8992892 [details]
Bug 1424422 - Use built-in icons on titlebar on lwt themes

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

Comment 28

[Dão Gottwald [:dao]]

Comment on attachment 8992892 [details]
Bug 1424422 - Use built-in icons on titlebar on lwt themes

https://reviewboard.mozilla.org/r/257722/#review264694

See my previous comment.

Comment 29

[Martin Stránský [:stransky] (ni? me)]

Attachment: patch from mozreview

Comment 31

[Emilio Cobos Álvarez (:emilio)]

Bug 1718846 fixed this.