Last Comment Bug 476927 - Mac: make all area painted by text be part of text frame's ink overflow area (Reftest failure due to painting issue)
: Mac: make all area painted by text be part of text frame's ink overflow area ...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Layout: View Rendering (show other bugs)
: Trunk
: x86 Mac OS X
: -- normal (vote)
: ---
Assigned To: Jonathan Kew (:jfkthame)
:
Mentors:
Depends on: 475968 503718 542595
Blocks: ireflow 471630
  Show dependency treegraph
 
Reported: 2009-02-04 13:19 PST by Boris Zbarsky [:bz]
Modified: 2011-06-22 08:56 PDT (History)
17 users (show)
roc: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Testcase (332 bytes, text/html)
2009-02-04 16:50 PST, Boris Zbarsky [:bz]
no flags Details
Reference (105 bytes, text/html)
2009-02-04 16:51 PST, Boris Zbarsky [:bz]
no flags Details
test file demonstrating "pixel bleed" with Quartz antialiasing (1.04 KB, text/html)
2009-04-02 06:59 PDT, Jonathan Kew (:jfkthame)
no flags Details
enlarged screenshot of the test file, showing residual antialiasing artifacts (19.25 KB, image/png)
2009-04-02 07:01 PDT, Jonathan Kew (:jfkthame)
no flags Details
add "padding" to text bounds on OS X to allow for possible antialiasing pixels (10.21 KB, patch)
2009-04-02 13:59 PDT, Jonathan Kew (:jfkthame)
roc: review+
Details | Diff | Splinter Review
screenshot showing similar artifacts in Safari (16.84 KB, image/png)
2009-04-18 09:23 PDT, Jonathan Kew (:jfkthame)
no flags Details
test rect intersection instead of containment because of possible overflow effects (12.55 KB, patch)
2009-06-15 05:00 PDT, Jonathan Kew (:jfkthame)
no flags Details | Diff | Splinter Review
corrected previous patch for mochitest (2.34 KB, patch)
2009-06-15 06:42 PDT, Jonathan Kew (:jfkthame)
no flags Details | Diff | Splinter Review
avoid mochitest failure due to overflow by adding margin to text spans (1.88 KB, patch)
2009-06-15 16:31 PDT, Jonathan Kew (:jfkthame)
roc: review+
Details | Diff | Splinter Review
patch - add "padding" to text bounds on OS X to allow for possible antialiasing pixels (2.37 KB, patch)
2010-10-11 08:37 PDT, Jonathan Kew (:jfkthame)
no flags Details | Diff | Splinter Review

Description Boris Zbarsky [:bz] 2009-02-04 13:19:51 PST
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) (Exited; email my personal email if necessary) 2009-02-04 16:13:14 PST
Hmm, where did that testcase go? :-)
Comment 2 Boris Zbarsky [:bz] 2009-02-04 16:50:54 PST
Created attachment 360619 [details]
Testcase

Oops.  I thought I'd attached it.
Comment 3 Boris Zbarsky [:bz] 2009-02-04 16:51:32 PST
Created attachment 360620 [details]
Reference
Comment 4 Boris Zbarsky [:bz] 2009-03-30 09:05:29 PDT
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) (Exited; email my personal email if necessary) 2009-03-30 15:22:30 PDT
Jonathan K, would you mind taking this?
Comment 6 Jonathan Kew (:jfkthame) 2009-03-30 16:11:22 PDT
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) 2009-03-31 09:34:57 PDT
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) 2009-04-02 06:59:54 PDT
Created attachment 370638 [details]
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) 2009-04-02 07:01:04 PDT
Created attachment 370640 [details]
enlarged screenshot of the test file, showing residual antialiasing artifacts
Comment 10 Jonathan Kew (:jfkthame) 2009-04-02 13:59:10 PDT
Created attachment 370721 [details] [diff] [review]
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) 2009-04-07 10:40:00 PDT
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) 2009-04-18 09:23:04 PDT
Created attachment 373483 [details]
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] 2009-04-18 10:49:53 PDT
How about Safari 4 beta?

/be
Comment 14 Jonathan Kew (:jfkthame) 2009-04-24 16:07:40 PDT
Yes; tested with Safari version 4 Public Beta (6530.3), and I see similar artifacts to 3.2.1.
Comment 15 Karl Tomlinson (:karlt) 2009-06-14 23:53:03 PDT
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) 2009-06-15 05:00:32 PDT
Created attachment 383240 [details] [diff] [review]
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) 2009-06-15 06:42:29 PDT
Created attachment 383263 [details] [diff] [review]
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) (Exited; email my personal email if necessary) 2009-06-15 15:25:29 PDT
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) 2009-06-15 16:31:05 PDT
Created attachment 383372 [details] [diff] [review]
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) (Exited; email my personal email if necessary) 2009-07-10 00:47:46 PDT
http://hg.mozilla.org/mozilla-central/rev/78c2cb8171d5
http://hg.mozilla.org/mozilla-central/rev/705ef05105e8
Comment 21 Jonathan Kew (:jfkthame) 2009-07-15 04:19:07 PDT
Backed out due to bug 503718:
http://hg.mozilla.org/mozilla-central/rev/17a10b614f99
Comment 22 Jonathan Kew (:jfkthame) 2010-10-11 08:37:22 PDT
Created attachment 482247 [details] [diff] [review]
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) (Exited; email my personal email if necessary) 2010-10-11 13:39:52 PDT
We definitely want to do this, but I think not for FF4. It adds risk we don't need.
Comment 24 Alex 2011-04-11 06:09:28 PDT
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) 2011-04-11 06:26:29 PDT
(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) 2011-06-09 22:01:03 PDT
Is this the same bug? http://mcc.id.au/temp/glyph-hover.html
Comment 27 Jonathan Kew (:jfkthame) 2011-06-10 00:02:39 PDT
(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) 2011-06-22 01:57:39 PDT
Pushed to trunk:
http://hg.mozilla.org/mozilla-central/rev/b5dead378605

Note You need to log in before you can comment on or make changes to this bug.