Closed
Bug 1424422
Opened 7 years ago
Closed 3 years ago
[CSD] window buttons in title bar always black even with a dark LW theme
Categories
(Core :: Widget: Gtk, defect, P2)
Tracking
()
RESOLVED
DUPLICATE
of bug 1718846
People
(Reporter: aceman, Assigned: stransky)
References
(Blocks 2 open bugs)
Details
Attachments
(2 files, 2 obsolete files)
(deleted),
image/png
|
Details | |
(deleted),
patch
|
Details | Diff | Splinter Review |
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 hidden (mozreview-request) |
Assignee | ||
Comment 3•7 years 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•7 years ago
|
Assignee: nobody → stransky
Updated•7 years ago
|
Priority: -- → P2
Comment 5•7 years 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•7 years 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 | ||
Comment 8•7 years 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•7 years 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.
Comment 13•7 years 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•7 years 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 | ||
Updated•7 years ago
|
status-firefox60:
--- → affected
Assignee | ||
Comment 16•7 years ago
|
||
Dao, do you think we can get this to Firefox 60 which will have enabled the titlebar rendering?
Flags: needinfo?(dao+bmo)
Comment 18•7 years ago
|
||
(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•7 years ago
|
Attachment #8941797 -
Flags: review?(dao+bmo)
Comment hidden (mozreview-request) |
Comment 20•6 years ago
|
||
Updated patch for current trunk.
Comment 21•6 years 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•6 years 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)
Comment 24•6 years ago
|
||
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•6 years ago
|
||
(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•6 years ago
|
||
(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•6 years 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 | ||
Comment 29•6 years ago
|
||
Assignee | ||
Updated•6 years ago
|
Attachment #8992892 -
Attachment is obsolete: true
Flags: needinfo?(stransky)
Assignee | ||
Updated•6 years ago
|
Attachment #8941797 -
Attachment is obsolete: true
Comment 31•3 years ago
|
||
Bug 1718846 fixed this.
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → DUPLICATE
You need to log in
before you can comment on or make changes to this bug.
Description
•