1391539 - Show full height tab separators when hovering tabs

Status: VERIFIED FIXED

(Firefox :: Theme, defect, P1)

Description

[Johann Hofmann [:johannh]]

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

Comment 1

[Johann Hofmann [:johannh]]

Attachment: Bug 1391539 - Show tab separators when hovering tabs.

Review commit: https://reviewboard.mozilla.org/r/170024/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/170024/

Comment 2

[Johann Hofmann [:johannh]]

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 3

[Johann Hofmann [:johannh]]

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.

Comment 4

[Johann Hofmann [:johannh]]

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).

Comment 5

[Johann Hofmann [:johannh]]

Comment on attachment 8898651 [details]
Bug 1391539 - Show tab separators when hovering tabs.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/170024/diff/1-2/

Comment 6

[Johann Hofmann [:johannh]]

Comment on attachment 8898651 [details]
Bug 1391539 - Show tab separators when hovering tabs.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/170024/diff/2-3/

Comment 7

[Johann Hofmann [:johannh]]

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.

Comment 8

[Johann Hofmann [:johannh]]

Comment on attachment 8898651 [details]
Bug 1391539 - Show tab separators when hovering tabs.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/170024/diff/3-4/

Comment 9

[Johann Hofmann [:johannh]]

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?

Comment 10

[Johann Hofmann [:johannh]]

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

Comment 11

[Johann Hofmann [:johannh]]

Comment on attachment 8898651 [details]
Bug 1391539 - Show tab separators when hovering tabs.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/170024/diff/4-5/

Comment 12

[:Gijs (out for now; he/him)]

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?

Comment 13

[:Gijs (out for now; he/him)]

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?

Comment 14

[:Gijs (out for now; he/him)]

Attachment: seam-separator.png

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

Comment 15

[:Gijs (out for now; he/him)]

(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 16

[:Gijs (out for now; he/him)]

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.

Comment 17

[Johann Hofmann [:johannh]]

Comment on attachment 8898651 [details]
Bug 1391539 - Show tab separators when hovering tabs.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/170024/diff/5-6/

Comment 18

[Johann Hofmann [:johannh]]

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 19

[Dão Gottwald [:dao]]

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 20

[Johann Hofmann [:johannh]]

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 21

[:Gijs (out for now; he/him)]

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 22

[Johann Hofmann [:johannh]]

Comment on attachment 8898651 [details]
Bug 1391539 - Show tab separators when hovering tabs.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/170024/diff/6-7/

Comment 23

[Johann Hofmann [:johannh]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=af9cf8deb131db85cb134d71b9bcdd8d8b518325

Comment 24

[Dão Gottwald [:dao]]

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

Comment 25

[Dão Gottwald [:dao]]

(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 26

[Johann Hofmann [:johannh]]

Comment on attachment 8898651 [details]
Bug 1391539 - Show tab separators when hovering tabs.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/170024/diff/7-8/

Comment 27

[Johann Hofmann [:johannh]]

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 28

[Dão Gottwald [:dao]]

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 29

[Johann Hofmann [:johannh]]

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 30

[Johann Hofmann [:johannh]]

Comment on attachment 8898651 [details]
Bug 1391539 - Show tab separators when hovering tabs.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/170024/diff/8-9/

Comment 31

[Dão Gottwald [:dao]]

Comment on attachment 8898651 [details]
Bug 1391539 - Show tab separators when hovering tabs.

https://reviewboard.mozilla.org/r/170024/#review192304

Comment 32

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/40ef1df3618b
Show tab separators when hovering tabs. r=dao

Comment 33

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/40ef1df3618b

Comment 34

[Valentina Claudia Ona]

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.