597353 - Ensure pinned tabs have the same height and alignment as normal tabs

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[Dão Gottwald [:dao]]

vertical-align:middle is different from centered alignment in XUL, as the former depends on the font metrics which can have unequal padding at the top and the bottom. Enabling DirectWrite shows this problem or even just increasing the font size regardless of DirectWrite. vertical-align:middle on the tab stack is also wrong for the progress line which isn't supposed to be centered. So instead we need to make sure that the tab-content container has the same height for app tabs and normal tabs. Squeezing the label instead of completely removing it achieves this.

Comment 1

[Dão Gottwald [:dao]]

Attachment: patch

Comment 2

[:Gavin Sharp [email: [e-mail]]]

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

Changing the vertical align seems to actually make the favicon alignment worse...
http://grab.by/6r3i
http://grab.by/6r3x

Comment 3

[Dão Gottwald [:dao]]

(In reply to comment #2)
> Comment on attachment 476204 [details] [diff] [review]
> patch
> 
> Changing the vertical align seems to actually make the favicon alignment
> worse...
> http://grab.by/6r3i
> http://grab.by/6r3x

6r3i is with the patch and 6r3x without it?
tab-content has align="center", so this should take care of the alignment as long as the box height for app tabs is the same as for normal tabs. This worked for me on Windows and Linux, but maybe there's something Mac-specific that I'm missing.

Comment 4

[:Gavin Sharp [email: [e-mail]]]

Both screenshots have the .tab-label[pinned] hunk applied. 6r3i includes the change to "vertical-align: top" for .tab-stack, 6r3x does not.

Comment 5

[Dão Gottwald [:dao]]

Attachment: patch v2

this should fix pinstripe

Comment 6

[Dão Gottwald [:dao]]

This needs to be fixed sooner than right before RC, ideally for beta 8.

This is a central piece of CSS, I don't want to see more theme changes building on the misalignment.

Also, this will affect third-party themes.

Also, if for some reason this doesn't always work as expected, we'd want to find out about it as soon as possible.

Comment 7

[Johnathan Nightingale [:johnath]]

Good points. I final+'d it as polish, but you're right that themers could compound the problem here if they treat it as given. I agree that beta8 would be ideal, but I wouldn't hold a specific beta for it, so bumping to betaN

Comment 8

[Dão Gottwald [:dao]]

(In reply to comment #6)
> This is a central piece of CSS, I don't want to see more theme changes building
> on the misalignment.

Happening in bug 597592 right now :/

Comment 9

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 476496 [details] [diff] [review]
patch v2

Can you add a comment explaining where the 19px comes from?

Comment 10

[Dão Gottwald [:dao]]

Attachment: patch v2 (comment added)

I'd like to land this for beta7 in preparation for bug 597592, which is wanted for that beta.

Comment 11

[Dão Gottwald [:dao]]

Comment on attachment 479366 [details] [diff] [review]
patch v2 (comment added)

bug 597592 now blocks beta 7.

Comment 12

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/c510e150a244