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

Status: VERIFIED FIXED

(Core :: Layout, defect, P1)

Description

[Emil Ivanov]

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

Comment 1

[Emil Ivanov]

Attachment: screenshot

Comment 2

[Emil Ivanov]

Attachment: 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.

Comment 3

[Mike Beltzner [:beltzner, not reading bugmail]]

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.

Comment 4

[Emil Ivanov]

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.

Comment 5

[David Baron :dbaron:]

Did this regress recently (in the past few weeks)?

Comment 6

[Emil Ivanov]

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

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Need more testcase reduction here. This could be a P1 blocker.

Comment 8

[Emil Ivanov]

Attachment: reduced testcase

Comment 9

[Martijn Wargers (dead)]

Attachment: 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.

Comment 10

[Martijn Wargers (dead)]

Indeed, this also regressed between 2007-07-25 and 2007-07-26, so a regression from bug 376124.

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: 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.

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix

This should fix it.

It seems to fix bug 421885 as well.

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 15

[David Baron :dbaron:]

(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?

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(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.

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 18

[David Baron :dbaron:]

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.

Comment 19

[David Baron :dbaron:]

The tests I was thinking of were the ones at http://fantasai.tripod.com/www-style/2002/table-backgrounds/tests/ .

Comment 20

[David Baron :dbaron:]

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.

Comment 21

[David Baron :dbaron:]

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.

Comment 22

[Mike Schroepfer]

(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.

Comment 23

[Mike Schroepfer]

Since this is also supposed to fix 421885 let's just get it in.

Comment 24

[David Baron :dbaron:]

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.

Comment 25

[philippe (part-time)]

This has cause d quite a serious regression with backgrounds on table-rows, see bug 424016.

Comment 26

[David Baron :dbaron:]

I backed this out due to causing bug 424016.

Comment 27

[Bernd]

> 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.

Comment 28

[David Baron :dbaron:]

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.

Comment 29

[David Baron :dbaron:]

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.

Comment 30

[David Baron :dbaron:]

Er, that regression was just a missing piece to my fix; I think it wasn't present in the previous patch.

Comment 31

[David Baron :dbaron:]

Attachment: patch

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.)

Comment 32

[Daniel Holbert [:dholbert]]

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

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

r=dholbert

Comment 33

[David Baron :dbaron:]

Comment on attachment 309383 [details] [diff] [review]
fix

Requesting approval1.9b5 to reland this patch that I landed and backed out yesterday.

Comment 34

[David Baron :dbaron:]

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.)

Comment 35

[Damon Sicore (:damons)]

Comment on attachment 309383 [details] [diff] [review]
fix

a1.9beta5+=damons

Comment 36

[Damon Sicore (:damons)]

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

a1.9beta5+=damons

Comment 37

[David Baron :dbaron:]

Relanded.

Comment 38

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Thanks! The mCellRect declaration could use a comment explaining what coordinate system it's in.

Comment 39

[Carsten Book [:Tomcat]]

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