1399930 - [macOS] [Photon] Regression: The below part of the Tab Bar doesn't react on double-clicks

Status: VERIFIED FIXED

(Firefox :: Theme, defect, P1)

Description

[Mehmet]

Attachment: Bildschirmfoto 2017-09-14 um 18.37.25.png

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_6) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/63.0.3214.0 Safari/537.36

Steps to reproduce:

57.0a1 (2017-09-14) (64-Bit)
macOS 10.12.6

1.) Double-click on the Tab Bar (in the area from the middle to the bottom)


Actual results:

Depending on your OS-Settings, the window should expand/shrink or minimize into the Dock


Expected results:

Nothing happens.

A screenshot is attached.

Comment 1

[Mehmet]

Sorry typo:

Actual results:
Nothing happens.


Expected results:
Depending on your OS-Settings, the window should expand/shrink or minimize into the Dock

Comment 2

[:Gijs (he/him)]

Previously, this was a result of vibrancy ( bug 1161565 ) . If that's the case here too, finding a regression window would find bug 1377284. Can you doublecheck that and/or post a correct regression window?

Comment 3

[Mehmet]

(In reply to :Gijs from comment #2)
> Previously, this was a result of vibrancy ( bug 1161565 ) . If that's the
> case here too, finding a regression window would find bug 1377284. Can you
> doublecheck that and/or post a correct regression window?

Regression Range:

Good Build: 57.0a1 (2017-09-12) (64-bit)

Bad Build: 57.0a1 (2017-09-13) (64-bit)

Comment 4

[Johann Hofmann [:johannh]]

Yes, this seems to only happen with the new drag space turned on. But I think only on OSX, not on Windows. Can you confirm that the probably goes away if you turn off the dragspace?

I'm trying to understand bug 1399930, but it looks related somehow.

Comment 5

[Johann Hofmann [:johannh]]

(In reply to Johann Hofmann [:johannh] from comment #4)
> Yes, this seems to only happen with the new drag space turned on. But I
> think only on OSX, not on Windows. Can you confirm that the probably goes
> away if you turn off the dragspace?
> 
> I'm trying to understand bug 1399930, but it looks related somehow.

Heh, I meant bug 1161565.

Comment 6

[Mehmet]

(In reply to Johann Hofmann [:johannh] from comment #5)
> (In reply to Johann Hofmann [:johannh] from comment #4)
> > Yes, this seems to only happen with the new drag space turned on. But I
> > think only on OSX, not on Windows. Can you confirm that the probably goes
> > away if you turn off the dragspace?
> > 
> > I'm trying to understand bug 1399930, but it looks related somehow.
> 
> Heh, I meant bug 1161565.

Hello Johann,

yes, it is also reproducible if "Drag Space" is disabled.

Comment 7

[Johann Hofmann [:johannh]]

(In reply to Mehmet from comment #6)
> (In reply to Johann Hofmann [:johannh] from comment #5)
> > (In reply to Johann Hofmann [:johannh] from comment #4)
> > > Yes, this seems to only happen with the new drag space turned on. But I
> > > think only on OSX, not on Windows. Can you confirm that the probably goes
> > > away if you turn off the dragspace?
> > > 
> > > I'm trying to understand bug 1399930, but it looks related somehow.
> > 
> > Heh, I meant bug 1161565.
> 
> Hello Johann,
> 
> yes, it is also reproducible if "Drag Space" is disabled.

Ok, that's a bit confusing to me, maybe this isn't bug 1349552 after all. Would you mind using mozregression to find the offending patch?

Comment 8

[Johann Hofmann [:johannh]]

With dragspace disabled, ideally.

Comment 9

[Mehmet]

(In reply to Johann Hofmann [:johannh] from comment #7)
> (In reply to Mehmet from comment #6)
> > (In reply to Johann Hofmann [:johannh] from comment #5)
> > > (In reply to Johann Hofmann [:johannh] from comment #4)
> > > > Yes, this seems to only happen with the new drag space turned on. But I
> > > > think only on OSX, not on Windows. Can you confirm that the probably goes
> > > > away if you turn off the dragspace?
> > > > 
> > > > I'm trying to understand bug 1399930, but it looks related somehow.
> > > 
> > > Heh, I meant bug 1161565.
> > 
> > Hello Johann,
> > 
> > yes, it is also reproducible if "Drag Space" is disabled.
> 
> Ok, that's a bit confusing to me, maybe this isn't bug 1349552 after all.
> Would you mind using mozregression to find the offending patch?

I'll take a look tomorrow.

Comment 10

[ovidiu boca[:Ovidiu]]

I can confirm this bug, I did a regression range and here are the results:


3:19.48 LOG: MainThread Bisector INFO Last good revision: a73cc4e08bf5a005722c95b43f84ab0c8ff2bc7c (2017-09-12)
 3:19.48 LOG: MainThread Bisector INFO First bad revision: 8645a74bbbd06b67699317df1abf3897db0e43d5 (2017-09-13)
 3:19.48 LOG: MainThread Bisector INFO Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=a73cc4e08bf5a005722c95b43f84ab0c8ff2bc7c&tochange=8645a74bbbd06b67699317df1abf3897db0e43d5


I couldn't go further than this.

Comment 11

[Johann Hofmann [:johannh]]

Huh, well that regression range doesn't tell us a lot :)

I'll try to backout bug 1349552 locally and if that works I'll try to bisect what part of the patch caused it (might be the extra vibrancy on the menu bar for some reason).

Comment 12

[Johann Hofmann [:johannh]]

Attachment: Bug 1399930 - Add drag space padding to the tabs toolbar on OSX.

The previous drag space was only set as min-height on the titlebar,
which led tabsintitlebar.js to not account that extra space for moving
the titlebar to the bottom (because it calculates that height from
tab strip height). So while it looked fine visually, the titlebar did
not stretch all the way to the bottom, so that e.g. double-clicks aren't
registered correctly.

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

Comment 13

[Johann Hofmann [:johannh]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=667b131485658927fc2f3d484b2ab15cce782d95

Comment 14

[:Gijs (he/him)]

Comment on attachment 8908564 [details]
Bug 1399930 - Add drag space padding to the tabs toolbar on OSX.

https://reviewboard.mozilla.org/r/180240/#review185732

Tentative r=me... this stuff is always hairy.

I noticed when testing this patch that:
1) this bug also happens with lwthemes (and this patch doesn't fix that), but I also see this on 56.
2) we've lost lwthemes-in-the-titlebar on OS X when you enable the titlebar, at some point in the 57 cycle. It's not clear to me whether that was deliberate, but it's a bit sad. On 56, I can get the old behaviour but with a dark foreground colour against a dark background, so the titlebar is unreadable. :-\

I don't think either of those issues are to do with this patch / regression, so r=me either way, but hence my taking a long time to review...

::: browser/themes/osx/browser.css:890
(Diff revision 1)
>  
>  #TabsToolbar {
>    -moz-appearance: none;
>    /* overlap the nav-bar's top border */
>    margin-bottom: calc(-1 * var(--tab-toolbar-navbar-overlap));
> +  padding-top: var(--space-above-tabbar);

So can we remove the min-height?

Comment 15

[:Gijs (he/him)]

(In reply to :Gijs from comment #14)
> 2) we've lost lwthemes-in-the-titlebar on OS X when you enable the titlebar,
> at some point in the 57 cycle. It's not clear to me whether that was
> deliberate, but it's a bit sad. On 56, I can get the old behaviour but with
> a dark foreground colour against a dark background, so the titlebar is
> unreadable. :-\

Looks like this was deliberate in bug 1392219.

Comment 16

[Johann Hofmann [:johannh]]

(In reply to :Gijs from comment #14)
> this stuff is always hairy.

Oh, man, it really is.

> I noticed when testing this patch that:
> 1) this bug also happens with lwthemes (and this patch doesn't fix that),
> but I also see this on 56.

Do we have a bug for that?

> 2) we've lost lwthemes-in-the-titlebar on OS X when you enable the titlebar,
> at some point in the 57 cycle. It's not clear to me whether that was
> deliberate, but it's a bit sad. On 56, I can get the old behaviour but with
> a dark foreground colour against a dark background, so the titlebar is
> unreadable. :-\

Yes, it was a deliberate decision but I'd love to revisit this by drawing the title text ourselves.

> I don't think either of those issues are to do with this patch / regression,
> so r=me either way, but hence my taking a long time to review...

Thanks for being so thorough. :)

> ::: browser/themes/osx/browser.css:890
> (Diff revision 1)
> >  
> >  #TabsToolbar {
> >    -moz-appearance: none;
> >    /* overlap the nav-bar's top border */
> >    margin-bottom: calc(-1 * var(--tab-toolbar-navbar-overlap));
> > +  padding-top: var(--space-above-tabbar);
> 
> So can we remove the min-height?

No, the min-height is still required. The padding change is just so that the negative margin offset is correctly applied, i.e. bug 1349552 correctly set the min-height of the titlebar to 40px, but the negative offset was still calculated as 32px and so 8px of space between the nav bar and the titlebar were unclickable (if I understood it correctly).

Comment 17

[Mehmet]

Just only a note: In the case if the Title Bar is enabled, double-clicking on the Tab Bar is working as expected at the moment and is not broken. This means, a double-click in this case opens a new tab. Just wanted to note this, that you please consider it in the fix :-) Thank you.

Comment 18

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/6bd0d9d8bc16

Comment 19

[ovidiu boca[:Ovidiu]]

I verified this on MAc OS X 10.10 with FF Nightly 57.0a1(2017-09-18) and when the default theme is enabled everything works as expected. If you chose "Dark" or "Light" theme the bellow part of the Tab Bar doesn't work on double click. Johann, can you please take a look at this and tell me what do you suggest to do with this, reopen? Thanks

Comment 20

[Johann Hofmann [:johannh]]

(In reply to ovidiu boca[:Ovidiu] from comment #19)
> I verified this on MAc OS X 10.10 with FF Nightly 57.0a1(2017-09-18) and
> when the default theme is enabled everything works as expected. If you chose
> "Dark" or "Light" theme the bellow part of the Tab Bar doesn't work on
> double click. Johann, can you please take a look at this and tell me what do
> you suggest to do with this, reopen? Thanks

Ah, excellent find, thank you! Please file a bug. FYI, this isn't a regression from 57, I can reproduce on Release. I think I know how to solve this for Dark/Light themes, but the issue also exists for lwthemes in general, which is a bit trickier to get right.

Comment 21

[ovidiu boca[:Ovidiu]]

(In reply to Johann Hofmann [:johannh] from comment #20)
> 
> Ah, excellent find, thank you! Please file a bug. FYI, this isn't a
> regression from 57, I can reproduce on Release. I think I know how to solve
> this for Dark/Light themes, but the issue also exists for lwthemes in
> general, which is a bit trickier to get right.

I filed bug 1401195 for the Dark and Light issue. I will mark this as verified fixed.