1419442 - [CSD] Titlebar size - add drag Space

Status: RESOLVED FIXED

(Core :: Widget: Gtk, enhancement, P2)

Description

[Michal Stanke (Mozilla.cz) [:mstanke][:MikkCZ]]

Firefox 57 introduced "Drag Space" preference (browser.tabs.extraDragSpace) in the toolbar Customize mode, to add some extra space above the tabs to ease moving the window using the mouse. With CSD enabled, the extra drag space is shown regardless of the preference value.

Mozilla/5.0 (X11; Linux x86_64; rv:59.0) Gecko/20100101 Firefox/59.0 ID:20171121100129

Comment 1

[Anass Ahmed]

As I mentioned in my comment here: https://bugzilla.mozilla.org/show_bug.cgi?id=1283299#c115

If you chose the "default" theme and selected a theme like "Adapta", the drag space is not show anymore (the option doesn't do anything at all in any case).

Comment 2

[Mancooo]

Hi, I am on Firefox 57 on Fedora 27 so I have CSD for some time.

The "Drag Space" area is really ugly and looks out of place with the default Gnome theme. I use it with a dark theme and the drag space really sticks out and can't be removed. It also adds some extra pixels for no good reason.

Please remove the "Drag Space". Firefox would look much nicer without it.

Comment 3

[Martin Stránský [:stransky] (ni? me)]

We define default-decoration titlebar style as workaround to ensure the titlebar buttons does not overflow outside. Recently the titlebar size is calculated as tab size + titlebar border/padding (default-decoration has 6px padding at default Adwaita theme). We need to fix titlebar size calculation to also include titlebar button sizes.

Comment 4

[Martin Stránský [:stransky] (ni? me)]

Attachment: Bug 1419442 - [Linux/Titlebar rendering] Add optional drag space above titlebar,

Review commit: https://reviewboard.mozilla.org/r/220444/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/220444/

Comment 5

[Dão Gottwald [:dao]]

Comment on attachment 8951185 [details]
Bug 1419442 - [Linux/Titlebar rendering] Add optional drag space above titlebar,

https://reviewboard.mozilla.org/r/220444/#review226392

::: browser/themes/linux/browser.css:655
(Diff revision 1)
>  }
>  
>  /* We draw to titlebar when Gkt+ CSD is available */
>  @media (-moz-gtk-csd-available) {
> +  :root[tabsintitlebar] > #titlebar {
> +    min-height: calc(var(--tab-min-height) + var(--space-above-tabbar));

How does this "fix titlebar size calculation to also include titlebar button sizes"? I don't get it.

We do have the same rule on Mac, but that's for rendering the title bar background at the right size, not for controling the space above tabs.

Comment 6

[Martin Stránský [:stransky] (ni? me)]

Comment on attachment 8951185 [details]
Bug 1419442 - [Linux/Titlebar rendering] Add optional drag space above titlebar,

https://reviewboard.mozilla.org/r/220444/#review226392

> How does this "fix titlebar size calculation to also include titlebar button sizes"? I don't get it.
> 
> We do have the same rule on Mac, but that's for rendering the title bar background at the right size, not for controling the space above tabs.

I was wrong at comment 3. The button sizes are automatically included at titlebar size. For instance when Adwaita theme defines titlebar button height 32pixels the titlebar has minimal size 32pixels (plus some margin we have there AFAIK). It also means there's an extra drag space above tabs due to bigger button sizes.

This patch fixes when Gtk titlebar button sizes are small (for instance Arc theme defines titlebar buttons 16x16pixels wide) and user enables extra drag space above tab strip. I essentially implements Bug 1349552 for Linux. I just realized we also need to enable the tests here.

Comment 7

[Dão Gottwald [:dao]]

(In reply to Martin Stránský [:stransky] from comment #6)
> This patch fixes when Gtk titlebar button sizes are small (for instance Arc
> theme defines titlebar buttons 16x16pixels wide) and user enables extra drag
> space above tab strip. I essentially implements Bug 1349552 for Linux. I
> just realized we also need to enable the tests here.

Please use Windows as a model for this. As I said Mac has some special requirements that I don't think apply to Gtk.

Comment 8

[Martin Stránský [:stransky] (ni? me)]

Comment on attachment 8951185 [details]
Bug 1419442 - [Linux/Titlebar rendering] Add optional drag space above titlebar,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/220444/diff/1-2/

Comment 9

[Martin Stránský [:stransky] (ni? me)]

Comment on attachment 8951185 [details]
Bug 1419442 - [Linux/Titlebar rendering] Add optional drag space above titlebar,

https://reviewboard.mozilla.org/r/220444/#review226392

> I was wrong at comment 3. The button sizes are automatically included at titlebar size. For instance when Adwaita theme defines titlebar button height 32pixels the titlebar has minimal size 32pixels (plus some margin we have there AFAIK). It also means there's an extra drag space above tabs due to bigger button sizes.
> 
> This patch fixes when Gtk titlebar button sizes are small (for instance Arc theme defines titlebar buttons 16x16pixels wide) and user enables extra drag space above tab strip. I essentially implements Bug 1349552 for Linux. I just realized we also need to enable the tests here.

There's what windows does, Thanks.

Comment 10

[Dão Gottwald [:dao]]

Comment on attachment 8951185 [details]
Bug 1419442 - [Linux/Titlebar rendering] Add optional drag space above titlebar,

https://reviewboard.mozilla.org/r/220444/#review226414

::: browser/themes/linux/browser.css:669
(Diff revision 2)
>    :root[tabsintitlebar][sizemode="maximized"] > #titlebar {
>      -moz-appearance: -moz-window-titlebar-maximized;
>    }
>  
> +  /* Add extra space to titlebar for dragging */
> +  :root[sizemode="normal"][chromehidden~="menubar"] #toolbar-menubar ~ #TabsToolbar,

Just use :root[sizemode="normal"][chromehidden~="menubar"] #TabsToolbar here. See bug 1409407 for details.

::: browser/themes/linux/browser.css:670
(Diff revision 2)
>      -moz-appearance: -moz-window-titlebar-maximized;
>    }
>  
> +  /* Add extra space to titlebar for dragging */
> +  :root[sizemode="normal"][chromehidden~="menubar"] #toolbar-menubar ~ #TabsToolbar,
> +  :root[sizemode="normal"] #toolbar-menubar[autohide="true"][inactive] ~ #TabsToolbar {

and + instead of ~ here

Comment 11

[Martin Stránský [:stransky] (ni? me)]

(In reply to Dão Gottwald [::dao] from comment #10)
> Comment on attachment 8951185 [details]
> Bug 1419442 - [Linux/Titlebar rendering] Add optional drag space above
> titlebar,
> 
> https://reviewboard.mozilla.org/r/220444/#review226414
> 
> ::: browser/themes/linux/browser.css:669
> (Diff revision 2)
> >    :root[tabsintitlebar][sizemode="maximized"] > #titlebar {
> >      -moz-appearance: -moz-window-titlebar-maximized;
> >    }
> >  
> > +  /* Add extra space to titlebar for dragging */
> > +  :root[sizemode="normal"][chromehidden~="menubar"] #toolbar-menubar ~ #TabsToolbar,
> 
> Just use :root[sizemode="normal"][chromehidden~="menubar"] #TabsToolbar
> here. See bug 1409407 for details.
> 
> ::: browser/themes/linux/browser.css:670
> (Diff revision 2)
> >      -moz-appearance: -moz-window-titlebar-maximized;
> >    }
> >  
> > +  /* Add extra space to titlebar for dragging */
> > +  :root[sizemode="normal"][chromehidden~="menubar"] #toolbar-menubar ~ #TabsToolbar,
> > +  :root[sizemode="normal"] #toolbar-menubar[autohide="true"][inactive] ~ #TabsToolbar {
> 
> and + instead of ~ here

Thanks. There's a bug when titlebar button are bigger that TabsToolbar height (defined by Adwaita theme for instance) - it also produces extra dragspace and --space-above-tabbar is added on top.

I'd rather fix this and Bug 1434621 together as it highly depends on each other.

Comment 13

[Dão Gottwald [:dao]]

(In reply to Martin Stránský [:stransky] from comment #11)
> Thanks. There's a bug when titlebar button are bigger that TabsToolbar
> height (defined by Adwaita theme for instance) - it also produces extra
> dragspace and --space-above-tabbar is added on top.
> 
> I'd rather fix this and Bug 1434621 together as it highly depends on each
> other.

How would you want to address this? Surely the "Drag Space" option should result in more drag space when checked, even if there was already some drag space without it?

Comment 14

[Jan Niklas Hasse]

Chromium fixes this by making the titlebar buttons smaller when maximized.

Comment 15

[Martin Stránský [:stransky] (ni? me)]

(In reply to Dão Gottwald [::dao] from comment #13)
> (In reply to Martin Stránský [:stransky] from comment #11)
> > Thanks. There's a bug when titlebar button are bigger that TabsToolbar
> > height (defined by Adwaita theme for instance) - it also produces extra
> > dragspace and --space-above-tabbar is added on top.
> > 
> > I'd rather fix this and Bug 1434621 together as it highly depends on each
> > other.
> 
> How would you want to address this? Surely the "Drag Space" option should
> result in more drag space when checked, even if there was already some drag
> space without it?

The "Drag Space" is disabled the menubar is shown above the tabstrip for instance so we can behave similarly here IMHO when button sizes generates enough dragspace already.

Comment 16

[Martin Stránský [:stransky] (ni? me)]

Attachment: Bug 1419442 - [Linux/Titlebar rendering] Center titlebar buttons,

Linux renders titlebar buttons vertically centered so emulate that for our titlebar rendering.
When menubar is visible center titlebar buttons according menubar, othervise center against tabbar.
Also consider titlebar button sizes which are be defined by actual Gtk+ theme.

Review commit: https://reviewboard.mozilla.org/r/220892/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/220892/

Comment 17

[Martin Stránský [:stransky] (ni? me)]

Comment on attachment 8951185 [details]
Bug 1419442 - [Linux/Titlebar rendering] Add optional drag space above titlebar,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/220444/diff/2-3/

Comment 18

[Code Review Bot [:reviewbot]]

Comment on attachment 8951592 [details]
Bug 1419442 - [Linux/Titlebar rendering] Center titlebar buttons,

https://reviewboard.mozilla.org/r/220892/#review226812


Code analysis found 5 defects in this patch:
 - 5 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: browser/base/content/browser-tabsintitlebar.js:238
(Diff revision 1)
>          let extraMargin = tabAndMenuHeight - titlebarContentHeight;
> -        if (AppConstants.platform != "macosx") {
> +        if (AppConstants.platform == "linux") {
> +          // Linux centers buttons at titlebar so let's emulate that.
> +          if (!fullMenuHeight) {
> +            // Menubar is not visible - center buttons according to tabs.
> +            titlebarContent.style.marginTop = extraMargin/2 + "px";

Error: Infix operators must be spaced. [eslint: space-infix-ops]

::: browser/base/content/browser-tabsintitlebar.js:239
(Diff revision 1)
> -        if (AppConstants.platform != "macosx") {
> +        if (AppConstants.platform == "linux") {
> +          // Linux centers buttons at titlebar so let's emulate that.
> +          if (!fullMenuHeight) {
> +            // Menubar is not visible - center buttons according to tabs.
> +            titlebarContent.style.marginTop = extraMargin/2 + "px";
> +            titlebarContent.style.marginBottom = extraMargin/2 + "px";

Error: Infix operators must be spaced. [eslint: space-infix-ops]

::: browser/base/content/browser-tabsintitlebar.js:241
(Diff revision 1)
> +          if (!fullMenuHeight) {
> +            // Menubar is not visible - center buttons according to tabs.
> +            titlebarContent.style.marginTop = extraMargin/2 + "px";
> +            titlebarContent.style.marginBottom = extraMargin/2 + "px";
> +          } else {
> +            if (fullMenuHeight > titlebarContentHeight) {

Error: Unexpected if as the only statement in an else block. [eslint: no-lonely-if]

::: browser/base/content/browser-tabsintitlebar.js:244
(Diff revision 1)
> +            titlebarContent.style.marginBottom = extraMargin/2 + "px";
> +          } else {
> +            if (fullMenuHeight > titlebarContentHeight) {
> +              // Buttons are smaller than menubar so center them according to menubar.
> +              let fullMenuHeightMargin = fullMenuHeight - titlebarContentHeight;
> +              titlebarContent.style.marginTop = fullMenuHeightMargin/2 + "px";

Error: Infix operators must be spaced. [eslint: space-infix-ops]

::: browser/base/content/browser-tabsintitlebar.js:245
(Diff revision 1)
> +          } else {
> +            if (fullMenuHeight > titlebarContentHeight) {
> +              // Buttons are smaller than menubar so center them according to menubar.
> +              let fullMenuHeightMargin = fullMenuHeight - titlebarContentHeight;
> +              titlebarContent.style.marginTop = fullMenuHeightMargin/2 + "px";
> +              titlebarContent.style.marginBottom = fullMenuHeightMargin/2 + (extraMargin - fullMenuHeightMargin) + "px";

Error: Infix operators must be spaced. [eslint: space-infix-ops]

Comment 19

[Dão Gottwald [:dao]]

Comment on attachment 8951592 [details]
Bug 1419442 - [Linux/Titlebar rendering] Center titlebar buttons,

https://reviewboard.mozilla.org/r/220892/#review226818

::: browser/base/content/browser-tabsintitlebar.js:235
(Diff revision 1)
>        if (tabAndMenuHeight > titlebarContentHeight) {
>          // We need to increase the titlebar content's outer height (ie including margins)
>          // to match the tab and menu height:
>          let extraMargin = tabAndMenuHeight - titlebarContentHeight;
> -        if (AppConstants.platform != "macosx") {
> +        if (AppConstants.platform == "linux") {
> +          // Linux centers buttons at titlebar so let's emulate that.

I'm not sure why we would implement this with JS. Can you just set -moz-box-align: center on #titlebar-buttonbox?

Comment 20

[Martin Stránský [:stransky] (ni? me)]

Attachment: buttonbox.png

button box container size

Comment 21

[Martin Stránský [:stransky] (ni? me)]

Attachment: titlebar-content.png

titlebar-content sizes

Comment 22

[Martin Stránský [:stransky] (ni? me)]

(In reply to Dão Gottwald [::dao] from comment #19)
> Comment on attachment 8951592 [details]
> Bug 1419442 - [Linux/Titlebar rendering] Center titlebar buttons,
> 
> https://reviewboard.mozilla.org/r/220892/#review226818
> 
> ::: browser/base/content/browser-tabsintitlebar.js:235
> (Diff revision 1)
> >        if (tabAndMenuHeight > titlebarContentHeight) {
> >          // We need to increase the titlebar content's outer height (ie including margins)
> >          // to match the tab and menu height:
> >          let extraMargin = tabAndMenuHeight - titlebarContentHeight;
> > -        if (AppConstants.platform != "macosx") {
> > +        if (AppConstants.platform == "linux") {
> > +          // Linux centers buttons at titlebar so let's emulate that.
> 
> I'm not sure why we would implement this with JS. Can you just set
> -moz-box-align: center on #titlebar-buttonbox?

Yes, that was my intention here. But it does not work as #titlebar-buttonbox and even #titlebar-content has exact size as titlebar buttons. The #titlebar-buttonbox is placed up by margin-bottom set by browser-tabsintitlebar.js - so there's no other way than change the browser-tabsintitlebar.js to do anything here (please see the attached pictures).

Comment 23

[Dão Gottwald [:dao]]

Okay, would you mind handling that in a separate bug after all? I'd like to investigate a bit further what currently constraints the layout here and if there are other options.

Comment 24

[Martin Stránský [:stransky] (ni? me)]

(In reply to Dão Gottwald [::dao] from comment #23)
> Okay, would you mind handling that in a separate bug after all? I'd like to
> investigate a bit further what currently constraints the layout here and if
> there are other options.

Sure, reopened Bug 1434621 for it, Thanks.

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/67191cda45ff
[Linux/Titlebar rendering] Add optional drag space above titlebar, r=dao

Comment 26

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/67191cda45ff

Comment 27

[Michal Stanke (Mozilla.cz) [:mstanke][:MikkCZ]]

The checkbox in the customization mode now only increases/decreases the extra drag space, but does not remove it completely when unchecked. Also when maximizing the window some extra space above the tabs is still there. I think the extra space, which is always there, may also be the cause of bug 1439834.