Closed Bug 1188294 Opened 9 years ago Closed 9 years ago

Tab overflow shadows overlap with toolbar on Windows 10

Categories

(Firefox :: Theme, defect, P3)

40 Branch
Unspecified
Windows 10
defect

Tracking

()

VERIFIED FIXED
Firefox 42
Tracking Status
firefox42 --- verified

People

(Reporter: phlsa, Assigned: Gijs)

References

Details

Attachments

(2 files, 1 obsolete file)

Attached image overlap.png (deleted) —
Since the Windows 10 theme took away the border of the tab strip, the overflow shadows now overlap with the toolbar in a rather unappealing way.
No longer blocks: windows-10
Flags: needinfo?(gijskruitbosch+bugs)
Attached patch Patch (obsolete) (deleted) — Splinter Review
This will be impacted by bug 1186244 if we do decide to get rid of the negative top margin (which I think is questionable), but this seems simpler and more visible (ie is also a problem on non-lwts), so I'd like to get this in either way.
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Flags: needinfo?(gijskruitbosch+bugs)
Attachment #8644453 - Flags: review?(dao)
Comment on attachment 8644453 [details] [diff] [review] Patch Would it make sense to use another variable for the second overlap? Something like --tab-toolbar-navbar-overlap-border vs. --tab-toolbar-navbar-overlap-highlight? I'm not sure (the comment doesn't seem to explain) why we'd need 1px on both Win10 and non-Win10. Should the margin be a combination of both overlaps, i.e. should non-Win10 use 2px here?
Attachment #8644453 - Flags: review?(dao)
(In reply to Dão Gottwald [:dao] from comment #3) > I'm not sure (the comment doesn't seem to explain) why we'd need 1px on both > Win10 and non-Win10. Should the margin be a combination of both overlaps, > i.e. should non-Win10 use 2px here? Answering my own question, I think non-Win10 currently wants 1px not because of the border overlap (i.e. your comment is wrong) but because of the highlight overlap, just like Win10. We probably don't want to combine both overlaps here because these buttons have a white border that's expected to cover the toolbar border and touch the toolbar highlight.
(In reply to Dão Gottwald [:dao] from comment #4) > (In reply to Dão Gottwald [:dao] from comment #3) > > I'm not sure (the comment doesn't seem to explain) why we'd need 1px on both > > Win10 and non-Win10. Should the margin be a combination of both overlaps, > > i.e. should non-Win10 use 2px here? > > Answering my own question, I think non-Win10 currently wants 1px not because > of the border overlap (i.e. your comment is wrong) but because of the > highlight overlap, just like Win10. I mean, it depends how you read it. The total overlap is going to be 2px unless you adjust the margin. We only want to overlap the border (and not the highlight), so we make the margin-bottom 1px meaning there's 1px overlap (of the original 2). You're trying to answer "why margin-bottom: 1 and not 0", and I was trying to answer "why margin-bottom: 1 and not 2". If you still find the comment unclear, then please suggest something you would prefer. (In reply to Dão Gottwald [:dao] from comment #3) > Comment on attachment 8644453 [details] [diff] [review] > Patch > > Would it make sense to use another variable for the second overlap? > Something like --tab-toolbar-navbar-overlap-border vs. > --tab-toolbar-navbar-overlap-highlight? I don't really think so. It used to be the case that we had a border everywhere, and devedition sets the existing variable to 0 and then works fine (because it does still have a border), so from that perspective the name of the existing variable is just wrong. Basically, at this point it's quite difficult to reverse-engineer variables into the existing margins, because of the negative margins on both the tab and the navbar, and how we're breaking assumptions on windows 10 that "even" devedition (which also sets the pre-existing variable to 0) doesn't break (namely: a border between the navbar and tabbar). I don't think that should be part of this bug, especially not on 41 which will be beta next week, if a one-liner can just fix the issue at hand.
Flags: needinfo?(dao)
Maybe: + * This gets pulled up by 1px so it *only* overlaps with the dark border (and not the highlight) between the tabs and the navbar. would be clearer?
(In reply to :Gijs Kruitbosch from comment #5) > (In reply to Dão Gottwald [:dao] from comment #4) > > (In reply to Dão Gottwald [:dao] from comment #3) > > > I'm not sure (the comment doesn't seem to explain) why we'd need 1px on both > > > Win10 and non-Win10. Should the margin be a combination of both overlaps, > > > i.e. should non-Win10 use 2px here? > > > > Answering my own question, I think non-Win10 currently wants 1px not because > > of the border overlap (i.e. your comment is wrong) but because of the > > highlight overlap, just like Win10. > > I mean, it depends how you read it. The total overlap is going to be 2px > unless you adjust the margin. We only want to overlap the border (and not > the highlight), so we make the margin-bottom 1px meaning there's 1px overlap > (of the original 2). You're trying to answer "why margin-bottom: 1 and not > 0", and I was trying to answer "why margin-bottom: 1 and not 2". No, I was posing and answering the latter question, and when I said "overlap" I was referring to the tabs toolbar and the navbar overlapping which we're accounting for here. I still don't understand why introducing and using a second variable for the highlight overlap wouldn't be an appropriate solution here.
Flags: needinfo?(dao)
(In reply to Dão Gottwald [:dao] from comment #7) > (In reply to :Gijs Kruitbosch from comment #5) > > (In reply to Dão Gottwald [:dao] from comment #4) > > > (In reply to Dão Gottwald [:dao] from comment #3) > > > > I'm not sure (the comment doesn't seem to explain) why we'd need 1px on both > > > > Win10 and non-Win10. Should the margin be a combination of both overlaps, > > > > i.e. should non-Win10 use 2px here? > > > > > > Answering my own question, I think non-Win10 currently wants 1px not because > > > of the border overlap (i.e. your comment is wrong) but because of the > > > highlight overlap, just like Win10. > > > > I mean, it depends how you read it. The total overlap is going to be 2px > > unless you adjust the margin. We only want to overlap the border (and not > > the highlight), so we make the margin-bottom 1px meaning there's 1px overlap > > (of the original 2). You're trying to answer "why margin-bottom: 1 and not > > 0", and I was trying to answer "why margin-bottom: 1 and not 2". > > No, I was posing and answering the latter question, and when I said > "overlap" I was referring to the tabs toolbar and the navbar overlapping > which we're accounting for here. > > I still don't understand why introducing and using Using where? Presumably you mean more than just this 1 place, which by itself wouldn't add a lot of value... win10 right now still has -1 margin between the tabbar and navbar, and everything else has -2. devedition has 0 overlap. The bottom margin for this shadow thing needs to be 1px everywhere, except devedition (which hardcodes it to 0). It is not clear to me how that corresponds to any of the overlaps. In fact, the more I think about it, the more I think that this value needs to take into account whether or not there is a (dark) border at the top of the navbar. The fact that there isn't on win10 then means that we want the shadow to be positioned *higher* than we'd want it to be if there *was* a border (viz. devedition where the margin-bottom is set to 0). Maybe (some mathematically identical version of) the following would work ? calc(-1 * calc(var(neg-margin-on-tabbar) + var(neg-margin-on-navbar) + var(navbar-top-border-width)) -1 * (-1 (0 on win10 and deved) + -1 (0 on deved) + 1 (0 on win10))
Flags: needinfo?(dao)
(In reply to :Gijs Kruitbosch from comment #8) > > I still don't understand why introducing and using > > Using where? Presumably you mean more than just this 1 place, which by > itself wouldn't add a lot of value... Also here: http://hg.mozilla.org/mozilla-central/annotate/d6ea652c5799/browser/themes/windows/browser.css#l922 and here: http://hg.mozilla.org/mozilla-central/annotate/d6ea652c5799/browser/themes/windows/browser.css#l310 Same for OS X and Linux. > It is not clear to me how that corresponds to any of the overlaps. In fact, > the more I think about it, the more I think that this value needs to take > into account whether or not there is a (dark) border at the top of the > navbar. I don't understand. We've established that we want 1px here for both Win10 and non-Win10, i.e. regardless of whether there's a dark border, have we not? That's because the tabs toolbar and nav bar overlap by 1px for the highlight. We may change this on Win10 as part of or as a followup to bug 1186244, i.e. we'd then set the variable to 0 for Win10...
Flags: needinfo?(dao)
Not sure why we can't just reduce the z-index of the shadow on Windows 10. Fixes the issue.
(In reply to Tim Nguyen [:ntim] (mostly away until 26 August) from comment #10) > Not sure why we can't just reduce the z-index of the shadow on Windows 10. > Fixes the issue. That sounds like a workaround, not a proper fix. We just need to position the button correctly, then we don't need to hide the part that's in the wrong place.
(In reply to Tim Nguyen [:ntim] (mostly away until 26 August) from comment #10) > Not sure why we can't just reduce the z-index of the shadow on Windows 10. > Fixes the issue. Also, I'd imagine that interferes with ensuring it goes over the top of the selected tab (which itself goes over the top of the navbar).
Attached patch Patch v2 (deleted) — Splinter Review
Tested on win10 and win8, nowhere else, but assuming I got the replaces right I don't think there should be any issues?
Attachment #8644453 - Attachment is obsolete: true
Attachment #8645207 - Flags: review?(dao)
Attachment #8645207 - Flags: review?(dao) → review+
Flags: qe-verify+
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
Mozilla/5.0 (Windows NT 10.0; WOW64; rv:42.0) Gecko/20100101 Firefox/42.0 Confirming the fix on Windows 10 64-bit using latest Aurora, build ID: 20150920004018. Filled bug 1206671 as I've noticed that there's a small offset for the "Open A new tab" and "List All tabs" buttons.
Status: RESOLVED → VERIFIED
QA Contact: cornel.ionce
Depends on: 1206671
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: