Closed Bug 1198236 Opened 5 years ago Closed 5 years ago

Pinned tab separators shouldn't touch the toolbar

Categories

(Firefox :: Theme, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 43
Tracking Status
firefox43 --- fixed

People

(Reporter: dao, Assigned: dao)

References

Details

(Keywords: regression)

Attachments

(1 file, 2 obsolete files)

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.
Are there specific circumstances under which this happens? (windows-only? something else?) I tested on several platforms and missed this. :-\
Attached patch patch (obsolete) — Splinter Review
Attachment #8652297 - Flags: review?(gijskruitbosch+bugs)
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?
Attachment #8652297 - Flags: review?(gijskruitbosch+bugs)
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.
Attachment #8652297 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/ad5c087d617b
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 43
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]:
Attachment #8652297 - Flags: approval-mozilla-aurora?
Duplicate of this bug: 1199124
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...
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #8652297 - Flags: approval-mozilla-aurora?
Attached patch patch (obsolete) — Splinter Review
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.
Attachment #8652297 - Attachment is obsolete: true
Attachment #8653361 - Flags: review?(gijskruitbosch+bugs)
Blocks: 1179756
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...
Attachment #8653361 - Flags: review?(gijskruitbosch+bugs) → review+
(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...
(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 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.
Attachment #8653361 - Flags: feedback-
Attached patch patch v2Splinter Review
> This patch accidently removes tab stroke background size.

thanks, fixed
Attachment #8653361 - Attachment is obsolete: true
Attachment #8653399 - Flags: superreview-
Attachment #8653399 - Flags: superreview-
(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.
Merge of backout:
https://hg.mozilla.org/mozilla-central/rev/e96b718a8ad2
Target Milestone: Firefox 43 → ---
https://hg.mozilla.org/mozilla-central/rev/cbb955b0495c
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 43
Depends on: 1317686
No longer depends on: 1317686
You need to log in before you can comment on or make changes to this bug.