Closed Bug 857626 Opened 7 years ago Closed 6 years ago

Remove height property from Australis tab separators

Categories

(Firefox :: Theme, defect)

All
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 28

People

(Reporter: ge3k0s, Assigned: Gijs)

References

Details

(Whiteboard: [Australis:M3])

Attachments

(2 files, 3 obsolete files)

In last UX build, the separators between each tab aren't showed anymore.
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.
Assignee: nobody → mnoorenberghe+bmo
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
OS: All → Windows 7
(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.
Flags: needinfo?(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?
Flags: needinfo?(dao)
Whiteboard: [Australis:M3]
Status: ASSIGNED → NEW
Summary: (Australis tabs) Separators aren't visible anymore → Remove height property from Australis tab separators
Taking at the request of MattN.
Assignee: mnoorenberghe+bmo → gijskruitbosch+bugs
(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.
Status: NEW → ASSIGNED
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?
(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
(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.
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.
Attachment #741299 - Flags: feedback?(mnoorenberghe+bmo)
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.
Previous patch had a stray hunk, oops.
Attachment #741299 - Attachment is obsolete: true
Attachment #741299 - Flags: feedback?(mnoorenberghe+bmo)
Attachment #741302 - Flags: feedback?(mnoorenberghe+bmo)
Attachment #741301 - Flags: review+
Comment on attachment 741302 [details] [diff] [review]
Patch to use inline-block and height instead of -moz-box

per comment #8
Attachment #741302 - Attachment is obsolete: true
Attachment #741302 - Flags: feedback?(mnoorenberghe+bmo)
Attachment #741301 - Attachment description: Patch moving the -moz-box-align: stretch and removing the height: declaration → [checked in] Patch moving the -moz-box-align: stretch and removing the height: declaration
Attachment #741301 - Flags: feedback?(mnoorenberghe+bmo)
Whiteboard: [Australis:M3] → [Australis:M3][fixed-in-ux]
Attached patch Fix new tab button on Windows (obsolete) — Splinter Review
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?
Attachment #741408 - Flags: review?(dao)
Comment on attachment 741408 [details] [diff] [review]
Fix new tab button on Windows

How about Linux?
(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.
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).
Attachment #741408 - Attachment is obsolete: true
Attachment #741408 - Flags: review?(dao)
Attachment #741468 - Flags: review?(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.
Attachment #741468 - Flags: review?(dao) → review+
Attachment #741468 - Attachment description: Fix new tab button on Windows and Linux → [checked in] Fix new tab button on Windows and Linux
Blocks: 865487
https://hg.mozilla.org/mozilla-central/rev/d571b56010d6
https://hg.mozilla.org/mozilla-central/rev/09471e83215e
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:M3][fixed-in-ux] → [Australis:M3]
Target Milestone: --- → Firefox 28
You need to log in before you can comment on or make changes to this bug.