Closed Bug 1439834 Opened 6 years ago Closed 6 years ago

[CSD] Actual titlebar height is bigger than titlebar height rendered by theme engine

Categories

(Core :: Widget: Gtk, defect)

Product:
Core
Component:
Widget: Gtk
Version:
59 Branch
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla61
Tracking Flags:
Tracking Status
firefox60 --- fixed
firefox61 --- fixed

People

(Reporter: stransky, Assigned: stransky)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

Attachments

(3 files)

Bug 1439834 - Draw titlebar with some extent,
59 bytes, text/x-review-board-request
dao
: review+
jcristau
: approval-mozilla-beta+
Details
Bug 1439834 - Stretch #titlebar over dragspace,
59 bytes, text/x-review-board-request
dao
: review+
jcristau
: approval-mozilla-beta+
Details
Bug 1439834 - Make #TabsToolbar transparent when styled #titlebar (-moz-window-titlebar) should be shown,
59 bytes, text/x-review-board-request
dao
: review+
jcristau
: approval-mozilla-beta+
Details 
When drag space is enabled na Adwaita theme the titlebar height is bigger that actual height rendered by theme engine:

https://bugzilla.mozilla.org/attachment.cgi?id=8952367
Assignee: nobody → stransky
Comment on attachment 8954722 [details]
Bug 1439834 - Stretch #titlebar over dragspace,

https://reviewboard.mozilla.org/r/223852/#review231262

::: commit-message-57306:3
(Diff revision 1)
> +Bug 1439834 - Stretch #titlebar over dragspace, r?dao
> +
> +Add missing drag space height (present as #TabsToolbar margin) to

We should use padding rather than margin for this, like we do on Windows, so it's automatically included in the height calculation.
Attachment #8954722 - Flags: review?(dao+bmo) → review-
Comment on attachment 8954723 [details]
Bug 1439834 - Make #TabsToolbar transparent when styled #titlebar (-moz-window-titlebar) should be shown,

https://reviewboard.mozilla.org/r/223854/#review231264

::: browser/themes/linux/browser.css:664
(Diff revision 1)
>    :root[sizemode="normal"] #toolbar-menubar[autohide="true"][inactive] + #TabsToolbar {
>      margin-top: var(--space-above-tabbar);
> +    /* Make #TabsToolbar transparent as we style underlying #titlebar with
> +       -moz-window-titlebar (Gtk+ theme).
> +     */
> +    -moz-appearance: none;

Why are you doing this for [sizemode="normal"] only? And shouldn't this depend on [tabsintitlebar]?
Attachment #8954723 - Flags: review?(dao+bmo) → review-
Comment on attachment 8954721 [details]
Bug 1439834 - Draw titlebar with some extent,

https://reviewboard.mozilla.org/r/223850/#review231266

::: widget/gtk/gtk3drawing.cpp:2302
(Diff revision 1)
> -    gtk_render_frame(style, cr, rect->x, rect->y, rect->width, rect->height);
>  
> +    // Some themes (Adwaita for instance) draws bold dark line at
> +    // titlebar bottom. It does not fit well with Firefox tabs so
> +    // draw with some extent to make the titlebar bottom part invisible.
> +    #define TITLEBAR_EXTENT 20

20 seems a bit excessive if the main concern is a bottom border that probably isn't taller than a few pixels? If we extend too much, it will make e.g. the gradient used on Ubuntu look flatter, right?
Attachment #8954721 - Flags: review?(dao+bmo) → review+
Blocks: 1439373
Comment on attachment 8954721 [details]
Bug 1439834 - Draw titlebar with some extent,

https://reviewboard.mozilla.org/r/223850/#review231266

> 20 seems a bit excessive if the main concern is a bottom border that probably isn't taller than a few pixels? If we extend too much, it will make e.g. the gradient used on Ubuntu look flatter, right?

Lowered to 4 pixels.
Comment on attachment 8954722 [details]
Bug 1439834 - Stretch #titlebar over dragspace,

https://reviewboard.mozilla.org/r/223852/#review231262

> We should use padding rather than margin for this, like we do on Windows, so it's automatically included in the height calculation.

Changed, thanks.
Comment on attachment 8954723 [details]
Bug 1439834 - Make #TabsToolbar transparent when styled #titlebar (-moz-window-titlebar) should be shown,

https://reviewboard.mozilla.org/r/223854/#review231264

> Why are you doing this for [sizemode="normal"] only? And shouldn't this depend on [tabsintitlebar]?

Added as extra rule, Thanks.
Comment on attachment 8954722 [details]
Bug 1439834 - Stretch #titlebar over dragspace,

https://reviewboard.mozilla.org/r/223852/#review231564
Attachment #8954722 - Flags: review?(dao+bmo) → review+
Comment on attachment 8954723 [details]
Bug 1439834 - Make #TabsToolbar transparent when styled #titlebar (-moz-window-titlebar) should be shown,

https://reviewboard.mozilla.org/r/223854/#review231568

::: browser/themes/linux/browser.css:678
(Diff revision 2)
>  
> +  /* Make #TabsToolbar transparent as we style underlying #titlebar with
> +   * -moz-window-titlebar (Gtk+ theme).
> +   */
> +  :root[tabsintitlebar][chromehidden~="menubar"] #TabsToolbar,
> +  :root[tabsintitlebar] #toolbar-menubar[autohide="true"][inactive] + #TabsToolbar {

Why does it matter whether the menu bar is shown? Shouldn't we do this regardless?
Attachment #8954723 - Flags: review?(dao+bmo)
Comment on attachment 8954723 [details]
Bug 1439834 - Make #TabsToolbar transparent when styled #titlebar (-moz-window-titlebar) should be shown,

https://reviewboard.mozilla.org/r/223854/#review231568

> Why does it matter whether the menu bar is shown? Shouldn't we do this regardless?

When the menu bar is shown we (IMHO) want to render the menu on menubackground (which is the TabsToolbar background) instead of general titlebar background.
(In reply to Martin Stránský [:stransky] from comment #15)
> Comment on attachment 8954723 [details]
> Bug 1439834 - Make #TabsToolbar transparent when styled #titlebar
> (-moz-window-titlebar) should be shown,
> 
> https://reviewboard.mozilla.org/r/223854/#review231568
> 
> > Why does it matter whether the menu bar is shown? Shouldn't we do this regardless?
> 
> When the menu bar is shown we (IMHO) want to render the menu on
> menubackground (which is the TabsToolbar background) instead of general
> titlebar background.

But the titlebar would still extend behind the menu and tabs toolbars, which seems odd and might even be visible depending on the theme. It would be cleaner and consistent with what we do on Windows to use only the titlebar background.
(In reply to Dão Gottwald [::dao] from comment #16)
> But the titlebar would still extend behind the menu and tabs toolbars, which
> seems odd and might even be visible depending on the theme. It would be
> cleaner and consistent with what we do on Windows to use only the titlebar
> background.

Okay, there's a patch there.
Comment on attachment 8954723 [details]
Bug 1439834 - Make #TabsToolbar transparent when styled #titlebar (-moz-window-titlebar) should be shown,

https://reviewboard.mozilla.org/r/223854/#review232574

::: browser/themes/linux/browser.css:677
(Diff revision 3)
>    }
>  
> +  /* Make #TabsToolbar transparent as we style underlying #titlebar with
> +   * -moz-window-titlebar (Gtk+ theme).
> +   */
> +  :root[tabsintitlebar] #TabsToolbar {

We probably need to do the same for #toolbar-menubar (for when it's shown with tabsintitlebar enabled).

::: browser/themes/linux/browser.css:678
(Diff revision 3)
>  
> +  /* Make #TabsToolbar transparent as we style underlying #titlebar with
> +   * -moz-window-titlebar (Gtk+ theme).
> +   */
> +  :root[tabsintitlebar] #TabsToolbar {
> +    -moz-appearance: none;

Do we also need to set the right text color here, such as CaptionText? I haven't checked whether that resolves to anything useful. This currently uses -moz-menubartext: https://searchfox.org/mozilla-central/rev/44fa24847e4e73ce8932de4c8cf47559302e4ba2/browser/themes/linux/browser.css#468
Attachment #8954723 - Flags: review?(dao+bmo)
Comment on attachment 8954723 [details]
Bug 1439834 - Make #TabsToolbar transparent when styled #titlebar (-moz-window-titlebar) should be shown,

https://reviewboard.mozilla.org/r/223854/#review232574

> Do we also need to set the right text color here, such as CaptionText? I haven't checked whether that resolves to anything useful. This currently uses -moz-menubartext: https://searchfox.org/mozilla-central/rev/44fa24847e4e73ce8932de4c8cf47559302e4ba2/browser/themes/linux/browser.css#468

The -moz-menubartext text color is fine here IMHO. 

#TabsToolbar text color is used for the titlebar/tabs text (text on tab, tab close icon, '+' icon for new tab) and we should not change that for [tabsintitlebar] only.
Comment on attachment 8954723 [details]
Bug 1439834 - Make #TabsToolbar transparent when styled #titlebar (-moz-window-titlebar) should be shown,

https://reviewboard.mozilla.org/r/223854/#review232574

> The -moz-menubartext text color is fine here IMHO. 
> 
> #TabsToolbar text color is used for the titlebar/tabs text (text on tab, tab close icon, '+' icon for new tab) and we should not change that for [tabsintitlebar] only.

Or do you mean that "-moz-appearance:" directive resets the color attribute? In that case I'd also use -moz-menubartext here.
(In reply to Martin Stránský [:stransky] from comment #23)
> Comment on attachment 8954723 [details]
> Bug 1439834 - Make #TabsToolbar transparent when styled #titlebar
> (-moz-window-titlebar) should be shown,
> 
> https://reviewboard.mozilla.org/r/223854/#review232574
> 
> > Do we also need to set the right text color here, such as CaptionText? I haven't checked whether that resolves to anything useful. This currently uses -moz-menubartext: https://searchfox.org/mozilla-central/rev/44fa24847e4e73ce8932de4c8cf47559302e4ba2/browser/themes/linux/browser.css#468
> 
> The -moz-menubartext text color is fine here IMHO. 
> 
> #TabsToolbar text color is used for the titlebar/tabs text (text on tab, tab
> close icon, '+' icon for new tab) and we should not change that for
> [tabsintitlebar] only.

But a menu bar (which -moz-menubartext is intended for) can have a completely different background than a title bar, no?
Flags: needinfo?(stransky)
(In reply to Dão Gottwald [::dao] from comment #25)
> But a menu bar (which -moz-menubartext is intended for) can have a
> completely different background than a title bar, no?

Theoretically yes, titlebar and menubar can be styled differently, but generally both uses the same text color. I checked some themes (Ubuntu Ambiance, default Adwaita, KDE Arc) and all has the same colors for titlebar color and hearedbar color.

As the titlebar/menubar color can be (and it's) styled, it's IMHO better to use the menubartext as it matches the styling somehow. The 100% fix would be to add -moz-titlebartext color but we'd rather leave that for further bug.
Flags: needinfo?(stransky)
Comment on attachment 8954723 [details]
Bug 1439834 - Make #TabsToolbar transparent when styled #titlebar (-moz-window-titlebar) should be shown,

https://reviewboard.mozilla.org/r/223854/#review233180
Attachment #8954723 - Flags: review?(dao+bmo) → review+
Pushed by stransky@redhat.com:
https://hg.mozilla.org/integration/autoland/rev/759df5effae3
Draw titlebar with some extent, r=dao
https://hg.mozilla.org/integration/autoland/rev/422c2b84ac8c
Stretch #titlebar over dragspace, r=dao
https://hg.mozilla.org/integration/autoland/rev/f36db4aef5c3
Make #TabsToolbar transparent when styled #titlebar (-moz-window-titlebar) should be shown, r=dao
Comment on attachment 8954721 [details]
Bug 1439834 - Draw titlebar with some extent,

Approval Request Comment
[Feature/Bug causing the regression]: none
[User impact if declined]: poor user experience when titlebar rendering is enabled.
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: theme fix on isolated feature (which is disabled by default).
[String changes made/needed]: none
Attachment #8954721 - Flags: approval-mozilla-beta?
Comment on attachment 8954722 [details]
Bug 1439834 - Stretch #titlebar over dragspace,

Approval Request Comment
[Feature/Bug causing the regression]: none
[User impact if declined]: poor user experience when titlebar rendering is enabled.
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]:  no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: theme fix on isolated feature (which is disabled by default).
[String changes made/needed]: none
Attachment #8954722 - Flags: approval-mozilla-beta?
Comment on attachment 8954723 [details]
Bug 1439834 - Make #TabsToolbar transparent when styled #titlebar (-moz-window-titlebar) should be shown,

Approval Request Comment
[Feature/Bug causing the regression]: none
[User impact if declined]: poor user experience when titlebar rendering is enabled.
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: theme fix on isolated feature (which is disabled by default).
[String changes made/needed]: none
Attachment #8954723 - Flags: approval-mozilla-beta?
Is that feature exposed somehow in the UI?  What's the rationale for uplift here if this is disabled?
Flags: needinfo?(stransky)
Yes, it can be enabled at Customization -> Title bar check box or at about:config, this feature is introduced at Firefox 60 and it's highly appreciated (see tracking Bug 1283299 for details).
Flags: needinfo?(stransky)
Comment on attachment 8954721 [details]
Bug 1439834 - Draw titlebar with some extent,

fixes for titlebar rendering on gtk, beta60+
Attachment #8954721 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8954722 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8954723 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Hm, this just feels hacky to me…
Depends on: 1456522
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: