Closed Bug 1414222 Opened 7 years ago Closed 7 years ago

Configure titlebar rendering on Linux/Gtk+

Categories

(Firefox :: Theme, enhancement)

Product:
Firefox
Component:
Theme
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 58
Tracking Flags:
Tracking Status
firefox57 --- unaffected
firefox58 --- fixed

People

(Reporter: stransky, Assigned: stransky)

References

Details

Attachments

(2 files)

Bug 1414222 - Hide or show #titlebar according to -moz-gtk-csd-available and draw tilebar buttons,
59 bytes, text/x-review-board-request
dao
: review+
Details
complete.patch
22.07 KB, patch
Details | Diff | Splinter Review 
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.
Dao, do you mind to look at it please? Thanks.
Assignee: nobody → stransky
How can I test this patch?
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)
Attached patch complete.patch — — Splinter Review 
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.
Updated, Thanks.
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 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 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)
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
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 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 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!
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
I'll look at it.
Flags: needinfo?(dao+bmo)
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
https://hg.mozilla.org/mozilla-central/rev/08db22c1312e
Status: NEW → RESOLVED
Closed: 7 years 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.

Attachment

General

Creator:
Created:
Updated:
Size: