Bug 205485 – [WinCairo] Some of combining family glyphs are replaced by space glyphs

• Component: Text
• Status: RESOLVED FIXED

[Fujii Hironori]

[Win] fast/text/stale-TextLayout-from-first-line.html is failing since Bug 204884

[Fujii Hironori]

Created attachment 386177 [details]
simplified test case

̀ doesn't show because
ComplexTextController::adjustGlyphsAndAdvances replaces a glyph
of &#x0300 with the space glyph.

stringIndices given by collectComplexTextRunsForCharacters are incorrect.

Uniscribe returns clusters.
For " ̀", it returns [0 0]. This means both glyphs consist of a single cluster starting at 0th glyph.

Then, clusters are converted into stringIndices. It's [0 0].
This means both glyphs correspond to 0th charcther. This is incorrect.
ComplexTextController::adjustGlyphsAndAdvances replaces glyphs in the positions of space charcthers with the space glyph.

[Fujii Hironori]

fast/text/emoji-single-parent-family.html has a similar issue.
👨‍👦 shows only the father glyph because ComplexTextController replaces the boy glyph with a space glyph.

ScriptShape

input text: [👨 ‍ 👦] (length=5)
output glyphs: [6368 6318] which are fater and boy glyphs
output clusters: [0 0 1 1 1] which means consisting two clusters, and the 0th cluster starts at glyphs[0] (father glyph), 1st cluster starts at glyphs[1] (boy glyph)

Then,

stringIndices: [0 2] which means glyphs[0] points to text[0], glyphs[1] points to text[2] (zwj).

ComplexTextController replaces the boy glyph with a space glyph because glyphs[0] (child glyph) points to zwj..

[Fujii Hironori]

Created attachment 386326 [details]
WIP patch

[Fujii Hironori]

Created attachment 387312 [details]
[Screenshot] WinCairo port

It breaks happily combining family glyphs.
https://emojiterra.com/family/

[Fujii Hironori]

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

[Fujii Hironori]

This patch solves the missing glyph problem of fast/text/stale-TextLayout-from-first-line.html,
but it doesn't pass the test yet due to another bug.

I change the summary of bug, and file another bug for fast/text/stale-TextLayout-from-first-line.html.

[Fujii Hironori]

Created attachment 387908 [details]
Patch

[Fujii Hironori]

Created attachment 387990 [details]
Patch

[Myles C. Maxfield]

Comment on attachment 387990 [details]
Patch

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

> Source/WebCore/platform/graphics/win/ComplexTextControllerUniscribe.cpp:146
> +        int endGlyphIndex = !finish ? clusters[endStringIndex] : -1;

Nowadays we prefer to use out-of-band sentinels. How about Optional<unsigned>?

[zalan]

(In reply to Myles C. Maxfield from comment #9)
> Comment on attachment 387990 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=387990&action=review
> 
> > Source/WebCore/platform/graphics/win/ComplexTextControllerUniscribe.cpp:146
> > +        int endGlyphIndex = !finish ? clusters[endStringIndex] : -1;
> 
> Nowadays we prefer to use out-of-band sentinels. How about
> Optional<unsigned>?
or Optional<size_t>

[Fujii Hironori]

Comment on attachment 387990 [details]
Patch

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

Thank you very much for the review.

>>> Source/WebCore/platform/graphics/win/ComplexTextControllerUniscribe.cpp:146
>>> +        int endGlyphIndex = !finish ? clusters[endStringIndex] : -1;
>> 
>> Nowadays we prefer to use out-of-band sentinels. How about Optional<unsigned>?
> 
> or Optional<size_t>

Yes, this part is bad.
It should use stringIndicesRange.end() as the sentinel if finish is true.

If the real index is used, by allowing to get the real index of end(), the code would look like:

> int endGlyphIndex = !finish ? clusters[endStringIndex] : stringIndicesRange.end().index();

If the iterator is used, by add a method to get a iterator from real index, the code would look like:

> auto endGlyphIndex = !finish ? stringIndicesRange.fromIndex(clusters[endStringIndex]) : stringIndicesRange.end();

I will take later way.

[Fujii Hironori]

Created attachment 388519 [details]
Patch for landing

[Fujii Hironori]

Comment on attachment 388519 [details]
Patch for landing

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

> Source/WebCore/platform/graphics/win/ComplexTextControllerUniscribe.cpp:162
> +        for (; glyphIndex != endGlyphIterator; ++glyphIndex)

This loop condition was greatly improved.
Less condition, no conversion from a iterator to a real index.

before:

> for (; glyphIndex != stringIndicesRange.end() && glyphIndex.index() != endGlyphIndex; ++glyphIndex)

after:

> for (; glyphIndex != endGlyphIterator; ++glyphIndex)

Thank you very much.

[Fujii Hironori]

Comment on attachment 388519 [details]
Patch for landing

Clearing flags on attachment: 388519

Committed r254965: <https://trac.webkit.org/changeset/254965>

[Fujii Hironori]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/58825349>