Closed
Bug 1419442
Opened 6 years ago
Closed 5 years ago
[CSD] Titlebar size - add drag Space
Categories
(Core :: Widget: Gtk, enhancement, P2)
Tracking
()
RESOLVED
FIXED
mozilla60
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
Comment 1•6 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).
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•6 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•6 years ago
|
Assignee: nobody → stransky
Priority: -- → P2
Comment hidden (mozreview-request) |
Comment 5•5 years ago
|
||
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•5 years ago
|
||
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•5 years ago
|
Flags: needinfo?(dao+bmo)
Comment 7•5 years ago
|
||
(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.
Updated•5 years ago
|
Flags: needinfo?(dao+bmo)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 9•5 years ago
|
||
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•5 years ago
|
||
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+
Assignee | ||
Comment 11•5 years ago
|
||
(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•5 years ago
|
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
Comment 13•5 years ago
|
||
(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•5 years ago
|
||
Chromium fixes this by making the titlebar buttons smaller when maximized.
Assignee | ||
Comment 15•5 years ago
|
||
(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•5 years ago
|
||
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•5 years ago
|
||
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)
Assignee | ||
Comment 20•5 years ago
|
||
button box container size
Assignee | ||
Comment 21•5 years ago
|
||
titlebar-content sizes
Assignee | ||
Comment 22•5 years ago
|
||
(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)
Comment 23•5 years ago
|
||
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•5 years ago
|
Attachment #8951592 -
Attachment is obsolete: true
Assignee | ||
Comment 24•5 years ago
|
||
(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•5 years ago
|
||
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•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/67191cda45ff
Status: NEW → RESOLVED
Closed: 5 years ago
status-firefox60:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Reporter | ||
Comment 27•5 years ago
|
||
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.
Updated•5 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•