369882 - vertically centered elements sit 1px too low (Firefox tabs, urlbar favicon)

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Bert Massop]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a3pre) Gecko/20070208 Minefield/3.0a3pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a3pre) Gecko/20070208 Minefield/3.0a3pre

When using multiple tabs, a strange line shows up in every lower corner of a tab (except for the tab on the foreground).

Reproducible: Always

Steps to Reproduce:
1. Just open two or more tabs
Actual Results:  
A strange line shows up in the lower corner of the tab.

Expected Results:  
This line shouldn't be there

Comment 1

[Bert Massop]

Attachment: screenshot

Comment 2

[(not reading, please use [e-mail] instead)]

I'll check the latest bits, but in my Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a2pre) Gecko/20070204 Minefield/3.0a2pre build I am not seeing it.

but, this looks like something we saw when working on the tab strip / theme refress back for fx 2.0

bert, anything special about your display or dpi settings?

Comment 3

[(not reading, please use [e-mail] instead)]

ok, I see this in the latest trunk bits, but I also see everything in serious zoom.

Comment 4

[(not reading, please use [e-mail] instead)]

> ok, I see this in the latest trunk bits

ok, now i see it.  I think might be fall out related to #177805.

> but I also see everything in serious zoom.

that was a mix of my layout.css.dpi being set to 300 and the landing of bug #177805.

as for my why dpi was set so high, must have been from when working on the fx theme for 2.0 for large dpi issues.

thanks to gavin for pointing me at bug #177805

Comment 5

[(not reading, please use [e-mail] instead)]

I wonder if this is really bug #350686, just exposed now due to bug #350686

as for my 300 dpi oddness, see bug #350686

Comment 6

[Dão Gottwald [:dao]]

That could be the same effect as seen in bug 369864, just vertically; although I don't yet understand how the vertical alignment for the tabs is defined.

Comment 8

[Bert Massop]

(In reply to comment #2)
> bert, anything special about your display or dpi settings?

I don't think so. I didn't change any dpi setting, and I'm running on a quite normal 1280x1024 resolution.

Maybe this problem doesn't show up on the next release, it might even be a strange coincidence, we'll see.

Comment 9

[Ere Maijala (slow)]

I see this too in the latest nightly. WinXP SP2, no changes made into display settings.

Comment 10

[Dão Gottwald [:dao]]

workaround:

.tabbrowser-tab {
  margin-bottom: 1px !important;
}

Comment 12

[(not reading, please use [e-mail] instead)]

on a related note, try this:

toggle between two blank tabs and the favicon for a blank document will jump by 1 px.

looking into dao's workaround.

Comment 13

[(not reading, please use [e-mail] instead)]

dao's work around fixes this bug, and my related issue in comment #12 and achieves parity with fx 2.0.

I'll work it into our theme css and attach for review.

Comment 14

[Dão Gottwald [:dao]]

there's probably a core bug that needs to be fixed, and a workaround for this particular case should only be the last resort.

Comment 15

[Bert Massop]

I agree with Dão.

As far as I can see; the tab "content" (favicon and title) is too small. If you select a tab, the "content" moves down 1px, and the line is hidden under the content.

Comment 16

[Dão Gottwald [:dao]]

Seth, I hope you agree on that:
Adding hacks for all manifestations of the same bug isn't a solution. The location bar, its favicon, and the search bar aren't centered either (1px too low); and that are only those that are obvious to me.
-> updating the summary and moving to Core

Comment 17

[(not reading, please use [e-mail] instead)]

dao: yes, you are right, this is core bug.  thanks for re-assigning it.

Comment 21

[David Baron :dbaron:]

Are all the elements where this is showing up elements with rounded corners?  (That is, some -moz-border-radius-* nonzero?)  This seems pretty similar / the same as bug 353860.

Comment 22

[Dão Gottwald [:dao]]

(In reply to comment #21)
> Are all the elements where this is showing up elements with rounded corners? 
> (That is, some -moz-border-radius-* nonzero?)

To my knowledge, no. For example, the favicon doesn't seem to be centered within the urlbar. (Contrary to comment 16, the urlbar itself seems to be centered, although it looks misaligned due to an odd line (not a border) at the bottom of the toolbar; see attachment 258373 [details].)

Comment 24

[Boris Zbarsky [:bzbarsky]]

So has anyone managed to create a minimal-ish testcase for this, by chance?

Comment 25

[Dão Gottwald [:dao]]

Attachment: testcase

The summary isn't quite accurate: the image element is centered, but the raw image is misaligned. Also, I can't reproduce this without <deck>.

Comment 26

[Boris Zbarsky [:bzbarsky]]

So the heart of the matter for that testcase seems to be that when we paint images we snap the rect to pixel boundaries (rounding to the nearest pixel) before calling down into thebes -- the rounding happens in nsLayoutUtils::DrawImage.  Then we're a single-pixel image, so nsThebesImage::Draw does a Rectangle() at (0,1), which are the rounded coordinates) of the given size.

On the other hand, nsCSSRendering::PaintBackgroundColor just does a FillRect on the fractional-pixel rect.  nsThebesRenderingContext::FillRect calls Rectangle() on a rectangle at (0,0.5), with snapToPixels set to true.  This uses gfxContext::UserToDevicePixelSnapped, which also seems to round to nearest pixel... but the numbers it tries to round are the output of UserToDevice.  And the cairo transform has a -0.5px vertical translation.  So UserToDevice() hands back the top-left vertex as being at (0,0).  Rounded, this gives (0,0).

If you recall, the image case was doing Rectangle() at (0,1).  Which gives (0,0.5) after calling UserToDevice, then gets rounded to (0,1).

So you see a 1px offset.

I'm not sure whether the right solution is to stop rounding in nsLayoutUtils::DrawImage (since the numbers we're rounding there have nothing to do with actual device pixel offsets), to continue rounding there but take into account the rendering context transform, or something else.  I do find it odd that we end up with the fractional-pixel background fill somehow at (0,0) before rounding; I wonder what coordinate system that thinks it's in.

Comment 27

[David Baron :dbaron:]

Based on the rules I test in layout/reftests/pixel-rounding/, the background is wrong (we should round edges at half-pixels to right or bottom); not sure why none of the tests there are failing, though.

Comment 28

[David Baron :dbaron:]

Actually, I take that back -- the background is correct, and for the image, we're somehow rounding the 0.5 up to 2.

Comment 29

[David Baron :dbaron:]

The <deck> in the testcase seems to be key, though.  That implies a view (+ widget?).

Comment 30

[David Baron :dbaron:]

Attachment: patch (checked in)

This fixes it for me.

If I didn't remove the pixel snapping in nsThebesImage::Draw, I get blurry images when they're half-pixel aligned.  I don't think that's needed anymore for the layout callers going through nsLayoutUtils::DrawImage; not sure if there are others we need to care about.

(But, really, it ought to be able to pixel-snap correctly.  Maybe I should figure out how to fix it instead...)

Comment 31

[David Baron :dbaron:]

And I get a REFTEST UNEXPECTED PASS for layout/reftests/pixel-rounding/offscreen-image-pos-5.html , which is bug 368280.

Comment 32

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 264806 [details] [diff] [review]
patch (checked in)

I think that's the main caller, so as long as all works fine there, this should be ok.

Comment 33

[David Baron :dbaron:]

Checked in to trunk.

Comment 34

[Eli Friedman]

This patch fixes the testcase, but I can still reproduce the original bug.  I'm working on a better fix.  (I think reftests can't catch this bug because the way they paint bypasses the standard paint code.)

Comment 35

[Eli Friedman]

Never mind about having a fix for this... the problem here is the tabs, and that's likely a border-radius bug.  (I think I have a fix for Bug 380438, though...)

Comment 36

[Dão Gottwald [:dao]]

(In reply to comment #35)
> the problem here is the tabs, and that's likely a border-radius bug.

-moz-border-radius is specified for .tab-image-left and tab-image-right but not for .tab-image-middle and .tab-close-button or .tabbrowser-tab.
Resetting these radii via DOMi makes the horizontal lines disappear, but the tab remains misaligned.

Comment 37

[Dão Gottwald [:dao]]

(In reply to comment #34)
> This patch fixes the testcase, but I can still reproduce the original bug.

It did fix the favicon too.

Comment 38

[Dão Gottwald [:dao]]

Attachment: testcase 2

I'm not sure if this is actually causing the tabs bug. At least, I got to this testcase by reducing a <tabbox> with all its anonymous content and a misaligned tab.

Comment 39

[Dão Gottwald [:dao]]

(In reply to comment #36)
> (In reply to comment #35)
> > the problem here is the tabs, and that's likely a border-radius bug.
> 
> -moz-border-radius is specified for .tab-image-left and tab-image-right but not
> for .tab-image-middle and .tab-close-button or .tabbrowser-tab.

That was wrong for .tabbrowser-tab, which does have a border-radius (originating from a |tab| rule), but removing it doesn't seem to make any difference.

Comment 40

[Dão Gottwald [:dao]]

(In reply to comment #38)
> Created an attachment (id=264943) [details]
> testcase 2
> 
> I'm not sure if this is actually causing the tabs bug.

I am now, so let me clarify the previous comments. Comment 36 is the crux -- there are two remaining bugs:

  1) the horizontal lines in .tab-image-left and .tab-image-right, caused by
     border-radius, and
  2) the vertical misalignment (see the leftmost circle in attachment 254550 [details]).

Testcase 2 represents 1), which is likely the same underlying problem as bug 353860. I suggest to focus on 2) here.

Comment 42

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

So both testcases now work fine, but the overall 1px too low problem still isn't fixed.  I'm not really sure what's going on; now the tab background area is still 1px too low (the close box looks like it's in the right spot; that is, it looks like it's 1px "too high").  But there is no more bogus horizontal line/blurryness that was due to borders and border radius.

Comment 43

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: screenshot with floor()

So I may need Eli's help here -- the problem is that for the middle part of the tabs, we get into nsThebesRenderingContext::DrawTile with a Y coordinate of 1590 twips, which is 1590/60 = 26.5.  We then end up drawing at 26.5, which causes the problem.  If we round, we'd draw at 27, which is also visually wrong. 

If I add a floor() to both the x/y offset (phase) and the target rect, I get the attached screenshot.  The middle portions look visually correct, the left/right sides are still wrong, presumably because they're not going through DrawTile.

Where does this error lie?  Is it an error that we're passing non-pixel-aligned data down to nsThebesRenderingContext?

Comment 44

[Eli Friedman]

Hmm, actually, the tabs seem to look fine in my tree.  Let me get my view fixes into the tree, and then I'll get back to you.

Comment 45

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

k, I'll wait until you do that

Comment 46

[Mike Connor [:mconnor]]

punting remaining a6 bugs to b1, all of these shipped in a5, so we're at least no worse off by doing so.

Comment 47

[Eli Friedman]

Can someone illustrate a screenshot with the issue using a build from today?  I'm not quite sure what the issue is.

Comment 48

[Adam Guthrie]

Attachment: screenshot comparing tabstrip on trunk and branch

You can see the problem if you look at the bottom of the tabstrip. The entire 1px "stripe" is missing from the button on trunk (since the entire tabstrip is lowered by 1px).

Comment 49

[Eli Friedman]

Oh; I see... On my computer Fx 2.0 looks much closer to the screenshot you labeled "trunk" than the one you labeled "branch" :(

Comment 51

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(In reply to comment #10)
> workaround:
> 
> .tabbrowser-tab {
>   margin-bottom: 1px !important;
> }

After talking with Eli, we don't think this is a bug -- our rounding is pretty consistent right now with everything other than native theme code (and that's mostly getting fixed).  The problem seems to have been that Fx2 just happened to round the other way for this situation; he says that he can get Fx2 to look like the current trunk when he runs with a higher dpi setting, which causes differences in rounding on Fx2.

So, this needs to be fixed in the theme -- either with the above workaround, or with some better analysis about how those pieces work together.  Better yet, the tabs should be redone so that they use the new border-image support being done in bug 378217, because that's bound to be faster than the way we're building the tabs up now.

Comment 52

[Dão Gottwald [:dao]]

Attachment: patch for winstripe tabs (checked in)

Comment 53

[Ryan Flint [:rflint] (ping via IRC for reviews)]

Comment on attachment 272943 [details] [diff] [review]
patch for winstripe tabs (checked in)

Per discussion with Vlad over IRC, we're going to hold off on this in favor of the more structural changes to the tabstrip coming with bugs 387345 and 339964.

Comment 54

[Dão Gottwald [:dao]]

There's no reason why this regression shouldn't be fixed before any other work starts. The current stylesheet is broken -- it contains margin-bottom: 1px, which hasn't any effect because of margin: 0px !important.

Comment 55

[Dão Gottwald [:dao]]

Comment on attachment 272943 [details] [diff] [review]
patch for winstripe tabs (checked in)

Since this has also caused bug 388076, I'm re-requesting review. Again, I think this should be fixed as soon as possible so that a refactoring of the tab strip has a working starting point.

Comment 56

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

I'm not the right person to approve this -- I'd try either mconnor or maybe gavin?  (Sorry for the runaround, I just don't touch any fo the theme stuff..)

Comment 57

[Boris Zbarsky [:bzbarsky]]

Gavin, why in-testsuite-?  It seems worth it to me to add some testcases for this situation in general.

Comment 58

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

(In reply to comment #57)
> Gavin, why in-testsuite-?  It seems worth it to me to add some testcases for
> this situation in general.

What situation in general? Testing that our rounding behavior doesn't change? That seems like a test that would belong in the bug that changed our rounding, rather than the bug about fixing the theme to compensate, but I'm not too picky (and realize that "that bug" might have been "switch to cairo" or "Eli's unit patch" or some other large change that's hard to isolate). Either way, I don't really know how you'd test this; feel free to reset to "?" if you have ideas :)

Comment 59

[Michael Wu [:mwu]]

Checking in toolkit/themes/winstripe/global/browser.css;
/cvsroot/mozilla/toolkit/themes/winstripe/global/browser.css,v  <--  browser.css
new revision: 1.31; previous revision: 1.30
done

Comment 60

[Boris Zbarsky [:bzbarsky]]

If I knew how to test, I would have attached a test...

Comment 61

[Dão Gottwald [:dao]]

Attachment: patch for tabstrip close button (browser.tabs. closeButtons = 3)

Comment 62

[Dão Gottwald [:dao]]

Attachment: better patch for tabstrip close button (browser.tabs. closeButtons = 3)

Comment 63

[Reed Loden [:reed]]

Checking in browser/themes/winstripe/browser/browser.css;
/cvsroot/mozilla/browser/themes/winstripe/browser/browser.css,v  <--  browser.css
new revision: 1.96; previous revision: 1.95
done