857626 - Remove height property from Australis tab separators

Status: RESOLVED FIXED

(Firefox :: Theme, defect)

Description

[u428464]

In last UX build, the separators between each tab aren't showed anymore.

Comment 1

[Matthew N. [:MattN]]

I'm would guess this was caused by addressing bug 738491 comment 152.  They are still working on OS X though which is weird since that change affects both.

Comment 2

[Matthew N. [:MattN]]

(In reply to Dão Gottwald [:dao] from bug 738491 comment #154)
> Comment on attachment 726543 [details] [diff] [review]
> Part 4 v.2.1 Propagate changes in attachment 726527 [details] [diff] [review]
> 
> >+/* Background tab separators (1px wide).
> >+   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 {
> >+  -moz-margin-start: -1px;
> >+  background-image: linear-gradient(rgba(10,31,51,0), rgba(10,31,51,0.3));
> >+  background-position: left bottom;
> >+  background-repeat: no-repeat;
> >+  background-size: 1px 100%;
> >+  content: '';
> >+  display: -moz-box;
> >+  height: @tabHeight@;
> >+  width: 1px;
> >+}
> 
> You shouldn't really have to set a height here since you're using -moz-box.

Dao, do you know why removing the height caused separators to be invisible on Windows and Linux while they still work on OS X? The rule quoted is the only one for separators as far as I can remember and it is used on all platforms.

Comment 3

[Dão Gottwald [:dao]]

This appears to be caused by -moz-box-align:center in xul.css.
http://hg.mozilla.org/mozilla-central/annotate/55f9e3e3dae7/toolkit/content/xul.css#l647
You can fix this by setting -moz-box-align:stretch for tabbrowser tabs.

I don't know why OS X isn't affected. Is something already setting -moz-box-align:stretch for tabs on OS X?

Comment 4

[:Gijs (he/him)]

Taking at the request of MattN.

Comment 5

[:Gijs (he/him)]

(In reply to Dão Gottwald [:dao] from comment #3)
> This appears to be caused by -moz-box-align:center in xul.css.
> http://hg.mozilla.org/mozilla-central/annotate/55f9e3e3dae7/toolkit/content/
> xul.css#l647
> You can fix this by setting -moz-box-align:stretch for tabbrowser tabs.
> 
> I don't know why OS X isn't affected. Is something already setting
> -moz-box-align:stretch for tabs on OS X?

http://hg.mozilla.org/projects/ux/file/1db126465f33/browser/themes/osx/browser.css#l63

This is why OS X isn't affected, it seems.

Comment 6

[:Gijs (he/him)]

Actually, as the use of -moz-box here also causes bug 852420, why don't we just keep the height and leave out display: -moz-box (instead use display: inline-block?). Is that a terrible idea for some reason I'm not seeing?

Comment 7

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #5)
> (In reply to Dão Gottwald [:dao] from comment #3)
> > This appears to be caused by -moz-box-align:center in xul.css.
> > http://hg.mozilla.org/mozilla-central/annotate/55f9e3e3dae7/toolkit/content/
> > xul.css#l647
> > You can fix this by setting -moz-box-align:stretch for tabbrowser tabs.
> > 
> > I don't know why OS X isn't affected. Is something already setting
> > -moz-box-align:stretch for tabs on OS X?
> 
> http://hg.mozilla.org/projects/ux/file/1db126465f33/browser/themes/osx/
> browser.css#l63
> 
> This is why OS X isn't affected, it seems.

I was wrong. There is (also?):

http://hg.mozilla.org/projects/ux/file/1db126465f33/browser/themes/osx/browser.css#l2240

Comment 8

[Dão Gottwald [:dao]]

(In reply to :Gijs Kruitbosch from comment #6)
> Actually, as the use of -moz-box here also causes bug 852420, why don't we
> just keep the height and leave out display: -moz-box (instead use display:
> inline-block?).

Tabs can get taller when using larger fonts, in which case separators should grow vertically to match the tabs.

Comment 9

[:Gijs (he/him)]

Attachment: Patch to use inline-block and height instead of -moz-box

So, AFAICT, this works. Matt, can you (a) check and/or (b) tell me what used to break (bug 852420 comment #0 seems to imply something didn't work when doing things this way, but I don't see it...).

I'll put up an alternative patch in a second that removes the height and moves the -moz-box-align: stretch to tabs.inc, if that is preferred... but at the moment I don't see why it would be.

Comment 10

[:Gijs (he/him)]

Attachment: [checked in] Patch moving the -moz-box-align: stretch and removing the height: declaration

Comment 11

[Dão Gottwald [:dao]]

Comment on attachment 741299 [details] [diff] [review]
Patch to use inline-block and height instead of -moz-box

>-/* Because of -moz-box-align: center above, separators will be invisible unless
>-   we set their min-height. See bug 583510 for more information. */
>-toolbarseparator {
>-  min-height: 22px;
>-}

This has nothing to do with this bug.

Comment 12

[:Gijs (he/him)]

Attachment: Patch to use inline-block and height instead of -moz-box

Previous patch had a stray hunk, oops.

Comment 13

[:Gijs (he/him)]

Comment on attachment 741302 [details] [diff] [review]
Patch to use inline-block and height instead of -moz-box

per comment #8

Comment 14

[:Gijs (he/him)]

Attachment: Fix new tab button on Windows

This broke the new tab button's width on windows; the following patch should fix that.

Separately, I'm not sure why the new tab button's styling is so different on Windows vs. Mac. Purely talking about size/margins, I wouldn't have expected it to be such an issue. Might be worth to try and unify some of these styles in a separate bug?

Comment 15

[Dão Gottwald [:dao]]

Comment on attachment 741408 [details] [diff] [review]
Fix new tab button on Windows

How about Linux?

Comment 16

[:Gijs (he/him)]

(In reply to Dão Gottwald [:dao] from comment #15)
> Comment on attachment 741408 [details] [diff] [review]
> Fix new tab button on Windows
> 
> How about Linux?

Do you mean it is a problem and you don't understand why I didn't fix it, or that you're not sure if it is and you're asking whether or not I checked?

I'm not sure and I can't check because of the DNS failures on [tbpl|ftp].mozilla.org, and the lack of a local linux build env. If you know it to be a problem, I'm happy to add the same hunk to the linux version of browser.css, and I'd reiterate my point that we should probably have a followup bug to unify some of this stuff.

Comment 17

[:Gijs (he/him)]

Attachment: [checked in] Fix new tab button on Windows and Linux

So now that DNS is half-back, I managed to check that linux is also broken. I also manually unzipped omni.ja, fixed it, zipped it back up, and this works, AFAICT (and that makes total sense given the patch).

Comment 18

[Dão Gottwald [:dao]]

Comment on attachment 741468 [details] [diff] [review]
[checked in] Fix new tab button on Windows and Linux

Ok for now, but you should file a bug on not setting -moz-box-align:stretch for .tabs-newtab-button in the first place.

Comment 19

[:Gijs (he/him)]

https://hg.mozilla.org/mozilla-central/rev/d571b56010d6
https://hg.mozilla.org/mozilla-central/rev/09471e83215e