631698 - 1px border visible below active app tabs when there is tab overflow (using Aero Glass with DirectWrite disabled)

Status: RESOLVED FIXED

(Firefox :: Theme, defect)

Description

[:Margaret Leibovic]

Attachment: screenshot

It looks like the main toolbar shifts down 1px when I enter tab overflow mode, causing this 1px border to appear. However, it does not affect normal tabs.

Comment 1

[Dão Gottwald [:dao]]

I can only reproduce this with DirectWrite disabled.

Comment 2

[Frank Yan (:fryn)]

(In reply to comment #1)
> I can only reproduce this with DirectWrite disabled.

Is DirectWrite disabled by default? I can reproduce this on Windows 7 with a fresh profile. How does one toggle DirectWrite?

Comment 3

[Frank Yan (:fryn)]

(In reply to comment #2)
> (In reply to comment #1)
> > I can only reproduce this with DirectWrite disabled.
> 
> Is DirectWrite disabled by default? I can reproduce this on Windows 7 with a
> fresh profile. How does one toggle DirectWrite?

Oh nevermind. Ignore my entire comment. Sorry.

Comment 4

[Dão Gottwald [:dao]]

Attachment: patch

The current glass-specific tab bar styling relies on a DirectWrite-specific quirk that makes tabs 1px higher, so it fall apart without DirectWrite.

Fixing this turned out to be a larger undertaking, as I decided to move to one uniform implementation for glass, non-glass and Linux. This should get us more consistency and ease maintenance. The alternative would have been glass-specific changes, letting the three implementations depart further from each other, which would be particularly bad for glass vs. non-glass: the former builds on the latter, so it quickly becomes messy if there are fundamental differences.

Comment 5

[Dão Gottwald [:dao]]

Attachment: patch

the correct patch

Comment 6

[Dão Gottwald [:dao]]

Attachment: patch

reviewed my own patch and spotted a chance to simplify a selector

Comment 7

[Dão Gottwald [:dao]]

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

After thinking about this a bit more, it seems that this is unnecessarily complex and might be unnecessarily expensive (e.g. when tabs overflow) for Linux and non-glass Windows.

This is actually already a concern right now, as the scrollbox in the tab bar is positioned relatively and shifted down by a pixel. This shouldn't be needed. So instead I will shift focus towards simplifying the base styling (keeping it consistent across Windows and Linux) so that the more involved glass-specific stuff can easily build upon that.

Comment 8

[Dão Gottwald [:dao]]

(In reply to comment #7)
> So instead I will shift focus towards simplifying the base styling (keeping it
> consistent across Windows and Linux) so that the more involved glass-specific
> stuff can easily build upon that.

As a further bonus, it should be possible to use the simple styling in maximized windows even with glass, as the border radius in restored windows is really the only factor requiring more complexity.

Comment 9

[Dão Gottwald [:dao]]

Attachment: patch v2

This should be very close. Still need to verify my last few modifications on Windows, though.

Comment 10

[Dão Gottwald [:dao]]

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

Works and fixes rough edges across the board. Glass-less Windows and Linux for the most part are now in line with Aero Glass, which in return allows us to use the default styling for Aero Glass except in restored windows.

Comment 11

[Frank Yan (:fryn)]

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

This patch causes the tabs to have a different height with DirectWrite compared to without DirectWrite. While I realize that font rendering quirks are the cause, I don't think that that will be an acceptable change.

> +#main-window:not([disablechrome]) #navigator-toolbox[tabsontop=true] {

I see that you're using the more compact [name=value] format instead of [name="value"]. :) However, if we're going to start using that, we ought to use it consistently from now on, so the following line should follow that too:

+  #navigator-toolbox[tabsontop="false"] > #nav-bar:not(:-moz-lwtheme) {

After Firefox 4, we ought to do a global find and replace (at least within browser/themes/*) to do this for all attribute selectors with values containing only letters.

Overall, this does a good job of simplifying the styling and reducing platform-specific code; particularly, it makes the tab backgrounds easier to understand. :)

Comment 12

[Dão Gottwald [:dao]]

(In reply to comment #11)
> Comment on attachment 510151 [details] [diff] [review]
> patch v2
> 
> This patch causes the tabs to have a different height with DirectWrite compared
> to without DirectWrite. While I realize that font rendering quirks are the
> cause, I don't think that that will be an acceptable change.

It's expected, as the font is higher with DirectWrite (17px instead of 16px I think). Bug 617506 unintentionally "fixed" this and hence caused this bug.

Comment 13

[Frank Yan (:fryn)]

(In reply to comment #12)
> (In reply to comment #11)
> > Comment on attachment 510151 [details] [diff] [review]
> > patch v2
> > 
> > This patch causes the tabs to have a different height with DirectWrite compared
> > to without DirectWrite. While I realize that font rendering quirks are the
> > cause, I don't think that that will be an acceptable change.
> 
> It's expected, as the font is higher with DirectWrite (17px instead of 16px I
> think). Bug 617506 unintentionally "fixed" this and hence caused this bug.

I see. Would setting the line-height to a fixed value fix that issue?

Comment 14

[Frank Yan (:fryn)]

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

In that case, aside from the nit, this patch looks fine.

Comment 15

[Dão Gottwald [:dao]]

(In reply to comment #13)
> I see. Would setting the line-height to a fixed value fix that issue?

I looked into this some time ago, it didn't really work. We need to be careful not to break large-font / high-DPI settings, Asian fonts, etc.

Comment 16

[Frank Yan (:fryn)]

Pushed.

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

Comment 17

[Valerio]

Attachment: no border above content area

After the land of this, with tabs on bottom there is no border above the content area

Comment 18

[Valerio]

sorry, I was wrong. I had an old userstyle applyed

Comment 19

[u279076]

It appears that the fix for this bug has caused a regression: see bug 655294 comment 5