Bug 148220 - [Harfbuzz] Fix incorrect font rendering when selecting texts in pages which specifies text-rendering: optimizeLegibility
Summary: [Harfbuzz] Fix incorrect font rendering when selecting texts in pages which s...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Joonghun Park
URL:
Keywords: InRadar
: 173809 (view as bug list)
Depends on:
Blocks:
 
Reported: 2015-08-20 06:38 PDT by Joonghun Park
Modified: 2017-09-27 12:52 PDT (History)
9 users (show)

See Also:


Attachments
Patch (2.79 KB, patch)
2015-08-20 06:43 PDT, Joonghun Park
no flags Details | Formatted Diff | Diff
Patch (2.80 KB, patch)
2015-08-20 06:46 PDT, Joonghun Park
mmaxfield: review-
Details | Formatted Diff | Diff
Screenshot of the problem (21.53 KB, image/png)
2017-02-17 06:41 PST, Guilaume Ayoub
no flags Details
Patch (8.58 KB, patch)
2017-09-14 03:48 PDT, Carlos Garcia Campos
mcatanzaro: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joonghun Park 2015-08-20 06:38:08 PDT
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.
Comment 1 Joonghun Park 2015-08-20 06:43:01 PDT
Created attachment 259465 [details]
Patch
Comment 2 Joonghun Park 2015-08-20 06:46:14 PDT
Created attachment 259466 [details]
Patch
Comment 3 Gyuyoung Kim 2016-04-12 00:15:06 PDT
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
Comment 4 Guilaume Ayoub 2016-05-09 08:18:18 PDT
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).
Comment 5 Michael Catanzaro 2017-02-17 06:30:09 PST
What is the incorrect rendering...?
Comment 6 Guilaume Ayoub 2017-02-17 06:40:42 PST
(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.
Comment 7 Guilaume Ayoub 2017-02-17 06:41:15 PST
Created attachment 301922 [details]
Screenshot of the problem
Comment 8 Michael Catanzaro 2017-02-17 08:01:32 PST
Ah yeah, this has been annoying me for a long time.
Comment 9 Michael Catanzaro 2017-02-18 08:11:42 PST
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.
Comment 10 Myles C. Maxfield 2017-02-27 14:03:18 PST
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).
Comment 11 Michael Catanzaro 2017-06-24 05:32:46 PDT
*** Bug 173809 has been marked as a duplicate of this bug. ***
Comment 12 Carlos Garcia Campos 2017-09-14 03:48:09 PDT
Created attachment 320756 [details]
Patch
Comment 13 Michael Catanzaro 2017-09-14 08:20:08 PDT
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+.
Comment 14 Michael Catanzaro 2017-09-15 06:38:48 PDT
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. :)
Comment 15 Carlos Garcia Campos 2017-09-15 06:42:43 PDT
Committed r222086: <http://trac.webkit.org/changeset/222086>
Comment 16 Radar WebKit Bug Importer 2017-09-27 12:52:38 PDT
<rdar://problem/34694193>