Last Comment Bug 989761 - [Windows Classic] Pinned tab separator overlapping nav-bar when in overflow mode
: [Windows Classic] Pinned tab separator overlapping nav-bar when in overflow mode
Status: VERIFIED FIXED
[Australis:P3+]
:
Product: Firefox
Classification: Client Software
Component: Theme (show other bugs)
: 31 Branch
: x86 Windows 7
-- normal (vote)
: Firefox 31
Assigned To: Mike Conley (:mconley)
:
: Dão Gottwald [:dao]
Mentors:
Depends on: 990099
Blocks: australis-tabs 986920
  Show dependency treegraph
 
Reported: 2014-03-30 09:11 PDT by Tss
Modified: 2014-04-11 06:01 PDT (History)
11 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
verified
verified
verified


Attachments
bug.png (16.06 KB, image/png)
2014-03-30 09:11 PDT, Tss
no flags Details
Nav-Bar.png (39.26 KB, image/png)
2014-03-31 07:24 PDT, Pedro
no flags Details
Patch v1 (applied on top of a backout of bug 986920) (1.26 KB, patch)
2014-04-01 13:14 PDT, Mike Conley (:mconley)
no flags Details | Diff | Splinter Review
Patch v2 (1.99 KB, patch)
2014-04-01 14:25 PDT, Mike Conley (:mconley)
no flags Details | Diff | Splinter Review
Patch v3 (2.07 KB, patch)
2014-04-01 15:15 PDT, Mike Conley (:mconley)
dao+bmo: review-
Details | Diff | Splinter Review
Patch v3.1 (3.92 KB, patch)
2014-04-02 13:29 PDT, Mike Conley (:mconley)
dao+bmo: review+
Details | Diff | Splinter Review
Patch v3.2 (r+'d by dao) (3.99 KB, patch)
2014-04-03 10:23 PDT, Mike Conley (:mconley)
mconley: review+
sledru: approval‑mozilla‑aurora+
sledru: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description User image Tss 2014-03-30 09:11:38 PDT
Created attachment 8399114 [details]
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 User image Pedro 2014-03-31 07:24:22 PDT
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 User image Pedro 2014-03-31 07:24:55 PDT
Created attachment 8399426 [details]
Nav-Bar.png
Comment 3 User image Paul Silaghi, QA [:pauly] 2014-04-01 06:56:38 PDT
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 User image Mike Conley (:mconley) 2014-04-01 07:48:26 PDT
Grrrr - I think this is fallout from bug 986920. *sigh*
Comment 5 User image Mike Conley (:mconley) 2014-04-01 07:54:01 PDT
As the one likely responsible, I'll see what I can do about this today.
Comment 6 User image Mike Conley (:mconley) 2014-04-01 12:55:20 PDT
I'm thinking I might want to back out bug 986920, and apply something a little different.

Patch coming up.
Comment 7 User image Mike Conley (:mconley) 2014-04-01 13:14:49 PDT
Created attachment 8400162 [details] [diff] [review]
Patch v1 (applied on top of a backout of bug 986920)
Comment 8 User image Mike Conley (:mconley) 2014-04-01 13:23:19 PDT
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 User image Dão Gottwald [:dao] 2014-04-01 13:46:00 PDT
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 User image Mike Conley (:mconley) 2014-04-01 14:18:37 PDT
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 User image Mike Conley (:mconley) 2014-04-01 14:25:37 PDT
Created attachment 8400199 [details] [diff] [review]
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 User image Dão Gottwald [:dao] 2014-04-01 14:58:29 PDT
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 User image Matthew N. [:MattN] (PM if requests are blocking you) 2014-04-01 15:04:57 PDT
(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 User image Mike Conley (:mconley) 2014-04-01 15:06:08 PDT
(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 User image Mike Conley (:mconley) 2014-04-01 15:15:59 PDT
Created attachment 8400226 [details] [diff] [review]
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 User image Dão Gottwald [:dao] 2014-04-01 15:20:09 PDT
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 User image Mike Conley (:mconley) 2014-04-01 15:24:47 PDT
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 User image Dão Gottwald [:dao] 2014-04-01 15:27:21 PDT
(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 User image Mike Conley (:mconley) 2014-04-01 16:09:52 PDT
(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 User image Mike Conley (:mconley) 2014-04-02 13:29:27 PDT
Created attachment 8400854 [details] [diff] [review]
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 User image Mike Conley (:mconley) 2014-04-02 13:31:37 PDT
Comment on attachment 8400854 [details] [diff] [review]
Patch v3.1

What say you, Dao?
Comment 22 User image Dão Gottwald [:dao] 2014-04-02 14:35:26 PDT
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 User image Mike Conley (:mconley) 2014-04-02 14:38:35 PDT
(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 User image Mike Conley (:mconley) 2014-04-03 09:43:13 PDT
Gentle review ping?
Comment 25 User image Dão Gottwald [:dao] 2014-04-03 10:09:19 PDT
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 User image Mike Conley (:mconley) 2014-04-03 10:22:05 PDT
(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 User image Mike Conley (:mconley) 2014-04-03 10:23:41 PDT
Created attachment 8401427 [details] [diff] [review]
Patch v3.2 (r+'d by dao)

Removed that unnecessary block.
Comment 28 User image Mike Conley (:mconley) 2014-04-04 09:54:56 PDT
remote:   https://hg.mozilla.org/integration/fx-team/rev/fd93d96bd9a1
Comment 29 User image Phil Ringnalda (:philor) 2014-04-06 09:55:40 PDT
https://hg.mozilla.org/mozilla-central/rev/fd93d96bd9a1
Comment 30 User image :Gijs 2014-04-07 05:03:32 PDT
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 User image Mike Conley (:mconley) 2014-04-07 08:51:27 PDT
(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 User image Mike Conley (:mconley) 2014-04-07 08:53:06 PDT
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 34 User image Paul Silaghi, QA [:pauly] 2014-04-09 02:39:48 PDT
Looks fixed in 31.0a1 (2014-04-08), Win 7 x64.
Comment 35 User image Cornel Ionce [QA] (:cornel_ionce) 2014-04-11 06:01:06 PDT
Verified fixed on Windows 7 32bit and 64bit using:
1. latest Aurora, build ID: 20140411004002.
2. Fx 29 beta 7, build ID: 20140410150427

Note You need to log in before you can comment on or make changes to this bug.