The default bug view has changed. See this FAQ.

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

VERIFIED FIXED in Firefox 29

Status

()

Firefox
Theme
VERIFIED FIXED
3 years ago
3 years ago

People

(Reporter: Tss, Assigned: mconley)

Tracking

(Depends on: 1 bug, Blocks: 1 bug)

31 Branch
Firefox 31
x86
Windows 7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox29 verified, firefox30 verified, firefox31 verified)

Details

(Whiteboard: [Australis:P3+])

Attachments

(3 attachments, 4 obsolete attachments)

(Reporter)

Description

3 years ago
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

3 years ago
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

3 years ago
Created attachment 8399426 [details]
Nav-Bar.png
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.
Blocks: 872617, 732583
Status: UNCONFIRMED → NEW
Component: Theme → Toolbars and Customization
Ever confirmed: true
No longer blocks: 872617
Whiteboard: [Australis:P3+]

Updated

3 years ago
Component: Toolbars and Customization → Theme
Grrrr - I think this is fallout from bug 986920. *sigh*
Blocks: 986920
As the one likely responsible, I'll see what I can do about this today.
Assignee: nobody → mconley
I'm thinking I might want to back out bug 986920, and apply something a little different.

Patch coming up.
Created attachment 8400162 [details] [diff] [review]
Patch v1 (applied on top of a backout of bug 986920)
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?
Attachment #8400162 - Flags: review?(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 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. :/
Attachment #8400162 - Flags: review?(dao)
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.
Attachment #8400162 - Attachment is obsolete: true
Attachment #8400199 - Flags: review?(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.
(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 :)
Status: NEW → ASSIGNED
(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*
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.
Attachment #8400199 - Attachment is obsolete: true
Attachment #8400199 - Flags: review?(dao)
Attachment #8400226 - Flags: review?(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.
Attachment #8400226 - Flags: review?(dao) → review-
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*.
(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.
(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.
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.
Attachment #8400226 - Attachment is obsolete: true
Comment on attachment 8400854 [details] [diff] [review]
Patch v3.1

What say you, Dao?
Attachment #8400854 - Flags: review?(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?
(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.
Gentle review ping?
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?
Attachment #8400854 - Flags: review?(dao) → review+
(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!
Created attachment 8401427 [details] [diff] [review]
Patch v3.2 (r+'d by dao)

Removed that unnecessary block.
Attachment #8400854 - Attachment is obsolete: true
Attachment #8401427 - Flags: review+
remote:   https://hg.mozilla.org/integration/fx-team/rev/fd93d96bd9a1
Whiteboard: [Australis:P3+] → [Australis:P3+][fixed-in-fx-team]
status-firefox28: --- → affected
status-firefox29: --- → affected
https://hg.mozilla.org/mozilla-central/rev/fd93d96bd9a1
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:P3+][fixed-in-fx-team] → [Australis:P3+]
Target Milestone: --- → Firefox 31

Comment 30

3 years ago
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?
Flags: needinfo?(mconley)

Updated

3 years ago
Depends on: 990099
(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.
status-firefox28: affected → ---
status-firefox30: --- → affected
Flags: needinfo?(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.
Attachment #8401427 - Flags: approval-mozilla-beta?
Attachment #8401427 - Flags: approval-mozilla-aurora?
Attachment #8401427 - Flags: approval-mozilla-beta?
Attachment #8401427 - Flags: approval-mozilla-beta+
Attachment #8401427 - Flags: approval-mozilla-aurora?
Attachment #8401427 - Flags: approval-mozilla-aurora+

Updated

3 years ago
Depends on: 993084

Updated

3 years ago
No longer depends on: 993084
Depends on: 993084

Updated

3 years ago
No longer depends on: 993084
Aurora: https://hg.mozilla.org/releases/mozilla-aurora/rev/1ac9b06bdf90

Beta: https://hg.mozilla.org/releases/mozilla-beta/rev/552251cb84b9
status-firefox29: affected → fixed
status-firefox30: affected → fixed
status-firefox31: --- → fixed
Looks fixed in 31.0a1 (2014-04-08), Win 7 x64.
Status: RESOLVED → VERIFIED
status-firefox31: fixed → verified
Keywords: verifyme
Verified fixed on Windows 7 32bit and 64bit using:
1. latest Aurora, build ID: 20140411004002.
2. Fx 29 beta 7, build ID: 20140410150427
status-firefox29: fixed → verified
status-firefox30: fixed → verified
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.