1411309 - Drag space in title bar does not collapse margins

Status: VERIFIED FIXED

(Firefox :: Theme, defect, P1)

Description

[Dale Harvey (:daleharvey)]

Follow on from https://bugzilla.mozilla.org/show_bug.cgi?id=1407185

Spec is @ https://mozilla.invisionapp.com/share/ZKBC94BPQ#/screens/229786513

The spec mentioned that the drag space is 40px wide both left and right, currently the drag space is 40px width, however the buttons they are next to (the osx window controls on the left, or the new tab / overflow button on the right) have their own margin giving the drag space a visual width of 47px on the left and 42px on the right.

Currently only tested this on OSX, I assume it will effect other platforms

Comment 1

[Dale Harvey (:daleharvey)]

Stephen do we want to collapse these so its 40px including the margins from the buttons, or leave as is?

Comment 2

[Stephen Horlander [:shorlander]]

(In reply to Dale Harvey (:daleharvey) PTO till 23rd from comment #1)
> Stephen do we want to collapse these so its 40px including the margins from
> the buttons, or leave as is?

Yes, it should be a total of 40px including that space (assuming it's draggable space).

Comment 3

[Dale Harvey (:daleharvey)]

Attachment: Bug 1411309 - Remove margin next to chrome controls and drag space.

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

Comment 4

[Dale Harvey (:daleharvey)]

On OSX there is always a button to the right when the placeholder appears and the window controls so I figured the simplest patch is to account for them in the placeholder width, modifying the window control margins (or the last button margins) to be aware of the placeholder seemed much more complicated

Comment 5

[Johann Hofmann [:johannh]]

Comment on attachment 8925013 [details]
Bug 1411309 - Remove margin next to chrome controls and drag space.

https://reviewboard.mozilla.org/r/196262/#review201838

Maybe we can keep these styles shared, let me know if you have any questions :)

::: browser/themes/linux/browser.css:532
(Diff revision 1)
>  
>  /* Tabstrip */
>  
>  %include ../shared/tabs.inc.css
>  
> +.titlebar-placeholder[type="pre-tabs"],

It's not relevant if we move this back to shared, but I don't believe Linux has drag space, so this wouldn't be necessary I think.

::: browser/themes/osx/browser.css:767
(Diff revision 1)
> +/*
> + * We want placeholder total width to be 40px, this accounts for  the margins
> + * of persistent buttons next to the placeholder
> + */
> +.titlebar-placeholder[type="pre-tabs"] {
> +  width: 33px;

I'd prefer if this stayed in shared and we subtracted the 7px margin from https://searchfox.org/mozilla-central/rev/5a60492a53667fc61a62af1847d005a210b7a4f6/browser/base/content/browser-tabsintitlebar.js#252

(Because then everything will be sized correctly)

::: browser/themes/osx/browser.css:770
(Diff revision 1)
> + */
> +.titlebar-placeholder[type="pre-tabs"] {
> +  width: 33px;
> +}
> +.titlebar-placeholder[type="post-tabs"] {
> +  width: 38px;

This should be 40px - var(--toolbarbutton-outer-padding), to account for UI density (and future changes).

Also, I don't understand why we don't need to apply this on Windows. There's also an outer padding on buttons there. Can't we move this back to shared?

Comment 6

[Dão Gottwald [:dao]]

(In reply to Johann Hofmann [:johannh] from comment #5)
> > +.titlebar-placeholder[type="pre-tabs"],
> 
> It's not relevant if we move this back to shared, but I don't believe Linux
> has drag space, so this wouldn't be necessary I think.

CAN_DRAW_IN_TITLEBAR support for Linux is on its way (bug 1283299).

> > +.titlebar-placeholder[type="post-tabs"] {
> > +  width: 38px;
> 
> This should be 40px - var(--toolbarbutton-outer-padding), to account for UI
> density (and future changes).

--toolbarbutton-outer-padding is part of the buttons and doesn't add to the drag space, so we shouldn't use that here:

(In reply to Stephen Horlander [:shorlander] from comment #2)
> Yes, it should be a total of 40px including that space (assuming it's
> draggable space).

Comment 7

[Johann Hofmann [:johannh]]

(In reply to Dão Gottwald [::dao] from comment #6)
> > > +.titlebar-placeholder[type="post-tabs"] {
> > > +  width: 38px;
> > 
> > This should be 40px - var(--toolbarbutton-outer-padding), to account for UI
> > density (and future changes).
> 
> --toolbarbutton-outer-padding is part of the buttons and doesn't add to the
> drag space, so we shouldn't use that here:

Ah, good point, though I wonder if this really needs to be strictly 40px draggable space instead of 38px. In that case the general approach here would be wrong and we'd need to eliminate the padding on the button somehow. I agree with Dale that that sounds a little less trivial and I was under the impression that we're making it 40px mostly for visual consistency, not for being able to drag the 2px next to the button (who does that?).

Would changing this padding to be margin work?

Comment 8

[Dão Gottwald [:dao]]

I think we can just leave this as-is at 42px visually, 40px draggable.

Comment 9

[Dale Harvey (:daleharvey)]

> I think we can just leave this as-is at 42px visually, 40px draggable.

On window controls placeholder we are 47px visual and 40px draggable, leave it as is and close this WONTFIX, or you think we should fix up the margin next to the controls?

Comment 10

[Dão Gottwald [:dao]]

Is that margin draggable?

Comment 11

[Dale Harvey (:daleharvey)]

Yup, clicking anywhere that doesnt register as a different button is draggable for me on osx

Comment 12

[Johann Hofmann [:johannh]]

(In reply to Dão Gottwald [::dao] from comment #10)
> Is that margin draggable?

Regardless, I believe that fixing up the window control width to be correct in browser-tabsintitlebar.js is something we should do. :)

Comment 13

[Dão Gottwald [:dao]]

(In reply to Dale Harvey (:daleharvey) PTO till 23rd from comment #11)
> Yup, clicking anywhere that doesnt register as a different button is
> draggable for me on osx

Then we should take this into account for that spacer.

Comment 14

[Dale Harvey (:daleharvey)]

Comment on attachment 8925013 [details]
Bug 1411309 - Remove margin next to chrome controls and drag space.

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

Comment 15

[Johann Hofmann [:johannh]]

Comment on attachment 8925013 [details]
Bug 1411309 - Remove margin next to chrome controls and drag space.

https://reviewboard.mozilla.org/r/196262/#review201902

::: browser/base/content/browser-tabsintitlebar.js:171
(Diff revision 2)
>  
>        // Get the height of the tabs toolbar:
>        let fullTabsHeight = rect($("TabsToolbar")).height;
>  
> -      // Buttons first:
> -      let captionButtonsBoxWidth = rect($("titlebar-buttonbox-container")).width;
> +      // Buttons first: The caption buttons are given a 7 px left margin which is
> +      // included in their width, but would like to remove that from being double

"included in their width" is a little misleading, what's happening here is that we're taking the width of the container of the window controls (#titlebar-buttonbox), so that any margins add up to it.

On another thought, have you tried just taking the dimensions of #titlebar-buttonbox instead of #titlebar-buttonbox-container and see how that works out? Maybe that's an even more elegant solution, OTOH there could be a reason why we're using the container here...

::: browser/base/content/browser-tabsintitlebar.js:174
(Diff revision 2)
>  
> -      // Buttons first:
> -      let captionButtonsBoxWidth = rect($("titlebar-buttonbox-container")).width;
> +      // Buttons first: The caption buttons are given a 7 px left margin which is
> +      // included in their width, but would like to remove that from being double
> +      // counted for a right margin as the drag space now sits flush next
> +      // to the buttons
> +      let captionButtonsBoxWidth = rect($("titlebar-buttonbox-container")).width - 7;

This is only on OSX, right? Probably should move it into the if condition below.

Comment 16

[Dale Harvey (:daleharvey)]

Comment on attachment 8925013 [details]
Bug 1411309 - Remove margin next to chrome controls and drag space.

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

Comment 17

[Dale Harvey (:daleharvey)]

> On another thought, have you tried just taking the dimensions of #titlebar-buttonbox 
> instead of #titlebar-buttonbox-container and see how that works out? 
> Maybe that's an even more elegant solution, OTOH there could be a reason why 
> we're using the container here...

Previously we wanted to double count the margin here as the right also needed a margin, but as windows doesnt have / want that margin and we dont want it on osx any more then not using the container works perfect, good idea

Comment 18

[Johann Hofmann [:johannh]]

Comment on attachment 8925013 [details]
Bug 1411309 - Remove margin next to chrome controls and drag space.

https://reviewboard.mozilla.org/r/196262/#review202212

Looks good to me, having only tested on OSX.

Thank you!

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c464c8f2c682
Remove margin next to chrome controls and drag space. r=johannh

Comment 20

[Dale Harvey (:daleharvey)]

For verification, on the right hand side will be 42px from the last button to the edge of the window with 40px being draggable and 2px being the margin of the last button. the space between the window controls and the first tab border is 40px exactly

Comment 21

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/c464c8f2c682

Comment 22

[Valentina Claudia Ona]

I verified this issue using latest Nightly 58.0a1 on Windows 7 x64, Windows 10 x64 and Mac OS 10.13 with Build ID 20171107220115.
I will mark this as verified fixed.