[Win] fast/text/stale-TextLayout-from-first-line.html is failing since Bug 204884
Created attachment 386177 [details] simplified test case ̀ doesn't show because ComplexTextController::adjustGlyphsAndAdvances replaces a glyph of ̀ 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.
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..
Created attachment 386326 [details] WIP patch
Created attachment 387312 [details] [Screenshot] WinCairo port It breaks happily combining family glyphs. https://emojiterra.com/family/
*** Bug 206058 has been marked as a duplicate of this bug. ***
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.
Created attachment 387908 [details] Patch
Created attachment 387990 [details] Patch
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>?
(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>
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.
Created attachment 388519 [details] Patch for landing
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.
Comment on attachment 388519 [details] Patch for landing Clearing flags on attachment: 388519 Committed r254965: <https://trac.webkit.org/changeset/254965>
All reviewed patches have been landed. Closing bug.
<rdar://problem/58825349>