Bug 174418 – [HarfBuzz] Decomposed Vietnamese characters are rendered incorrectly

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Fujii Hironori]

Created attachment 315220 [details]
screenshot

[HarfBuzz] Decomposed Vietnamese characters are rendered incorrectly

For example, "ể" has two unicode representation "ể" and "ể".
https://en.wiktionary.org/wiki/%E1%BB%83
Those characters should be rendered identically.

[Fujii Hironori]

Created attachment 315221 [details]
test case

[Fujii Hironori]

HarfBuzzShaper should normalize the input text before shaping.

Actually, HarfBuzzShaper::setNormalizedBuffer does the task.
https://trac.webkit.org/browser/webkit/trunk/Source/WebCore/platform/graphics/harfbuzz/HarfBuzzShaper.cpp?rev=219385#L233

But, this function hasn't called from anywhere since Bug 108077.

[Fujii Hironori]

Created attachment 315321 [details]
Patch

[Fujii Hironori]

Created attachment 315322 [details]
vietnamese-nfd-actual.png

[Fujii Hironori]

Created attachment 315323 [details]
vietnamese-nfd-expected.png

[Fujii Hironori]

(In reply to Fujii Hironori from comment #2)
> But, this function hasn't called from anywhere since Bug 108077.

Not Bug 108077, but Bug 90951.

[Fujii Hironori]

It'd be great help if you can take a glance at my patch and give me your thoughts.

[Kenichi Ishibashi]

The patch looks good to me. Please ask WebKit reviewers for actual review :)

[Fujii Hironori]

It's a great helpful. Thank you so much to have a time to review my patch.

[Michael Catanzaro]

Oh wow, apparently it's font bug week or something!

I have not the faintest clue if your patch is right or not. I'm encouraged that it fixes the problem and includes a layout test. I'm also encouraged to see that Kenichi likes your patch. I see that he wrote all this code, so that's good I suppose!

[Michael Catanzaro]

Comment on attachment 315321 [details]
Patch

I'm giving r+ even though I don't understand the code, because I'm pretty sure we don't have any Igalia reviewers who might understand it. Myles, if you see any reason not to commit this, please say so.

I have a suspicion that this commit might affect a lot of test results, so we will need to watch the bots carefully after landing it. So we will have to try landing this another day.

[Myles C. Maxfield]

(In reply to Michael Catanzaro from comment #10)
> I have not the faintest clue if your patch is right or not. I'm encouraged
> that it fixes the problem and includes a layout test. I'm also encouraged to
> see that Kenichi likes your patch. I see that he wrote all this code, so
> that's good I suppose!

😁

[Michael Catanzaro]

Comment on attachment 315321 [details]
Patch

OK, Myles posted a happy emojii, and the tree is back open, so we should be good to cq+ this tomorrow when we're available for watching the bots.

[WebKit Commit Bot]

Comment on attachment 315321 [details]
Patch

Clearing flags on attachment: 315321

Committed r219504: <http://trac.webkit.org/changeset/219504>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Michael Catanzaro]

Looks like there's no difference in the test results. OK then.

[Fujii Hironori]

Thank you, Michael. I'll keep an eys on it during this weekend. I'll investigate new failures next Tuesday if any. (next Monday is Japan's national holiday)

[Michael Catanzaro]

There were not any new failures!

[Michael Catanzaro]

Oh actually there was a regression, I just missed it. :( It's imported/blink/fast/text/international/text-shaping-arabic-diffs.html:

Expected image: https://build.webkit.org/results/GTK%20Linux%2064-bit%20Release%20(Tests)/r220734%20(2623)/imported/blink/fast/text/international/text-shaping-arabic-diffs.html

New image after this patch was committed: https://build.webkit.org/results/GTK%20Linux%2064-bit%20Release%20(Tests)/r220734%20(2623)/imported/blink/fast/text/international/text-shaping-arabic-actual.png

Fujii, any ideas?

[WebKit Commit Bot]

Re-opened since this is blocked by bug 175580

[Fujii Hironori]

Thank you for rolling back my change, Michael. I'm sorry for not noticing the regression.

normalizeCharacters was using FontCascade::treatAsZeroWidthSpaceInComplexScript while normalizeSpacesAndMirrorChars is using FontCascade::treatAsZeroWidthSpace.

[Fujii Hironori]

Created attachment 318221 [details]
Patch

[Michael Catanzaro]

Careful, or you'll become a text layout expert!

Is this patch ready for review?

[Fujii Hironori]

Comment on attachment 318221 [details]
Patch

EWS got green. finished testing in my env. It's ready for review now.

[WebKit Commit Bot]

Comment on attachment 318221 [details]
Patch

Clearing flags on attachment: 318221

Committed r220797: <http://trac.webkit.org/changeset/220797>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/33920068>