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

RESOLVED FIXED in Firefox 60

Status

()

defect
RESOLVED FIXED
a year ago
7 months ago

People

(Reporter: stransky, Assigned: stransky)

Tracking

(Depends on 1 bug, Blocks 1 bug)

59 Branch
mozilla61
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox60 fixed, firefox61 fixed)

Details

Attachments

(3 attachments)

(Assignee)

Description

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

Updated

a year ago
Assignee: nobody → stransky
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 4

a year ago
mozreview-review
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 5

a year ago
mozreview-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 6

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

Comment 10

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

Comment 11

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

Comment 12

a year ago
mozreview-review-reply
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 13

a year ago
mozreview-review
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 14

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

Comment 15

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

Comment 18

a year ago
(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 19

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

Comment 23

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

Comment 24

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

Comment 26

a year ago
(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 27

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

Comment 28

a year ago
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 29

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/759df5effae3
https://hg.mozilla.org/mozilla-central/rev/422c2b84ac8c
https://hg.mozilla.org/mozilla-central/rev/f36db4aef5c3
Status: NEW → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
(Assignee)

Comment 30

a year ago
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?
(Assignee)

Comment 31

a year ago
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?
(Assignee)

Comment 32

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

Comment 34

a year ago
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+

Comment 37

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