Closed Bug 369882 Opened 16 years ago Closed 15 years ago

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

Categories

(Firefox :: General, defect)

x86
Windows XP
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 3 alpha7

People

(Reporter: bert.massop, Assigned: dao)

References

Details

(Keywords: regression)

Attachments

(6 files, 3 obsolete files)

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
Attached image screenshot
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?
Status: UNCONFIRMED → NEW
Ever confirmed: true
ok, I see this in the latest trunk bits, but I also see everything in serious zoom.
Severity: trivial → normal
> 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
I wonder if this is really bug #350686, just exposed now due to bug #350686

as for my 300 dpi oddness, see bug #350686
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.
Summary: Strange line in lower tab corner → tabs sit 1px too low (strange line in lower corners)
(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.
I see this too in the latest nightly. WinXP SP2, no changes made into display settings.
Blocks: pixels
Flags: blocking-firefox3?
Version: unspecified → Trunk
workaround:

.tabbrowser-tab {
  margin-bottom: 1px !important;
}
Duplicate of this bug: 370320
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.
Assignee: nobody → sspitzer
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.
Status: NEW → ASSIGNED
there's probably a core bug that needs to be fixed, and a workaround for this particular case should only be the last resort.
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.
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
Assignee: sspitzer → nobody
Status: ASSIGNED → NEW
Component: Tabbed Browser → Layout: Block and Inline
Flags: blocking-firefox3?
Product: Firefox → Core
QA Contact: tabbed.browser → layout.block-and-inline
Summary: tabs sit 1px too low (strange line in lower corners) → vertically centered elements sit 1px too low (several manifestations in the Firefox UI, most notably the tabs)
Flags: blocking1.9?
dao: yes, you are right, this is core bug.  thanks for re-assigning it.
Duplicate of this bug: 371551
Duplicate of this bug: 375341
Duplicate of this bug: 377278
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.
Depends on: 353860
(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].)
Flags: blocking1.9? → blocking1.9+
Summary: vertically centered elements sit 1px too low (several manifestations in the Firefox UI, most notably the tabs) → vertically centered elements sit 1px too low (Firefox tabs, urlbar favicon)
Duplicate of this bug: 354046
So has anyone managed to create a minimal-ish testcase for this, by chance?
Attached file testcase (obsolete) —
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>.
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.
Assignee: nobody → general
Component: Layout: Block and Inline → GFX
QA Contact: layout.block-and-inline → ian
Attachment #254550 - Attachment description: Example → screenshot
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.
Actually, I take that back -- the background is correct, and for the image, we're somehow rounding the 0.5 up to 2.
The <deck> in the testcase seems to be key, though.  That implies a view (+ widget?).
Blocks: 380666
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...)
Assignee: general → dbaron
Status: NEW → ASSIGNED
Attachment #264806 - Flags: review?(vladimir)
And I get a REFTEST UNEXPECTED PASS for layout/reftests/pixel-rounding/offscreen-image-pos-5.html , which is bug 368280.
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.
Attachment #264806 - Flags: review?(vladimir) → review+
Blocks: 368280
Checked in to trunk.
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
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.)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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...)
(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.
(In reply to comment #34)
> This patch fixes the testcase, but I can still reproduce the original bug.

It did fix the favicon too.
Target Milestone: --- → mozilla1.9alpha6
Assignee: dbaron → general
Status: REOPENED → NEW
Attached file testcase 2 (obsolete) —
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.
(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.
(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.
Blocks: 370444
Duplicate of this bug: 381876
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.
Flags: in-testsuite?
Keywords: regression
Attachment #264943 - Attachment is obsolete: true
Attachment #264724 - Attachment is obsolete: true
Attached image 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?
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.
punting remaining a6 bugs to b1, all of these shipped in a5, so we're at least no worse off by doing so.
Target Milestone: mozilla1.9alpha6 → mozilla1.9beta1
Can someone illustrate a screenshot with the issue using a build from today?  I'm not quite sure what the issue is.
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).
Oh; I see... On my computer Fx 2.0 looks much closer to the screenshot you labeled "trunk" than the one you labeled "branch" :(
Duplicate of this bug: 386232
Blocks: 388076
(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.
Attachment #272943 - Flags: review?(vladimir)
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.
Attachment #272943 - Flags: review?(vladimir)
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 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.
Attachment #272943 - Flags: review?(vladimir)
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..)
Attachment #272943 - Flags: review?(vladimir) → review?(gavin.sharp)
Attachment #272943 - Flags: review?(gavin.sharp) → review+
Assignee: general → nobody
Component: GFX → General
Flags: in-testsuite?
Flags: in-testsuite-
Flags: blocking1.9+
Keywords: checkin-needed
Product: Core → Firefox
QA Contact: ian → general
Target Milestone: mozilla1.9beta1 → ---
Assignee: nobody → dao
Target Milestone: --- → Firefox 3 M7
Gavin, why in-testsuite-?  It seems worth it to me to add some testcases for this situation in general.
(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 :)
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
Status: NEW → RESOLVED
Closed: 16 years ago15 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
If I knew how to test, I would have attached a test...
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #278271 - Flags: review?(gavin.sharp)
Attachment #264806 - Attachment description: patch → patch (checked in)
Attachment #272943 - Attachment description: patch for winstripe tabs → patch for winstripe tabs (checked in)
Attachment #278271 - Attachment is obsolete: true
Attachment #280393 - Flags: review?(gavin.sharp)
Attachment #278271 - Flags: review?(gavin.sharp)
Attachment #280393 - Flags: review?(gavin.sharp) → review+
Keywords: checkin-needed
Attachment #280393 - Flags: approval1.9? → approval1.9+
Keywords: checkin-needed
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
Status: REOPENED → RESOLVED
Closed: 15 years ago15 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.