476927 - Mac: make all area painted by text be part of text frame's ink overflow area (Reftest failure due to painting issue)

Status: RESOLVED FIXED

(Core :: Web Painting, defect)

Description

[Boris Zbarsky [:bzbarsky]]

Test and reference coming up.  Over here, on Mac, I get a failure with the whole ',' being painted just a little differently (very subtle shifts in color).  The setTimeout in the onload handler is needed, unfortunately.  So is the addition of the <span> node in that function...

This was reduced from layout/reftests/first-letter/342120-1.xhtml which does NOT seem to trigger this bug on trunk, but does if we interrupt reflow in the right places.

Comment 1

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

Hmm, where did that testcase go? :-)

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Testcase

Oops.  I thought I'd attached it.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: Reference

Comment 4

[Boris Zbarsky [:bzbarsky]]

It'd still be awful nice to get some traction on this so bug 67752 doesn't introduce rando-orange when it lands...

Comment 5

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

Jonathan K, would you mind taking this?

Comment 6

[Jonathan Kew [:jfkthame]]

I'll take a look, but no guarantees! The differences are incredibly subtle - e.g., a fraction of a percent difference in one of the three color channels - but I'll dig around and see if I can pin down what's causing them.

Comment 7

[Jonathan Kew [:jfkthame]]

It seems the differences occur when the reftest is rendering to canvas, in order to capture and compare the images, but do *not* occur when the test and reference are rendered directly to a window (as tested by sampling actual pixels on my screen). So the behavior seems to depend which kind of surface Cairo is painting on.

Comment 8

[Jonathan Kew [:jfkthame]]

Attachment: test file demonstrating "pixel bleed" with Quartz antialiasing

The reftest failure occurs because when the onload handler fires, and the script modifies the document by inserting a new element, the display is refreshed. The textframe containing the "," is repainted with the clip set to its bounds; however, the antialiased rendering of the comma actually extends slightly beyond the bounds we are using. This results in double-painting of a few of the antialiasing pixels, and gives a slight color difference from the reference image.

(This is a Mac OS X/Quartz version of the Windows/ClearType problems we have already looked at, see bug 445087 and bug 475968.)

This test file, adapted from the testcase in 475968, demonstrates the discrepancy between the pixels Quartz actually touches and the bounding box to which we (sometimes) clip drawing operations. When the link is clicked, it "slides" the second line of text downwards, and a close examination will show that faint blue artifacts are left along the right-hand edge.

Note that this occurs whether or not we apply "text-rendering: optimizeLegibility;" to the document. There are other types of display issue that are helped by that option, but in this case the excess pixels are outside the bounding box that Quartz reports for the comma glyph, and so requesting "accurate" glyph bounds does not help us.

The fix needed will be similar to what we've had to do on Windows: add a pixel of "padding" to the reported glyph extents in certain cases.

Comment 9

[Jonathan Kew [:jfkthame]]

Attachment: enlarged screenshot of the test file, showing residual antialiasing artifacts

Comment 10

[Jonathan Kew [:jfkthame]]

Attachment: add "padding" to text bounds on OS X to allow for possible antialiasing pixels

This is similar to the Windows ClearType issues we have seen, though Quartz doesn't "spread" the pixels as aggressively when antialiasing and so the visual effects are less obvious. However, the test file (above) shows a faintly visible artifact of "antialiasing pixel bleed" to the right of the bounding box; bug 471630 shows a case where a similar effect on the left causes a reftest to fail.

The patch solves these issues by adding one pixel each side of the bounding box before returning from gfx{Atsui,CoreText}Font::Measure. (Another example of duplicated code between those two back-ends; a merge may be in order.) This is comparable to a recent change in gfxWindowsFont.

With this change, the test case here passes, and layout/reftests/bugs/387876-2.html also passes, so I have removed the RANDOM marker from that test.

On the other hand, the patch caused a couple of fresh reftest failures, in cases where the test now paints faint pixels outside of the nominal glyph bounds, and the reference was not expecting this. This leads to a subtle color difference on two pixels in bugs/387876-1 at the boundary between the 'a' and 'b' glyphs; their antialiasing pixels overlap slightly, and the result of painting separately is subtly different from the result of painting in a single operation. I think this is unavoidable, and not really a flaw; to deal with the test failure, I've changed it to use a sans-serif font, which avoids the overlap.

Similarly, the patch caused failures with bugs/363858-5 and -6, because the text "This..." is painted beginning exactly at the right edge of the available space (and therefore out of view). However, we now see a couple of faint antialiasing pixels from the "T" which extend to the left of its origin. Again, I don't believe this is actually a bug, it's a flawed assumption in the test. To resolve the failure, I've changed the text from "This..." to "Here..." as the "H" glyph does not exhibit this issue.

Pushed the patch to the tryserver to check whether the test changes have any unexpected effect on other platforms; awaiting results.

Comment 11

[Jonathan Kew [:jfkthame]]

Fixing this will cause overflow rects to be needed on lots of additional textframes (depending on fonts used, etc). Fortunately, bug 475968 is revising the storage of such rects, so the memory impact should be minimal.

Comment 12

[Jonathan Kew [:jfkthame]]

Attachment: screenshot showing similar artifacts in Safari

It's interesting to note that Webkit/Safari appears to suffer from a very similar problem; see screenshot of the testcase here as displayed by Safari 3.2.1. At "normal" zoom it gives extremely faint artifacts; this screenshot is after zooming out by one step, when the artifacts are much clearer.

Comment 13

[Brendan Eich [:brendan]]

How about Safari 4 beta?

/be

Comment 14

[Jonathan Kew [:jfkthame]]

Yes; tested with Safari version 4 Public Beta (6530.3), and I see similar artifacts to 3.2.1.

Comment 15

[Karl Tomlinson (:karlt)]

rob checked this in
http://hg.mozilla.org/mozilla-central/rev/58c89ce9719d

but it looked like it may have been causing

44188 ERROR TEST-UNEXPECTED-FAIL | /tests/layout/base/tests/test_bug450930.xhtml | scrolled-out invalidation should notify in subdoc

This may have been bug 483218, but it happened on the first two runs, which seems too coincidental given the frequency of occurrences of that bug, and we weren't able to be around to watch the tree any longer, sorry, so it got backed out:

http://hg.mozilla.org/mozilla-central/rev/be99fcdb6add

Comment 16

[Jonathan Kew [:jfkthame]]

Attachment: test rect intersection instead of containment because of possible overflow effects

So the failure in test_bug450930.xhtml seems to be caused by the possible addition of an "overflow" pixel to frame bounds; depending on exact content and fonts, this could have happened anyway but becomes much more likely with the patches here (on OS X) and in bug 475968 (Windows).

An example of tracing info from the test run shows:

   Incoming rect: (8,527.2000122070312,377,546.4000244140625)
   List rect 0: (7,527.2000122070312,377,546.4000244140625)
   NOT FOUND

(printed by the doesRectContainListElement() function). The expectation was that the "incoming" rect should have contained the list element, but because of the "padding" pixel on the LHS, the test fails.

Would it be reasonable to test that the rects intersect rather than testing for actual containment? This should still verify that the expected event is being sent, but will allow for the situation where the "boundingClientRect" of the subdoc is padded and does not precisely match the event's clientRect. The attached patch implements this, and seems to resolve the test failure on OS X; I expect it to also resolve the problem at bug 475968. With this patch applied, the corresponding test reports:

   Incoming rect: (8,527.2000122070312,377,546.4000244140625)
   List rect 0: (7,527.2000122070312,377,546.4000244140625) => FOUND

and all tests in test_bug450930.xhtml pass for me.

Comment 17

[Jonathan Kew [:jfkthame]]

Attachment: corrected previous patch for mochitest

Sorry, the previous attachment unintentionally merged the earlier code patch and this test fix. This version only modifies test_bug450930; if this is accepted, I believe the code change (as previously pushed and backed-out) can then be re-landed safely.

Comment 18

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

Testing for intersection instead of containment significantly weakens the test. I think a better approach would be to just remove all the text from the test DIVs, setting a height and width using inline style instead. Can you try that?

Comment 19

[Jonathan Kew [:jfkthame]]

Attachment: avoid mochitest failure due to overflow by adding margin to text spans

Rather than remove the text altogether (it's kind of nice to have it there when manually watching what the test is doing), we can wrap the words in <span>s with a few pixels of left margin. This ensures that potential antialiasing of the first glyph will not protrude beyond the original content rect and create an overflow area that disrupts the test.

Comment 20

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

http://hg.mozilla.org/mozilla-central/rev/78c2cb8171d5
http://hg.mozilla.org/mozilla-central/rev/705ef05105e8

Comment 21

[Jonathan Kew [:jfkthame]]

Backed out due to bug 503718:
http://hg.mozilla.org/mozilla-central/rev/17a10b614f99

Comment 22

[Jonathan Kew [:jfkthame]]

Attachment: patch - add "padding" to text bounds on OS X to allow for possible antialiasing pixels

Now that bug 542595 has landed, we should be able to fix this without the side-effects we saw previously (unwanted scrollbars due to the antialiasing pixel "overflow"). Updated patch to apply to current trunk.

Comment 23

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

We definitely want to do this, but I think not for FF4. It adds risk we don't need.

Comment 24

[Alex]

Is this going to make Firefox 5? The problem is still visible in the latest nightly (i.e. it hasn't been fixed by something else unrelated)

Comment 25

[Jonathan Kew [:jfkthame]]

(In reply to comment #24)
> Is this going to make Firefox 5? The problem is still visible in the latest
> nightly (i.e. it hasn't been fixed by something else unrelated)

With the unusually-short deadline for Fx5, I don't think we should try to do this now. Although it _should_ be possible to apply this without breaking other things, my feeling is that the issue is sufficiently minor, and the change sufficiently risky, that we should land it immediately _after_ the Fx5 merge to Aurora, and give it as much time as possible on nightlies before it moves towards a release version.

We should do the same thing with bug 475968, which is the Windows version of this issue.

Comment 26

[Cameron McCormack (:heycam)]

Is this the same bug? http://mcc.id.au/temp/glyph-hover.html

Comment 27

[Jonathan Kew [:jfkthame]]

(In reply to comment #26)
> Is this the same bug? http://mcc.id.au/temp/glyph-hover.html

No, that is because at small sizes we default to doing invalidation based on a "quick-and-dirty" path using glyph advances instead of the real glyph bounds, which means we don't take account of parts of the glyph that project beyond the typographic origin and advance. For a more extreme example, try
  <style>
  #a { font: 18px Zapfino; color: blue; background: #ccc; }
  #a:hover { color: red }
  </style>
  <p>Hover over the middle of the 'f': <span id=a>f</span>

As an author you can fix this by requesting "text-rendering:optimizeLegibility" in your style. The bug here is about glyph fringes that aren't included in the bounds even in that case.

Comment 28

[Jonathan Kew [:jfkthame]]

Pushed to trunk:
http://hg.mozilla.org/mozilla-central/rev/b5dead378605