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

NEW
Assigned to

Status

()

defect
P2
normal
a year ago
2 months ago

People

(Reporter: aceman, Assigned: stransky)

Tracking

(Blocks 1 bug)

Trunk
All
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox59 wontfix, firefox60 affected)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

a year ago
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.
(Reporter)

Comment 1

a year ago
Posted image screenshot
Comment hidden (mozreview-request)
(Assignee)

Comment 3

a year ago
We depend on Bug 1408335. 

I also wonder if we want to follow system button sizes when lwt theme is enabled.
Depends on: 1408335
(Assignee)

Updated

a year ago
Assignee: nobody → stransky
Priority: -- → P2
(Assignee)

Updated

a year ago
Duplicate of this bug: 1420866

Comment 5

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

Comment 6

a year ago
mozreview-review
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.
(Assignee)

Updated

a year ago
Duplicate of this bug: 1423190
(Assignee)

Comment 8

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

Comment 11

a year ago
mozreview-review-reply
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.
(Assignee)

Updated

a year ago
Blocks: 1435686
(Assignee)

Updated

a year ago
Duplicate of this bug: 1435686

Comment 13

a year ago
mozreview-review
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.
Attachment #8941797 - Flags: review?(dao+bmo)
Comment hidden (mozreview-request)
(Assignee)

Comment 15

a year ago
mozreview-review-reply
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.
(Assignee)

Comment 16

a year ago
Dao, do you think we can get this to Firefox 60 which will have enabled the titlebar rendering?
Flags: needinfo?(dao+bmo)

Updated

11 months ago
Duplicate of this bug: 1461920
(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.
Flags: needinfo?(dao+bmo) → needinfo?(stransky)

Updated

11 months ago
Attachment #8941797 - Flags: review?(dao+bmo)
Comment hidden (mozreview-request)
Updated patch for current trunk.

Comment 21

9 months ago
mozreview-review
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.
Attachment #8992892 - Flags: review?(dao+bmo)
Comment hidden (mozreview-request)

Comment 23

9 months ago
mozreview-review
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.
Attachment #8992892 - Flags: review?(dao+bmo)
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
(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.
(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 hidden (mozreview-request)

Comment 28

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

Updated

8 months ago
Attachment #8992892 - Attachment is obsolete: true
Flags: needinfo?(stransky)
(Assignee)

Updated

8 months ago
Attachment #8941797 - Attachment is obsolete: true
Duplicate of this bug: 1525222
You need to log in before you can comment on or make changes to this bug.