989761 - [Windows Classic] Pinned tab separator overlapping nav-bar when in overflow mode

Status: VERIFIED FIXED

(Firefox :: Theme, defect)

Description

[Tss]

Attachment: bug.png

Using Windows Classic theme, pinned tab separators overlap nav-bar when tabs are in overflow mode. Doesn't happen with Aero Basic or Glass.

Comment 1

[Pedro]

Dear Tss,

I have tried to replicate the bug you described in Firefox Nightly, using Windows Classic theme, but I'm unsure of what the problem is... The only difference between us is that I'm using Windows 7 x64, but that shouldn't have any impact on the matter.

I send an attachment of what happened to me (Nav-Bar)...

Comment 2

[Pedro]

Attachment: Nav-Bar.png

Comment 3

[Paul Silaghi, QA [:pauly]]

Confirmed in 31.0a1 (2014-03-31), win 7 x64.

(In reply to Pedro from comment #1)
> I'm unsure of what the problem is... 
Take a closer look at your screenshot at the tab separators using the 'magnifier' tool.

Comment 4

[Mike Conley (:mconley) (:⚙️)]

Grrrr - I think this is fallout from bug 986920. *sigh*

Comment 5

[Mike Conley (:mconley) (:⚙️)]

As the one likely responsible, I'll see what I can do about this today.

Comment 6

[Mike Conley (:mconley) (:⚙️)]

I'm thinking I might want to back out bug 986920, and apply something a little different.

Patch coming up.

Comment 7

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v1 (applied on top of a backout of bug 986920)

Comment 8

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8400162 [details] [diff] [review]
Patch v1 (applied on top of a backout of bug 986920)

From my testing, backing out bug 989761, and then applying this fixes:

1) Bug 989761 via alternate means
2) This bug
3) Bug 990099

Thoughts, Dao? Can you think of ways this might go wrong?

Comment 9

[Dão Gottwald [:dao]]

Comment on attachment 8400162 [details] [diff] [review]
Patch v1 (applied on top of a backout of bug 986920)

> @media (-moz-windows-classic) {
>-  #main-window[tabsintitlebar]:not([sizemode=fullscreen]) .tabbrowser-tab:not([selected=true]),
>-  #main-window[tabsintitlebar]:not([sizemode=fullscreen]) .tabs-newtab-button {
>+  /**
>+   * We need to bump up the z-index of the unselected tabs so that they
>+   * take priority over the new tab button for hovering.
>+   */
>+  #main-window[tabsintitlebar]:not([sizemode=fullscreen]) .tabbrowser-tab:not([selected=true]) {
>     position: relative;
>     z-index: 1;
>   }

If .tabs-newtab-button doesn't have position:relative anymore, what's special about -moz-windows-classic so that unselected tabs need special treatment to overlap the new tab button?

Comment 10

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8400162 [details] [diff] [review]
Patch v1 (applied on top of a backout of bug 986920)

Ah, this is no good - this puts the fog over-top of the new tab button. :/

Comment 11

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v2

It all has to do with the fog we applied to the titlebar in classic mode in bug 978752. That fog has a z-index of 0, and we need to make sure our layering is right on top of it.

This patch sidesteps the issue raised in bug 986920 by bumping the z-index of the icon only.

Comment 12

[Dão Gottwald [:dao]]

Comment on attachment 8400199 [details] [diff] [review]
Patch v2

Wouldn't it be simpler to set a z-index on #tabbrowser-tabs? Setting a z-index on every individual child seems messy and fragile and is already missing tab-drop-indicator.

Comment 13

[Matthew N. [:MattN]]

(In reply to Mike Conley (:mconley) from comment #8)
> From my testing, backing out bug 989761, and then applying this fixes:
> 
> 1) Bug 989761 via alternate means
> 2) This bug

What was the third? 1 & 2 are the same :)

Comment 14

[Mike Conley (:mconley) (:⚙️)]

(In reply to Matthew N. [:MattN] from comment #13)
> (In reply to Mike Conley (:mconley) from comment #8)
> > From my testing, backing out bug 989761, and then applying this fixes:
> > 
> > 1) Bug 989761 via alternate means
> > 2) This bug
> 
> What was the third? 1 & 2 are the same :)

Bah, I meant bug 986920. *sigh*

Comment 15

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v3

(In reply to Dão Gottwald [:dao] from comment #12)
> Comment on attachment 8400199 [details] [diff] [review]
> Patch v2
> 
> Wouldn't it be simpler to set a z-index on #tabbrowser-tabs? Setting a
> z-index on every individual child seems messy and fragile and is already
> missing tab-drop-indicator.

Oh, yes yes yes! I'd forgotten about the tabbrowser-tabs element. I think this is far more resilient, and appears to work.

Comment 16

[Dão Gottwald [:dao]]

Comment on attachment 8400226 [details] [diff] [review]
Patch v3

Please use #tabbrowser-tabs (id, not class).

Furthermore, can you also stop setting a z-index on the scroll buttons? Can position:relative be removed from .scrollbox-innerbox? Seems like this stuff shouldn't be needed anymore, but there are no comments, so I'm not sure what exactly it's meant to achieve.

Comment 17

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8400226 [details] [diff] [review]
Patch v3

It's also no good because it re-causes the bug that's the root of this bug report. *sigh*.

Comment 18

[Dão Gottwald [:dao]]

(In reply to Mike Conley (:mconley) from comment #17)
> Comment on attachment 8400226 [details] [diff] [review]
> Patch v3
> 
> It's also no good because it re-causes the bug that's the root of this bug
> report. *sigh*.

Likely because #nav-bar has a z-index of only 1.

Comment 19

[Mike Conley (:mconley) (:⚙️)]

(In reply to Dão Gottwald [:dao] from comment #18)
> (In reply to Mike Conley (:mconley) from comment #17)
> > Comment on attachment 8400226 [details] [diff] [review]
> > Patch v3
> > 
> > It's also no good because it re-causes the bug that's the root of this bug
> > report. *sigh*.
> 
> Likely because #nav-bar has a z-index of only 1.

Yes, bumping that helped.

But now I've got another conundrum - setting the z-index of #tabbrowser-tabs doesn't allow me to put the .tabbrowser-tab[selected] on top of the nav-bar, so we get this ugly seam where the selected tab intersects the nav-bar highlight.

I don't think I'm going to be much more use on this one today. If somebody else wants to take it, feel free, otherwise, I'll see if I can hammer through it tomorrow.

Comment 20

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v3.1

Ok, this switches us to #tabbrowser-tabs.

I also fixed the following other cases I noticed:

1) Pinned tab separators were overlapping the nav-bar when the tabstrip was overflowing due to the position: absolute / height: 100% applied to them in that case.
2) Scrollbox button borders were leaking over the nav-bar.

Comment 21

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8400854 [details] [diff] [review]
Patch v3.1

What say you, Dao?

Comment 22

[Dão Gottwald [:dao]]

Comment on attachment 8400854 [details] [diff] [review]
Patch v3.1

This is starting to look reasonable, though I have to ask: Why is this so more complicated than what we're doing for the Aero Glass fog?

Comment 23

[Mike Conley (:mconley) (:⚙️)]

(In reply to Dão Gottwald [:dao] from comment #22)
> Comment on attachment 8400854 [details] [diff] [review]
> Patch v3.1
> 
> This is starting to look reasonable, though I have to ask: Why is this so
> more complicated than what we're doing for the Aero Glass fog?

I think it's because for Aero Glass, there's nothing behind the fog, so we can z-index it wayyyyyyy to the back. In classic, we've got the titlebar texture to contend with - the fog must go _in front_ of the titlebar texture, but behind everything else.

Comment 24

[Mike Conley (:mconley) (:⚙️)]

Gentle review ping?

Comment 25

[Dão Gottwald [:dao]]

Comment on attachment 8400854 [details] [diff] [review]
Patch v3.1

>   #main-window[tabsintitlebar][sizemode=normal] .tabbrowser-arrowscrollbox > .arrowscrollbox-scrollbox > .scrollbox-innerbox:not(:-moz-lwtheme) {
>     position: relative;
>   }

This is still needed?

Comment 26

[Mike Conley (:mconley) (:⚙️)]

(In reply to Dão Gottwald [:dao] from comment #25)
> Comment on attachment 8400854 [details] [diff] [review]
> Patch v3.1
> 
> >   #main-window[tabsintitlebar][sizemode=normal] .tabbrowser-arrowscrollbox > .arrowscrollbox-scrollbox > .scrollbox-innerbox:not(:-moz-lwtheme) {
> >     position: relative;
> >   }
> 
> This is still needed?

No, that doesn't appear to be necessary in the slightest.

Thanks Dao!

Comment 27

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v3.2 (r+'d by dao)

Removed that unnecessary block.

Comment 28

[Mike Conley (:mconley) (:⚙️)]

remote:   https://hg.mozilla.org/integration/fx-team/rev/fd93d96bd9a1

Comment 29

[Phil Ringnalda (:philor)]

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

Comment 30

[:Gijs (he/him)]

Mike, did you mean to set 30 and 29 as affected, rather than 28 and 29? Also, is this safe enough for beta/aurora approval?

Comment 31

[Mike Conley (:mconley) (:⚙️)]

(In reply to :Gijs Kruitbosch from comment #30)
> Mike, did you mean to set 30 and 29 as affected, rather than 28 and 29?

Er, yup, I apparently can't count.

> Also, is this safe enough for beta/aurora approval?

Yeah, I think so - will request approval now that it's had time to bake.

Comment 32

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8401427 [details] [diff] [review]
Patch v3.2 (r+'d by dao)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 

bug 986920


User impact if declined: 

When users are using the Windows Classic theme, if they have pinned tabs, the pinned tab separators will leak into the nav-bar and look pretty bad.


Testing completed (on m-c, etc.): 

Lots of local testing, and some days baking on m-c.


Risk to taking this patch (and alternatives if risky): 

Low risk - it's style only, and is isolated to the classic theme rules.


String or IDL/UUID changes made by this patch:

None.

Comment 33

[Mike Conley (:mconley) (:⚙️)]

Aurora: https://hg.mozilla.org/releases/mozilla-aurora/rev/1ac9b06bdf90

Beta: https://hg.mozilla.org/releases/mozilla-beta/rev/552251cb84b9

Comment 34

[Paul Silaghi, QA [:pauly]]

Looks fixed in 31.0a1 (2014-04-08), Win 7 x64.

Comment 35

[Cornel Ionce [:noni] [Hubs QA]]

Verified fixed on Windows 7 32bit and 64bit using:
1. latest Aurora, build ID: 20140411004002.
2. Fx 29 beta 7, build ID: 20140410150427