See http://design.firefox.com/people/shorlander/photon/Mockups/macOS.html

Hey Dão, I took your second patch from bug 1387754 and tweaked it based on what I was saying in our discussion. There might be some rough edges here and there, but I'm at a point where I can't really optimize it any further and so I'd like to get your opinion on it :)

Comment on attachment 8898651 [details] Bug 1391539 - Show tab separators when hovering tabs. https://reviewboard.mozilla.org/r/170024/#review175194 ::: browser/themes/shared/tabs.inc.css:316 (Diff revision 1) > .tab-background { > border: 1px none transparent; > background-clip: padding-box; > } > > +%ifdef MENUBAR_CAN_AUTOHIDE This part is just for doing what bug 1391328 will hopefully do anyway, it would look weird on OSX otherwise.

Oh, important to mention: This is based on your patch that landed in bug 1387754, so you'd need to import on top of that one (or wait until it's in central).

Dão, this patch changed a lot after I rebased. When testing I noticed a couple of things that had changed/some additional issues, but I think the patch is a lot better now. I tried to explain most changes in comments. As mentioned in the commit message, I made two additional fixes that are technically not part of this bug, but I don't think we should land this without them. There is still an issue with the leftmost border added in bug 1390025 (it's 1px too large), but I think that can/should be resolved through solving the dependencies of bug 1390025. It would be great to get review on this quickly so that we can still uplift.

Dão, this has been waiting for two months and I'm not sure if we can make 57 anymore, which would be a shame. Are you going to review it this week or should I re-route to someone else?

Gijs, can you take a look please? Let me know if you have any questions. Sorry for pulling you in.

I'm really sorry, but not having been intimately involved in the visual work, I don't understand what this patch is supposed to be doing. Comment #0 points to the mockup, but I see tab separators there without hovering. Likewise, I see them on beta and nightly without hovering, and they stretch all the way without hovering. Your patch doesn't seem to change that. It's also confusing that the commit message of the patch is "show tab separators", when it seems to me they are already shown anyway. Are they supposed to be thicker? Longer? Different colour? The patch code seems to suggest several of these, but I see no difference on my win10 machine where I'm testing. Maybe it's too subtle for me to notice, but without knowing what to look for it's hard to be sure... Can you clarify where/how I am supposed to see a difference with/without this patch? What is it actually fixing?

OK, I've figured at least part of this out - the separators disappear when hovering without your patch. Is that everything that's changing here?

On my win10 machine (175% dpi/whatever), I see seams between the hover background and the separators.

(In reply to :Gijs from comment #14) > Created attachment 8914487 [details] > seam-separator.png > > On my win10 machine (175% dpi/whatever), I see seams between the hover > background and the separators. Whereas on the light theme, the end-side (right-hand) separator is displaced by 1px into the hover background, which makes it look darker/thicker because it overlays the hover background.

Comment on attachment 8898651 [details] Bug 1391539 - Show tab separators when hovering tabs. https://reviewboard.mozilla.org/r/170024/#review190740 Not sure this is the right approach given the visual issues I'm seeing, but also still not sure what else I should be checking.

Gijs, thank you very much for taking a look at this so quickly despite not having been involved in the sub-project a lot. > OK, I've figured at least part of this out - the separators disappear when hovering without your patch. Is that everything that's changing here? Yes. > On my win10 machine (175% dpi/whatever), I see seams between the hover background and the separators. For some reason, on certain DPI settings, the pseudo-placeholders were not entirely overlapped by their borders leaving a little space and pushing the tab-background slightly to the right. I didn't investigate into this further. Instead, this bug takes a new approach and replaces the old ::before and ::after pseudo-selector separators with a regular border around tabs. According to Dão, they were only there to hide and show the separators flexibly without affecting layout and we don't need to do that anymore. It's a little bigger patch now but it shouldn't actually be that complex, most of the added code just caters to existing edge cases that were covered differently before. Dão said he'd be able to review it asap, but I'm leaving the review flag for Gijs on in case you have any further remarks.

Comment on attachment 8898651 [details] Bug 1391539 - Show tab separators when hovering tabs. https://reviewboard.mozilla.org/r/170024/#review191804 ::: browser/themes/shared/tabs.inc.css:570 (Diff revision 6) > +.tabbrowser-tab { > + border-inline-start: 1px solid; > + border-color: var(--tab-separator-color); > border-image: linear-gradient(transparent 6px, > - currentColor 6px, > - currentColor calc(100% - 5px), > + var(--tab-separator-color) 6px, > + var(--tab-separator-color) calc(100% - 5px), As far as I can tell, you can keep using ::after and ::before here with currentColor and opacity, but without the width and negative margin. This avoids any issues with [brighttext] and should make this patch smaller.

Comment on attachment 8898651 [details] Bug 1391539 - Show tab separators when hovering tabs. https://reviewboard.mozilla.org/r/170024/#review191804 > As far as I can tell, you can keep using ::after and ::before here with currentColor and opacity, but without the width and negative margin. This avoids any issues with [brighttext] and should make this patch smaller. Ok, let me try that. To be honest, I prefer the border version, but it might be better to deliver a low-risk patch here.

Comment on attachment 8898651 [details] Bug 1391539 - Show tab separators when hovering tabs. https://reviewboard.mozilla.org/r/170024/#review191864 Clearing review given the last few comments on the bug (and to get this off my list).

Comment on attachment 8898651 [details] Bug 1391539 - Show tab separators when hovering tabs. https://reviewboard.mozilla.org/r/170024/#review192180 ::: browser/themes/shared/tabs.inc.css:10 (Diff revision 7) > %endif > %filter substitution > %define horizontalTabPadding 9px > > :root { > + --tabs-top-border: 0px; nit: --tabs-top-border-width ::: browser/themes/shared/tabs.inc.css:588 (Diff revision 7) > %endif > +/* Show full height tab separators on hover. */ > +.tabbrowser-tab:hover::before, > +.tabbrowser-tab[last-visible-tab]:hover::after, > +#tabbrowser-tabs:not([movingtab]) > .tabbrowser-tab[afterhovered]::before { > + border-image: linear-gradient(transparent calc(1px + var(--tabs-top-border)), Why 1px + ...? ::: browser/themes/shared/tabs.inc.css:599 (Diff revision 7) > + > +/* Show full height tab separators on selected tabs. */ > +#tabbrowser-tabs[movingtab] > .tabbrowser-tab[selected]::after, > +#tabbrowser-tabs:not([movingtab]) > .tabbrowser-tab[afterselected]::before, > +.tabbrowser-tab[selected]::before, > +.tabbrowser-tab[selected][last-visible-tab]::after { Need to use [visuallyselected] instead of [selected] throughout this rule ::: browser/themes/shared/tabs.inc.css:599 (Diff revision 7) > + > +/* Show full height tab separators on selected tabs. */ > +#tabbrowser-tabs[movingtab] > .tabbrowser-tab[selected]::after, > +#tabbrowser-tabs:not([movingtab]) > .tabbrowser-tab[afterselected]::before, > +.tabbrowser-tab[selected]::before, > +.tabbrowser-tab[selected][last-visible-tab]::after { I think these selectors can be simplified since e.g. without [last-visible-tab], .tabbrowser-tab[selected]::after isn't displayed anyway. ::: browser/themes/shared/tabs.inc.css:609 (Diff revision 7) > + transparent calc(100% - 1px - var(--tab-toolbar-navbar-overlap))) 1 !important; > + opacity: 1; > +} > > /* Also show separators beside the selected tab when dragging it. */ > -#tabbrowser-tabs[movingtab] > .tabbrowser-tab[beforeselected]:not([last-visible-tab])::after, > +#tabbrowser-tabs[movingtab] > .tabbrowser-tab[selected]::after, ditto

(In reply to Dão Gottwald [::dao] from comment #24) > > -#tabbrowser-tabs[movingtab] > .tabbrowser-tab[beforeselected]:not([last-visible-tab])::after, > > +#tabbrowser-tabs[movingtab] > .tabbrowser-tab[selected]::after, > > ditto That was referring to this: > > +#tabbrowser-tabs[movingtab] > .tabbrowser-tab[selected]::after, > > +#tabbrowser-tabs:not([movingtab]) > .tabbrowser-tab[afterselected]::before, > > +.tabbrowser-tab[selected]::before, > > +.tabbrowser-tab[selected][last-visible-tab]::after { > > Need to use [visuallyselected] instead of [selected] throughout this rule

Comment on attachment 8898651 [details] Bug 1391539 - Show tab separators when hovering tabs. https://reviewboard.mozilla.org/r/170024/#review192180 > Why 1px + ...? To be honest, I'm not sure. The ::before element grows one px too large here for a reason I don't know, but it's consistent across platforms/UI density.

Comment on attachment 8898651 [details] Bug 1391539 - Show tab separators when hovering tabs. https://reviewboard.mozilla.org/r/170024/#review192228 ::: browser/themes/shared/tabs.inc.css:610 (Diff revision 8) > +} > > /* Also show separators beside the selected tab when dragging it. */ > -#tabbrowser-tabs[movingtab] > .tabbrowser-tab[beforeselected]:not([last-visible-tab])::after, > -.tabbrowser-tab:not([selected]):not([afterselected-visible]):not([afterhovered]):not([first-visible-tab]):not(:hover)::before, > -#tabbrowser-tabs:not([overflow]) > .tabbrowser-tab[last-visible-tab]:not([selected]):not([beforehovered]):not(:hover)::after { > +#tabbrowser-tabs[movingtab] > .tabbrowser-tab[visuallyselected]::after, > +.tabbrowser-tab:not([first-visible-tab])::before, > +#tabbrowser-tabs:not([overflow]):not([movingtab]) > .tabbrowser-tab[last-visible-tab]::after { While you're touching this, can you change the selector order as follows? .tabbrowser-tab:not([first-visible-tab])::before, #tabbrowser-tabs:not([overflow]):not([movingtab]) > .tabbrowser-tab[last-visible-tab]::after, /* Also show separators beside the selected tab when dragging it. */ #tabbrowser-tabs[movingtab] > .tabbrowser-tab[visuallyselected]::after { ::: browser/themes/windows/browser.css:127 (Diff revision 8) > } > > /* Always show full-height tab separators on tabs with borders. */ > - .tabbrowser-tab:not(:-moz-lwtheme)::before { > + .tabbrowser-tab:not(:-moz-lwtheme)::before, > + .tabbrowser-tab:not(:-moz-lwtheme)::after { > border-image: none !important; Why is this okay here whereas tabs.inc.css uses border-image for full height separators? ::: browser/themes/windows/browser.css:151 (Diff revision 8) > + .tabbrowser-tab[last-visible-tab]:not(:-moz-lwtheme) { > + border-inline-end: 1px solid var(--tabs-border); > + } > + > + .tabbrowser-tab[last-visible-tab]:not(:-moz-lwtheme)::after { > + border-style: none !important; Just use display: none here

Comment on attachment 8898651 [details] Bug 1391539 - Show tab separators when hovering tabs. https://reviewboard.mozilla.org/r/170024/#review192228 > Why is this okay here whereas tabs.inc.css uses border-image for full height separators? I'm not sure why, but the spacing inside the tabs toolbar for ::before and ::after seems different on Windows 7 in general, e.g. it doesn't need a bottom margin for the overlapping navbar border for some reason. I'm not terribly happy about that either.

Comment on attachment 8898651 [details] Bug 1391539 - Show tab separators when hovering tabs. https://reviewboard.mozilla.org/r/170024/#review192304

Pushed by jhofmann@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/40ef1df3618b Show tab separators when hovering tabs. r=dao

I verified this issue using Windows 10 x64, Windows 7 x64, Mac OS X 10.12 with Nightly 58.0a1. I will mark this as verified fixed.