Closed
Bug 989761
Opened 9 years ago
Closed 9 years ago
[Windows Classic] Pinned tab separator overlapping nav-bar when in overflow mode
Categories
(Firefox :: Theme, defect)
Tracking
()
VERIFIED
FIXED
Firefox 31
People
(Reporter: thalesplsantos, Assigned: mconley)
References
(Depends on 1 open bug)
Details
(Whiteboard: [Australis:P3+])
Attachments
(3 files, 4 obsolete files)
16.06 KB,
image/png
|
Details | |
39.26 KB,
image/png
|
Details | |
3.99 KB,
patch
|
mconley
:
review+
Sylvestre
:
approval-mozilla-aurora+
Sylvestre
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
Using Windows Classic theme, pinned tab separators overlap nav-bar when tabs are in overflow mode. Doesn't happen with Aero Basic or Glass.
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 3•9 years ago
|
||
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: australis-cust, australis-tabs
Status: UNCONFIRMED → NEW
Component: Theme → Toolbars and Customization
Ever confirmed: true
Updated•9 years ago
|
No longer blocks: australis-cust
Whiteboard: [Australis:P3+]
Updated•9 years ago
|
Component: Toolbars and Customization → Theme
Assignee | ||
Comment 4•9 years ago
|
||
Grrrr - I think this is fallout from bug 986920. *sigh*
Blocks: 986920
Assignee | ||
Comment 5•9 years ago
|
||
As the one likely responsible, I'll see what I can do about this today.
Assignee: nobody → mconley
Assignee | ||
Comment 6•9 years ago
|
||
I'm thinking I might want to back out bug 986920, and apply something a little different. Patch coming up.
Assignee | ||
Comment 7•9 years ago
|
||
Assignee | ||
Comment 8•9 years ago
|
||
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 9•9 years ago
|
||
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?
Assignee | ||
Comment 10•9 years ago
|
||
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)
Assignee | ||
Comment 11•9 years ago
|
||
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 12•9 years ago
|
||
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•9 years ago
|
||
(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
Assignee | ||
Comment 14•9 years ago
|
||
(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*
Assignee | ||
Comment 15•9 years ago
|
||
(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 16•9 years ago
|
||
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-
Assignee | ||
Comment 17•9 years ago
|
||
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•9 years ago
|
||
(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.
Assignee | ||
Comment 19•9 years ago
|
||
(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.
Assignee | ||
Comment 20•9 years ago
|
||
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
Assignee | ||
Comment 21•9 years ago
|
||
Comment on attachment 8400854 [details] [diff] [review] Patch v3.1 What say you, Dao?
Attachment #8400854 -
Flags: review?(dao)
Comment 22•9 years ago
|
||
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?
Assignee | ||
Comment 23•9 years ago
|
||
(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.
Assignee | ||
Comment 24•9 years ago
|
||
Gentle review ping?
Comment 25•9 years ago
|
||
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+
Assignee | ||
Comment 26•9 years ago
|
||
(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!
Assignee | ||
Comment 27•9 years ago
|
||
Removed that unnecessary block.
Attachment #8400854 -
Attachment is obsolete: true
Attachment #8401427 -
Flags: review+
Assignee | ||
Comment 28•9 years ago
|
||
remote: https://hg.mozilla.org/integration/fx-team/rev/fd93d96bd9a1
Whiteboard: [Australis:P3+] → [Australis:P3+][fixed-in-fx-team]
Assignee | ||
Updated•9 years ago
|
status-firefox28:
--- → affected
status-firefox29:
--- → affected
Comment 29•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/fd93d96bd9a1
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:P3+][fixed-in-fx-team] → [Australis:P3+]
Target Milestone: --- → Firefox 31
Comment 30•9 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)
Assignee | ||
Comment 31•9 years ago
|
||
(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.
Assignee | ||
Comment 32•9 years ago
|
||
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?
Updated•9 years ago
|
Attachment #8401427 -
Flags: approval-mozilla-beta?
Attachment #8401427 -
Flags: approval-mozilla-beta+
Attachment #8401427 -
Flags: approval-mozilla-aurora?
Attachment #8401427 -
Flags: approval-mozilla-aurora+
Assignee | ||
Comment 33•9 years ago
|
||
Aurora: https://hg.mozilla.org/releases/mozilla-aurora/rev/1ac9b06bdf90 Beta: https://hg.mozilla.org/releases/mozilla-beta/rev/552251cb84b9
Updated•9 years ago
|
status-firefox31:
--- → fixed
Comment 35•9 years ago
|
||
Verified fixed on Windows 7 32bit and 64bit using: 1. latest Aurora, build ID: 20140411004002. 2. Fx 29 beta 7, build ID: 20140410150427
You need to log in
before you can comment on or make changes to this bug.
Description
•