Show full height tab separators when hovering tabs

VERIFIED FIXED in Firefox 58

Status

()

defect
P1
normal
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: johannh, Assigned: johannh)

Tracking

(Depends on 2 bugs, Blocks 3 bugs)

unspecified
Firefox 58
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify +

Firefox Tracking Flags

(firefox57 wontfix, firefox58 verified)

Details

(Whiteboard: [reserve-photon-visual])

Attachments

(2 attachments)

Comment hidden (mozreview-request)
Assignee

Comment 2

2 years ago
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 :)
Assignee

Comment 3

2 years ago
mozreview-review
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.
Assignee

Comment 4

2 years ago
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).
Blocks: photon-tabs
No longer blocks: photon-visual
Iteration: --- → 57.2 - Aug 29
QA Contact: ovidiu.boca
Iteration: 57.2 - Aug 29 → 57.3 - Sep 19
Comment hidden (mozreview-request)
Iteration: 57.3 - Sep 19 → ---
Comment hidden (mozreview-request)
Assignee

Comment 7

2 years ago
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 hidden (mozreview-request)
Assignee

Updated

2 years ago
Blocks: 1403483
Assignee

Updated

2 years ago
Blocks: 1403520
Assignee

Comment 9

2 years ago
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?
Flags: needinfo?(dao+bmo)
Assignee

Updated

2 years ago
Blocks: 1404899
Assignee

Updated

2 years ago
Attachment #8898651 - Flags: review?(dao+bmo) → review?(gijskruitbosch+bugs)
Assignee

Comment 10

2 years ago
Gijs, can you take a look please? Let me know if you have any questions. Sorry for pulling you in.
Comment hidden (mozreview-request)

Comment 12

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

Comment 13

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

2 years ago
Posted image seam-separator.png
On my win10 machine (175% dpi/whatever), I see seams between the hover background and the separators.

Comment 15

2 years ago
(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

2 years ago
mozreview-review
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.
Attachment #8898651 - Flags: review?(gijskruitbosch+bugs)
Flags: needinfo?(jhofmann)
Flags: needinfo?(dao+bmo)
Comment hidden (mozreview-request)
Assignee

Comment 18

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

2 years ago
mozreview-review
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.
Assignee

Comment 20

2 years ago
mozreview-review-reply
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

2 years ago
mozreview-review
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).
Attachment #8898651 - Flags: review?(gijskruitbosch+bugs)
Comment hidden (mozreview-request)

Comment 24

2 years ago
mozreview-review
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
Attachment #8898651 - Flags: review?(dao+bmo)
(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 hidden (mozreview-request)
Assignee

Comment 27

2 years ago
mozreview-review-reply
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

2 years ago
mozreview-review
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
Attachment #8898651 - Flags: review?(dao+bmo)
Assignee

Comment 29

2 years ago
mozreview-review-reply
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 hidden (mozreview-request)

Comment 31

2 years ago
mozreview-review
Comment on attachment 8898651 [details]
Bug 1391539 - Show tab separators when hovering tabs.

https://reviewboard.mozilla.org/r/170024/#review192304
Attachment #8898651 - Flags: review?(dao+bmo) → review+

Comment 32

2 years ago
Pushed by jhofmann@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/40ef1df3618b
Show tab separators when hovering tabs. r=dao
https://hg.mozilla.org/mozilla-central/rev/40ef1df3618b
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
Depends on: 1406673
Depends on: 1406735
Assignee

Updated

2 years ago
Depends on: 1406691

Updated

2 years ago
Depends on: 1406740

Updated

2 years ago
Depends on: 1406741

Updated

2 years ago
Depends on: 1406743
Depends on: 1406767
Assignee

Updated

2 years ago
Blocks: 1409341
No longer blocks: 1408953
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.
Status: RESOLVED → VERIFIED

Updated

2 years ago
Depends on: 1422478
Depends on: 1424992
You need to log in before you can comment on or make changes to this bug.