Configure titlebar rendering on Linux/Gtk+

RESOLVED FIXED in Firefox 58

Status

()

Firefox
Theme
RESOLVED FIXED
17 days ago
13 days ago

People

(Reporter: stransky, Assigned: stransky)

Tracking

unspecified
Firefox 58
Points:
---

Firefox Tracking Flags

(firefox57 unaffected, firefox58 fixed)

Details

MozReview Requests

()

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

Attachments

(2 attachments)

(Assignee)

Description

17 days ago
When -moz-gtk-csd-available media is off Gtk+ does not support rendering to titlebar, the titlebar is rendered by window managed and we don't draw our own so hide it.

When -moz-gtk-csd-available media is on, window manager does not draw titlebar, we draw our own and render tabs here. Also render close/minimize/maximize buttons according to system setup.
Comment hidden (mozreview-request)
(Assignee)

Comment 2

17 days ago
Dao, do you mind to look at it please? Thanks.
(Assignee)

Updated

17 days ago
Assignee: nobody → stransky
How can I test this patch?

Updated

17 days ago
status-firefox57: --- → unaffected

Comment 4

17 days ago
mozreview-review
Comment on attachment 8924924 [details]
Bug 1414222 - Hide or show #titlebar according to -moz-gtk-csd-available and draw tilebar buttons,

https://reviewboard.mozilla.org/r/196192/#review201378

::: browser/themes/linux/browser.css:715
(Diff revision 1)
>    padding: 3px;
>  }
> +
> +/* Hide the titlebar explicitly on versions of GTK+ where
> + * it's rendered by window manager. */
> +@media not all and (-moz-gtk-csd-available) {

@media (-moz-gtk-csd-available: 0) {

::: browser/themes/linux/browser.css:716
(Diff revision 1)
>  }
> +
> +/* Hide the titlebar explicitly on versions of GTK+ where
> + * it's rendered by window manager. */
> +@media not all and (-moz-gtk-csd-available) {
> +  #main-window > #titlebar {

nit: remove #main-window >

::: browser/themes/linux/browser.css:723
(Diff revision 1)
> +  }
> +}
> +
> +/* We draw to titlebar when Gkt+ CSD is available */
> +@media (-moz-gtk-csd-available) {
> +  #main-window[tabsintitlebar] #titlebar:-moz-lwtheme {

nit: use :root instead of #main-window

::: browser/themes/linux/browser.css:777
(Diff revision 1)
> +    #titlebar-max {
> +      display: none;
> +    }
> +    #main-window[sizemode="maximized"] #titlebar-max {
> +      display: none;
> +    }

This second rule looks redundant.
Attachment #8924924 - Flags: review?(dao+bmo)
(Assignee)

Comment 5

17 days ago
Created attachment 8924949 [details] [diff] [review]
complete.patch

Thanks for looking at it. There's a patch to trunk which contain all missing pieces - just apply it to the latest trunk, build, run on Gtk+ > 3.20 and you should be able to see it working.
Comment hidden (mozreview-request)
(Assignee)

Comment 7

17 days ago
Updated, Thanks.

Comment 8

17 days ago
mozreview-review
Comment on attachment 8924924 [details]
Bug 1414222 - Hide or show #titlebar according to -moz-gtk-csd-available and draw tilebar buttons,

https://reviewboard.mozilla.org/r/196192/#review201420

::: browser/themes/linux/browser.css:723
(Diff revision 2)
> +  }
> +}
> +
> +/* We draw to titlebar when Gkt+ CSD is available */
> +@media (-moz-gtk-csd-available) {
> +  :root #titlebar:-moz-lwtheme {

Looks like [tabsintitlebar] is missing here.

Can you also please use the child selector?

::: browser/themes/linux/browser.css:733
(Diff revision 2)
> +  }
> +
> +  #main-window[tabsintitlebar][sizemode="normal"] > #titlebar {
> +    -moz-appearance: -moz-window-titlebar;
> +  }
> +  #main-window[tabsintitlebar][sizemode="maximized"] > #titlebar {

nit: use :root instead of #main-window
Attachment #8924924 - Flags: review?(dao+bmo)
Comment hidden (mozreview-request)
(Assignee)

Comment 10

17 days ago
mozreview-review-reply
Comment on attachment 8924924 [details]
Bug 1414222 - Hide or show #titlebar according to -moz-gtk-csd-available and draw tilebar buttons,

https://reviewboard.mozilla.org/r/196192/#review201420

> Looks like [tabsintitlebar] is missing here.
> 
> Can you also please use the child selector?

Sorry I misunderstood you.

Comment 11

17 days ago
mozreview-review
Comment on attachment 8924924 [details]
Bug 1414222 - Hide or show #titlebar according to -moz-gtk-csd-available and draw tilebar buttons,

https://reviewboard.mozilla.org/r/196192/#review201452

::: browser/themes/linux/browser.css:743
(Diff revision 3)
> +   * click and hover mouse events to work properly for the button in the restored
> +   * window state. Otherwise, elements in the navigator-toolbox, like the menubar,
> +   * can swallow those events.
> +   */
> +  #titlebar-buttonbox {
> +    z-index: 1;

Does this actually make a difference? z-index only applies to positioned elements and #titlebar-buttonbox doesn't seem to be one.

::: browser/themes/linux/browser.css:766
(Diff revision 3)
> +  @media (-moz-gtk-csd-maximize-button) {
> +    #titlebar-max {
> +      list-style-image: url("moz-icon://stock/window-maximize-symbolic");
> +      -moz-appearance: -moz-window-button-maximize;
> +    }
> +    #main-window[sizemode="maximized"] #titlebar-max {

here's another one... s/#main-window/:root/
Attachment #8924924 - Flags: review?(dao+bmo)
(Assignee)

Comment 12

14 days ago
mozreview-review
Comment on attachment 8924924 [details]
Bug 1414222 - Hide or show #titlebar according to -moz-gtk-csd-available and draw tilebar buttons,

https://reviewboard.mozilla.org/r/196192/#review201760

::: browser/themes/linux/browser.css:743
(Diff revision 3)
> +   * click and hover mouse events to work properly for the button in the restored
> +   * window state. Otherwise, elements in the navigator-toolbox, like the menubar,
> +   * can swallow those events.
> +   */
> +  #titlebar-buttonbox {
> +    z-index: 1;

I got it on windows toolbar theming when fixing a bug that toolbar buttons was hidden:

https://dxr.mozilla.org/mozilla-central/rev/b5a3b8ef6902998507fc881b6d628b055457fe31/browser/themes/windows/browser.css#300
(Assignee)

Comment 13

14 days ago
mozreview-review-reply
Comment on attachment 8924924 [details]
Bug 1414222 - Hide or show #titlebar according to -moz-gtk-csd-available and draw tilebar buttons,

https://reviewboard.mozilla.org/r/196192/#review201760

> I got it on windows toolbar theming when fixing a bug that toolbar buttons was hidden:
> 
> https://dxr.mozilla.org/mozilla-central/rev/b5a3b8ef6902998507fc881b6d628b055457fe31/browser/themes/windows/browser.css#300

Sorry, I must me blind ;)
Comment hidden (mozreview-request)

Comment 15

14 days ago
mozreview-review
Comment on attachment 8924924 [details]
Bug 1414222 - Hide or show #titlebar according to -moz-gtk-csd-available and draw tilebar buttons,

https://reviewboard.mozilla.org/r/196192/#review201824

::: browser/themes/linux/browser.css:766
(Diff revision 4)
> +  @media (-moz-gtk-csd-maximize-button) {
> +    #titlebar-max {
> +      list-style-image: url("moz-icon://stock/window-maximize-symbolic");
> +      -moz-appearance: -moz-window-button-maximize;
> +    }
> +    :root[sizemode="maximized"] > #titlebar-max {

need to remove > here, #titlebar-max is not a direct child of the root node.
Attachment #8924924 - Flags: review?(dao+bmo) → review+
Comment hidden (mozreview-request)
(Assignee)

Comment 17

14 days ago
mozreview-review-reply
Comment on attachment 8924924 [details]
Bug 1414222 - Hide or show #titlebar according to -moz-gtk-csd-available and draw tilebar buttons,

https://reviewboard.mozilla.org/r/196192/#review201824

> need to remove > here, #titlebar-max is not a direct child of the root node.

Fixed, Thanks!
(Assignee)

Updated

14 days ago
Keywords: checkin-needed

Comment 18

14 days ago
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/749437068f04
Hide or show #titlebar according to -moz-gtk-csd-available and draw tilebar buttons, r=dao
Keywords: checkin-needed
Backed out 1 changesets (bug 1414222) for failing Browser Chrome browser_parsable_css.js

https://hg.mozilla.org/integration/autoland/rev/61505446ad650508f3a52d9648db8072a3711701

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=142401135&repo=autoland&lineNumber=2674

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=749437068f048b341ffe50a6b29cdee4403ee3c4&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=usercancel&filter-resultStatus=success
Flags: needinfo?(dao+bmo)
(Assignee)

Comment 20

14 days ago
I'll look at it.
Flags: needinfo?(dao+bmo)

Comment 21

13 days ago
Pushed by stransky@redhat.com:
https://hg.mozilla.org/integration/autoland/rev/08db22c1312e
Hide or show #titlebar according to -moz-gtk-csd-available and draw tilebar buttons, r=dao

Comment 22

13 days ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/08db22c1312e
Status: NEW → RESOLVED
Last Resolved: 13 days ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
You need to log in before you can comment on or make changes to this bug.