[Harfbuzz] Fix incorrect font rendering when selecting texts in pages which specifies text-rendering: optimizeLegibility

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Joonghun Park]

Fix incorrect font rendering when selecting texts in pages which specifies text-rendering: optimizeLegibility CSS property and corresponding value.
For example, this issue can be reproducible in https://www.wordpress.com by selecting texts in there in which it specifies the css style mentioned.

[Joonghun Park]

Created attachment 259465 [details]
Patch

[Joonghun Park]

Created attachment 259466 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 259466 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=259466&action=review

Please upload a test as well.

> Source/WebCore/platform/graphics/cairo/FontCairoHarfbuzzNG.cpp:69
> +        glyphBuffer.reverse(0, glyphBuffer.size());

Why can't we do this behavior in previous condition ?

   if (run.rtl()) {
       float finalRoundingWidth = it.m_finalRoundingWidth;
       it.advance(run.length(), &localGlyphBuffer);
       initialAdvance = finalRoundingWidth + it.m_runWidthSoFar - afterWidth;
       glyphBuffer.reverse(0, glyphBuffer.size());
    } else

[Guilaume Ayoub]

I have this bug and find it very annoying too (and thank you for trying to solve it!), but unfortunately I'm not fully satisfied with this patch. It's removing kerning and ligatures everywhere, even with optimizeLegibility set and even when no text is selected. This makes optimizeLegibility almost useless and breaks pages when kerning is needed (for Arabic I suppose, but also for websites based on Material Design Lite as they need kerning for icons, see https://getmdl.io/components/index.html#buttons-section).

[Michael Catanzaro]

What is the incorrect rendering...?

[Guilaume Ayoub]

(In reply to comment #5)
> What is the incorrect rendering...?

When you select some text in a paragraph, the text of that's not selected in the paragraph gets the color of the selected text.

[Guilaume Ayoub]

Created attachment 301922 [details]
Screenshot of the problem

[Michael Catanzaro]

Ah yeah, this has been annoying me for a long time.

[Michael Catanzaro]

Hey Myles, could you review this patch please? We know it's not correct, but I'm curious to hear your comments on it. See comment #6 and comment #7 for description of the problem and screenshot.

[Myles C. Maxfield]

Comment on attachment 259466 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=259466&action=review

> Source/WebCore/platform/graphics/cairo/FontCairoHarfbuzzNG.cpp:48
> +    WidthIterator it(this, run, 0, false, forTextEmphasis);

WidthIterator only is used for the simple text codepath. It doesn't work with complex text (which is what this function is supposed to be for).

[Michael Catanzaro]

*** Bug 173809 has been marked as a duplicate of this bug. ***

[Carlos Garcia Campos]

Created attachment 320756 [details]
Patch

[Michael Catanzaro]

Comment on attachment 320756 [details]
Patch

Myles, do you want to review this?

Thanks Carlos. If you don't get a review from Myles soon, then ping me for the r+.

[Michael Catanzaro]

Comment on attachment 320756 [details]
Patch

I changed my mind, let's land this now. Myles can let us know if he sees something broken. :)

[Carlos Garcia Campos]

Committed r222086: <http://trac.webkit.org/changeset/222086>

[Radar WebKit Bug Importer]

<rdar://problem/34694193>