[CSD] Titlebar size - add drag Space

RESOLVED FIXED in Firefox 60

Status

()

enhancement
P2
normal
RESOLVED FIXED
2 years ago
Last year

People

(Reporter: MikkCZ, Assigned: stransky)

Tracking

(Blocks 1 bug)

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

Firefox Tracking Flags

(firefox59 wontfix, firefox60 fixed)

Details

Attachments

(3 attachments, 1 obsolete attachment)

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

2 years ago
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

2 years ago
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.
Assignee

Comment 3

2 years ago
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.
Summary: [CSD] Drag Space is shown above the tabs regardless the setting → [CSD] Titlebar size - drag Space is shown above the tabs regardless the setting
Assignee

Updated

2 years ago
Assignee: nobody → stransky
Priority: -- → P2

Comment 5

Last year
mozreview-review
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.
Attachment #8951185 - Flags: review?(dao+bmo)
Assignee

Comment 6

Last year
mozreview-review-reply
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.
Assignee

Updated

Last year
Flags: needinfo?(dao+bmo)
(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.
Flags: needinfo?(dao+bmo)
Comment hidden (mozreview-request)
Assignee

Comment 9

Last year
mozreview-review-reply
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

Last year
mozreview-review
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
Attachment #8951185 - Flags: review?(dao+bmo) → review+
(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.
Assignee

Updated

Last year
Duplicate of this bug: 1434621
Assignee

Updated

Last year
Summary: [CSD] Titlebar size - drag Space is shown above the tabs regardless the setting → [CSD] Titlebar size - fix drag Space size and titlebar button positions
(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?
Chromium fixes this by making the titlebar buttons smaller when maximized.
(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 hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 18

Last year
mozreview-review
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

Last year
mozreview-review
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?
Attachment #8951592 - Flags: review?(dao+bmo)
Posted image buttonbox.png
button box container size
Posted image titlebar-content.png
titlebar-content sizes
(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).
Flags: needinfo?(dao+bmo)
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.
Flags: needinfo?(dao+bmo)
Assignee

Updated

Last year
Attachment #8951592 - Attachment is obsolete: true
(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.
Summary: [CSD] Titlebar size - fix drag Space size and titlebar button positions → [CSD] Titlebar size - add drag Space

Comment 25

Last year
Pushed by stransky@redhat.com:
https://hg.mozilla.org/integration/autoland/rev/67191cda45ff
[Linux/Titlebar rendering] Add optional drag space above titlebar, r=dao

Comment 26

Last year
bugherder
https://hg.mozilla.org/mozilla-central/rev/67191cda45ff
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
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.
You need to log in before you can comment on or make changes to this bug.