Closed Bug 1419442 Opened 3 years ago Closed 2 years ago

[CSD] Titlebar size - add drag Space

Categories

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

59 Branch
enhancement

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox59 --- wontfix
firefox60 --- fixed

People

(Reporter: mstanke, Assigned: stransky)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 1 obsolete file)

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
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).
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.
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: nobody → stransky
Priority: -- → P2
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)
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.
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 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 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.
Duplicate of this bug: 1434621
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 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 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)
Attached image buttonbox.png
button box container size
Attached 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)
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
Pushed by stransky@redhat.com:
https://hg.mozilla.org/integration/autoland/rev/67191cda45ff
[Linux/Titlebar rendering] Add optional drag space above titlebar, r=dao
https://hg.mozilla.org/mozilla-central/rev/67191cda45ff
Status: NEW → RESOLVED
Closed: 2 years ago
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.