1198236 - Pinned tab separators shouldn't touch the toolbar

Status: RESOLVED FIXED

(Firefox :: Theme, defect)

Description

[Dão Gottwald [:dao]]

Just noticed that bug 1189212 made pinned tab separators touch the toolbar. The margin-top seems to work as expected in that context but margin-bottom doesn't.

Comment 1

[:Gijs (he/him)]

Are there specific circumstances under which this happens? (windows-only? something else?) I tested on several platforms and missed this. :-\

Comment 2

[Dão Gottwald [:dao]]

Attachment: patch

Comment 3

[:Gijs (he/him)]

Comment on attachment 8652297 [details] [diff] [review]
patch

I still don't see this even without the patch, which means I can't really adequately review what you're doing here. Can you provide more details / STR for how to reproduce the issue?

Comment 4

[:Gijs (he/him)]

Comment on attachment 8652297 [details] [diff] [review]
patch

Review of attachment 8652297 [details] [diff] [review]:
-----------------------------------------------------------------

Ah, so I raced your explanation in #fx-team, which was that this shows up when the tabstrip is overflown.

That also means I know why: overflown tabs are position:fixed .

Changing the padding wfm. An alternative, if you prefer, would be changing the position / height in this case, but I don't know how easy it'd be to make that work with e.g. larger tab bars or whatever.

Comment 5

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/ad5c087d617b

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ad5c087d617b

Comment 7

[Dão Gottwald [:dao]]

Comment on attachment 8652297 [details] [diff] [review]
patch

Approval Request Comment
[Feature/regressing bug #]: bug 1189212 (pending approval)
[User impact if declined]: pinned tab separators are inconsistent with normal tab separators (taller than intended)
[Describe test coverage new/current, TreeHerder]:
[Risks and why]: low; very simple CSS patch
[String/UUID change made/needed]:

Comment 9

[Dão Gottwald [:dao]]

It looks like my patch is ineffective and this is still broken, albeit I'm sure I tested it at the time I wrote it...

Comment 10

[Dão Gottwald [:dao]]

Backed out: https://hg.mozilla.org/integration/fx-team/rev/e96b718a8ad2

Comment 11

[Dão Gottwald [:dao]]

Attachment: patch

Resorting to background-image. var() doesn't seem to work within linear-gradient, so I also had to get rid of --tab-separator-margin, but I've made it so that we don't need to repeat the logic for when to show the separator all over the place.

Comment 12

[:Gijs (he/him)]

Comment on attachment 8653361 [details] [diff] [review]
patch

Review of attachment 8653361 [details] [diff] [review]:
-----------------------------------------------------------------

Code looks fine to me, though I am confused, too, because I tested the backed-out patch, too, plus I don't understand why it wouldn't have worked...

Comment 13

[Dão Gottwald [:dao]]

(In reply to :Gijs Kruitbosch from comment #12)
> Comment on attachment 8653361 [details] [diff] [review]
> patch
> 
> Review of attachment 8653361 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Code looks fine to me, though I am confused, too, because I tested the
> backed-out patch, too, plus I don't understand why it wouldn't have worked...

I think it's some weird XUL layout bug, (randomly?) making the frame taller than it should be, such that it stretches beyond the tabs toolbar and behind the nav bar...

Comment 14

[Tim Nguyen :ntim]

(In reply to Dão Gottwald [:dao] from comment #11)
> Created attachment 8653361 [details] [diff] [review]
> patch
> 
> Resorting to background-image. var() doesn't seem to work within
> linear-gradient
Are you sure ? We've been using color variables with linear-gradients. https://dxr.mozilla.org/mozilla-central/source/browser/themes/shared/devtools/toolbars.inc.css#553
You may want to file a bug for your case.

Comment 15

[Tim Nguyen :ntim]

Comment on attachment 8653361 [details] [diff] [review]
patch

Review of attachment 8653361 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/themes/shared/tabs.inc.css
@@ -11,5 @@
>  }
> -#TabsToolbar {
> -  --tab-separator-opacity: 0.2;
> -  --tab-separator-margin: 4px;
> -  --tab-stroke-background-size: auto 100%;

This patch accidently removes tab stroke background size.

Comment 16

[Dão Gottwald [:dao]]

Attachment: patch v2

> This patch accidently removes tab stroke background size.

thanks, fixed

Comment 17

[Dão Gottwald [:dao]]

(In reply to Tim Nguyen [:ntim] from comment #14)
> (In reply to Dão Gottwald [:dao] from comment #11)
> > Created attachment 8653361 [details] [diff] [review]
> > patch
> > 
> > Resorting to background-image. var() doesn't seem to work within
> > linear-gradient
> Are you sure ? We've been using color variables with linear-gradients.
> https://dxr.mozilla.org/mozilla-central/source/browser/themes/shared/
> devtools/toolbars.inc.css#553
> You may want to file a bug for your case.

Maybe it's the combination of linear-gradient(), calc(100% - ...) and var(). Not sure what's going on, but I tested this quite a bit with a few variations.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Merge of backout:
https://hg.mozilla.org/mozilla-central/rev/e96b718a8ad2

Comment 19

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/cbb955b0495c

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/cbb955b0495c