372072 - Broken reftests on Mac involving text

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

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

REFTEST UNEXPECTED FAIL: file:///Users/roc/mozilla-checkin/mozilla/layout/reftests/bugs/9458-zorder-3.html
REFTEST UNEXPECTED FAIL: file:///Users/roc/mozilla-checkin/mozilla/layout/reftests/bugs/9458-zorder-4.html
REFTEST UNEXPECTED FAIL: file:///Users/roc/mozilla-checkin/mozilla/layout/reftests/bugs/9458-zorder-5.html

REFTEST UNEXPECTED FAIL: file:///Users/roc/mozilla-checkin/mozilla/layout/reftests/bugs/18217-zorder-3.html
REFTEST UNEXPECTED FAIL: file:///Users/roc/mozilla-checkin/mozilla/layout/reftests/bugs/18217-zorder-4.html
REFTEST UNEXPECTED FAIL: file:///Users/roc/mozilla-checkin/mozilla/layout/reftests/bugs/18217-zorder-5.html

Plus another one for table-width/balancing-2.html.

The problem for the first 6 failures seems to be that the "x" is visible outside the green box *and* the reference has just one x while the tests have 2 x's stacked, so the antialiased edge is slightly darker in the tests than in the reference.

I'm not sure what the balancing-2 failure is caused by. It's very hard to see but adjacent to the left edge of the topmost yellow box, there's one very slightly yellow pixel, I think the end of an "x".

Comment 1

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

Attachment: attempted fix

I tried to fix the failures with these changes to the reference renderings, but it didn't work. Not sure why yet.

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

In the z-ordering tests, the x underneath may in many cases be red rather than green.

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

And also, in your reference changes for the z-ordering tests, you might be drawing two x's on top of two backgrounds, rather than x/background/x/background.  Although depending on whether the backgrounds match that may not matter.

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The best solution for the z-index tests might be to stick   before and after all the Xs.

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

And I think for both balancing-1 and balancing-2 the thing to do is to replace all the x's with  s.  They're only really there to make cells non-empty, and they are sometimes positioned at different places.

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(really,  's in the reference where they're really needed, and with nothing when they're inside inline-blocks)

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: possible patch

Here's what I was thinking.

I'm testing this, and I plan to check it in if it doesn't break anything.

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I checked in the above patch.  Let me know if it helps.

Comment 9

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

Yeah, that makes reftests work on Mac trunk. Thanks.

But now my textrun changes are causing reftest failures, e.g., with layout/reftests/bugs/351641-1b.html. I think the problem is that with kerning, and without the new textframe, for some fonts the text "bcdefghij" is not the same width as "bcd<span>efg</span>hij". I think something similar is happening with layout/reftests/first-letter/quote-1c.html.

So what should we do? nsTextFrameThebes should fix this problem completely since it builds textruns that span text nodes. But what should we do in the meantime? I suppose I could try disabling kerning on all platforms, is it worth it?

Comment 10

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

Actually it's not kerning --- that's already disabled --- but I think subpixel text alignment. Isn't there already a bug on this?

In the failing tests, text rendered as a single string gets pixels aligned with high subpixel precision. When we render substrings separately, the substring starts are aligned to, at best, appunits. The difference in alignment causes subtle antialiasing changes. This will actually still be a problem for nsTextFrameThebes. I guess I'll have to change the two tests that are failing for me now.

Comment 11

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

I *suppose* I could round glyph advances to appunits in the textruns to avoid this problem.

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

If it's not a huge number of tests and you're planning to fix them later, it's fine to mark them conditionally failing or random, i.e., fails-if(MOZ_WIDGET_TOOLKIT=="cocoa") or random-if(...).  Just make sure there's a bug number describing the problem and put that bug number in a comment at the end of the line.

Comment 13

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

I've actually fixed this in the textrun code. I have the Mac textrun code rounding glyph advances up to the next appunit, and I turned off some round-to-pixel code in gfxTextRun::Draw, and everything's good.