Bug 56629 – REGRESSION(78846) [chromium] Justified text renders at incorrect offsets on windows

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[James Robinson]

REGRESSION(78846) [chromium] Justified text renders at incorrect offsets on windows

[James Robinson]

Created attachment 86145 [details]
Patch

[James Robinson]

Some sample text: http://software.hixie.ch/utilities/js/live-dom-viewer/saved/885

In chrome builds with webkit r78846 and newer this renders hilariously badly when text is selected.  With this patch applied the rendering is better (a smaller error is accumulated), but on some lines it is still possible to accumulate multiple pixels of error and end up with entire glyphs missing.  I can't figure out why chromium win is so much worse than safari win as there isn't any logic in FontCGWin's implementation of drawGlyphs that I can see that attempts to correctly allocate justification spacing around.

[James Robinson]

Upon investigation it looks like the drawGDIGlyphs() path in FontCGWin.cpp isn't hit in the common case, and I'm not able to hit it by messing about with my text preferences locally.  I'm not sure what is supposed to render down this path but I suspect that justified text is broken if that code is ever hit.

The common codepath in Safari win appears to use CGContextShowGlyphsWithAdvances() which takes a floating point advances and somehow internally maps this to integral advances to pass to GDI.  Presumably the mapping from float advances -> int advances within this codepath is smarter than just rounding each advance to an int.  At a guess I'd speculate that the logic looks like the logic that was removed from WidthIterator.cpp in r78846.

It seems like the way forward is to reimplement WidthIterator's accumulated floating point error logic in several platforms in order to snap to integer boundaries more accurately.  Hyatt, Mitz - do you have any advice?  This is not an area I'm familiar with so I could possibly be way off base.

[James Robinson]

Comment on attachment 86145 [details]
Patch

better patch incoming

[James Robinson]

Created attachment 86202 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 86202 [details]
Patch

The change looks fine.  BUt why no test updates?

[Stephen White]

This looks good to me too, although brettw should probably take a look.

[James Robinson]

(In reply to comment #6)
> (From update of attachment 86202 [details])
> The change looks fine.  BUt why no test updates?

We already have tests for the rendering of justified text.  They broke on chromium windows at r78846 and were incorrectly rebaselined (for example r78929 added a bad new pixel baseline for fast/text/basic/002.html - the text isn't justified correctly in the checked in -expected.png).  The problem is with a patch like r78846 that breaks hundreds of tests it's not very easy to manually check that each change is not a regression.  I don't think adding more tests will really help with this situation, sadly.

I will rebaseline the tests affected after landing and try to be careful about looking for more regressions.

[Brett Wilson (Google)]

This code looks like it will work to me.

[Dimitri Glazkov (Google)]

Comment on attachment 86202 [details]
Patch

<3

[James Robinson]

Looks like ~72 pixel test results will be affected - all progressions.  I'll update test_expectations.txt when landing to avoid churn on the bots.

[James Robinson]

Committed r81528: <http://trac.webkit.org/changeset/81528>

[James Robinson]

73 tests will need a rebaseline for chromium win.  I verified on my win7 box that all tests progress with this patch, but will pull pixel results off the bots to make sure I get the right thing for winxp.

It's kind of sad that we have many pixel tests for justified text and we still regress it pretty badly without the tests alerting us of the problem.

[James Robinson]

New chromium win pixel baselines landed in http://trac.webkit.org/changeset/81544.  Should be smooth sailing now.