Closed Bug 421069 Opened 13 years ago Closed 13 years ago

specifying line-height in px or with decimal values causes rendering errors

Categories

(Core :: Layout, defect, P1)

x86
Windows XP
defect

Tracking

()

VERIFIED FIXED
mozilla1.9beta5

People

(Reporter: stream, Assigned: roc)

References

()

Details

(Keywords: regression, testcase)

Attachments

(7 files)

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

There is a problem with the footer i will attach image later.

This is happening only in fx3. Tested with firefox 2.0.0.12, opera 9.26, opera 9.5 beta build 9815, safari 3.1 beta, webkit r30768, ie 6, ie7.

Reproducible: Always
Attached image screenshot
Flags: blocking1.9?
Version: unspecified → Trunk
Keywords: regression
Attached file testcase
You can see the exact effect from from view > page style and select with or without line-height. The style with line-height is with red background.
This is kind of strange. I played with the testcase in Firebug, and noticed that the keyline (and I'm not sure why that's affected by the line-height parameter on the previous table element) that's rendering wrong does so when the value for line-height is specified in "px" units or with a decimal value.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: line-height in fx3 is different than fx2 (and all other browsers) → specifying line-height in px or with decimal values causes rendering errors
I made some more testing. When that link with line-height is only text, there is no problem. Combined text and image is a problem. It occurs first on 10px and every 2 after like: 10px 12px 14px 16px 18px 20px etc. Also tried to change the font from 11px to 10px and the first error occured on 9px then 11px etc...
Tried with font 1px and the first error was on 3px then 5px.
Did this regress recently (in the past few weeks)?
The earliest build which i found with this bug is Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a7pre) Gecko/2007072517 Minefield/3.0a7pre
Keywords: testcase
Need more testcase reduction here. This could be a P1 blocker.
Flags: blocking1.9? → blocking1.9+
Priority: -- → P1
Attached file reduced testcase
Assignee: nobody → roc
Attached file testcase3
Hey, thanks for reducing the testcase, I made another version of the reduced testcase, using data uri images.
On trunk, this testcase shows the image with a grayish line, where it should be black.
All in all, this reminds me a lot of bug 421885.
Indeed, this also regressed between 2007-07-25 and 2007-07-26, so a regression from bug 376124.
Blocks: 376124
Attached file testcase 4
Thanks Emil and Martijn ... I quite like this reduction of Emil's testcase. With data: URLs it's a bit hard to see what's going on sometimes. Unfortunately this one isn't as permanent as Martijn's.
So this is sort of what I feared in bug 376124. The table background is painted with a translation (non-integer-pixel) in the nsThebesRenderingContext, but the cell images are not. In this case it's not the nsThebesRenderingContext rounding that's hurting us, it's the rounding that layout does in nsLayoutUtils.

Basically I think we need to avoid doing any translation in nsThebesRenderingContext or at least ensure that (0,0) always maps to something pixel-aligned.
Attached patch fixSplinter Review
This should fix it.

It seems to fix bug 421885 as well.
Attachment #309383 - Flags: superreview?(dbaron)
Attachment #309383 - Flags: review?(dbaron)
I had a quick look at the other users of AutoPushTranslation. There are no other places where we push a translation and then go off to paint frame subtrees or images or other general drawing during regular operation. They're all used for special cases such as painting plugins, painting borders on framesets, painting a drag image, painting a widget background, printing, <canvas>, nsPresShell::RenderDocument, drawing image maps, or debug-only painting. We should probably get rid of them but they shouldn't cause us any major problems. Not sure about the image map drawing stuff, I don't even know what that does.
Whiteboard: [needs review]
(In reply to comment #12)
> cell images are not. In this case it's not the nsThebesRenderingContext
> rounding that's hurting us, it's the rounding that layout does in
> nsLayoutUtils.

What rounding (or pixel-snapping?) is hurting us, and why can't it consider the translation in the rendering context?
(In reply to comment #15)
> (In reply to comment #12)
> > cell images are not. In this case it's not the nsThebesRenderingContext
> > rounding that's hurting us, it's the rounding that layout does in
> > nsLayoutUtils.
> 
> What rounding (or pixel-snapping?) is hurting us,

The stuff in nsLayoutUtils::DrawImage.

> and why can't it consider the
> translation in the rendering context?

Because the rendering context just accumulates its translation into the gfxContext CTM, and that CTM might have other stuff in it like scaling or rotation.
I suppose we could check if the CTM HasNonTranslation and not do the nsLayoutUtils::DrawImage snapping at all in that case, and factor in the translation otherwise. But that seems more risky. Since translations are used very little in layout at the moment, it seems cleaner to eliminate them.
Comment on attachment 309383 [details] [diff] [review]
fix

OK, r+sr=dbaron.

I think fantasai has some test coverage for this code, though, that it would be good to get into reftest form before we land this, if possible.
Attachment #309383 - Flags: superreview?(dbaron)
Attachment #309383 - Flags: superreview+
Attachment #309383 - Flags: review?(dbaron)
Attachment #309383 - Flags: review+
Er, they're even already in our tree:
http://mxr.mozilla.org/seamonkey/source/layout/reftests/table-background/
...but they just don't have useful reference renderings.
I've been trying to write a reftest for this so I can land it, but I haven't been able to write a simple testcase showing the bug.  That said, I don't see the bug on any of the testcases in the bug report, either.
Target Milestone: --- → mozilla1.9beta5
(In reply to comment #21)
> I've been trying to write a reftest for this so I can land it, but I haven't
> been able to write a simple testcase showing the bug.  That said, I don't see
> the bug on any of the testcases in the bug report, either.
> 

Can anyone else verify the fix?  If so let's get it in.
Since this is also supposed to fix 421885 let's just get it in.
OK, checked in to trunk, although we really need to get reftests both for table border painting in general and for this bug in particular.
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Whiteboard: [needs review]
Depends on: 424016
This has cause d quite a serious regression with backgrounds on table-rows, see bug 424016.
I backed this out due to causing bug 424016.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
> Er, they're even already in our tree:

I checked them in when there wasn't a crash test option to cover at least crashes when the code is executed.
I have a working reftest for this bug now; I'm not going to take a pass at writing some tests for table background painting to narrow down which cases broke here.
I have a patch for the main regression (misplaced backgrounds on rows, row groups, columns, and column groups), but I still need to debug a regression (caught by the tests I just wrote) affecting table cell backgrounds in tables with border-collapse:collapse only.
Er, that regression was just a missing piece to my fix; I think it wasn't present in the previous patch.
Attached patch patchSplinter Review
Fix to the fix.

This applies on top of attachment 309383 [details] [diff] [review] (and some tests that I've written and will land shortly).

The fix is relatively straightforward.  I'm making mCellRect be translated by mRenderPt.  mCellRect was used in three ways.  Roc made two of those (touched by this patch) explicitly offset it by mRenderPt.  The third was passing it as the aBGClipArea to nsCSSRendering::PaintBackgroundWithSC.  If you look at the contents of PaintBackgroundWithSC, you'll see that aBGClipRect (what we're passing &mCellRect as) is expected to be in the same coordinate system as aBorderArea (for which we do pass a parameter translated by mRenderPt).  So it's reasonably obvious that that also needs to be translated for attachment 309383 [details] [diff] [review] to be internally consistent.

Since all three uses of mCellRect needed it to be translated, I made it translated when we set it and removed the explicit translations at the other two uses.  (My regression was that I'd missed the second of those two initially.)
Attachment #310877 - Flags: review?(dholbert)
Comment on attachment 310877 [details] [diff] [review]
patch

Makes sense to me, based on description in Comment #31.

r=dholbert
Attachment #310877 - Flags: review?(dholbert) → review+
Comment on attachment 309383 [details] [diff] [review]
fix

Requesting approval1.9b5 to reland this patch that I landed and backed out yesterday.
Attachment #309383 - Flags: approval1.9b5?
Comment on attachment 310877 [details] [diff] [review]
patch

Requesting approval1.9b5 for the fix for the regression for which I backed out the previous patch.

(I have tests ready to land that cover both this bug and the regression; they're not inside either of these patches though.)
Attachment #310877 - Flags: approval1.9b5?
Comment on attachment 309383 [details] [diff] [review]
fix

a1.9beta5+=damons
Attachment #309383 - Flags: approval1.9b5? → approval1.9b5+
Comment on attachment 310877 [details] [diff] [review]
patch

a1.9beta5+=damons
Attachment #310877 - Flags: approval1.9b5? → approval1.9b5+
Relanded.
Status: REOPENED → RESOLVED
Closed: 13 years ago13 years ago
Resolution: --- → FIXED
Thanks! The mCellRect declaration could use a comment explaining what coordinate system it's in.
Flags: in-testsuite+
verified fixed using the testcase and Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre) Gecko/2008041323 Minefield/3.0pre ID:2008041323
Status: RESOLVED → VERIFIED
Blocks: 523768
No longer depends on: 523768
You need to log in before you can comment on or make changes to this bug.