Bug 166013 – Correct spacing regression on inter-element complex path shaping on some fonts

• Status: RESOLVED
• Resolution: FIXED
• Component: Text

[Ebrahim Byagowi]

Steps to repro:
Compare stable Safari and nightly webkit on this page
https://fa.wikipedia.org/wiki/User:Ebrahim/t1?printable=yes&withCSS=MediaWiki:Gadget-Vazir.css

Actual:
After complete load of the page with its webfonts, space between the two big Arabic words is almost removed from the result which is not right.

Expected:
Result of stable Safari, just like Chrome and other browsers.


This is somehow in continuation of Bug 165084

[Radar WebKit Bug Importer]

<rdar://problem/29738514>

[Myles C. Maxfield]

Created attachment 297988 [details]
Browser comparison

It looks pretty similar to me (but I don't speak this language). Can you point out which part is wrong in the screenshot I just uploaded?

Thanks!

[Myles C. Maxfield]

(In reply to comment #2)
> Created attachment 297988 [details]
> Browser comparison
> 
> It looks pretty similar to me (but I don't speak this language). Can you
> point out which part is wrong in the screenshot I just uploaded?
> 
> Thanks!

The screenshot was taken as of r210240

[Ebrahim Byagowi]

(In reply to comment #2)
> Created attachment 297988 [details]
> Browser comparison
> 
> It looks pretty similar to me (but I don't speak this language). Can you
> point out which part is wrong in the screenshot I just uploaded?
> 
> Thanks!

The difference is on the space between first connected component with the second, they are two regular space separated words but on WebKit, it seems the space is not considered, you can use total element width to compare them.

[Ebrahim Byagowi]

Created attachment 297992 [details]
The different part

[Said Abou-Hallawa]

Created attachment 298026 [details]
Selection screenshot

When trying to select the space between the two words in the third line (the line with the largest font) and then selecting the first letter in the second word, an overlap appears in the selection highlight. This shows that the second word is misplaced.

[Said Abou-Hallawa]

Created attachment 298027 [details]
Font and word spacing change

This video shows that the page was displayed with the correct word spacing at the beginning and then the layout of the page changed. So we get the wrong word spacing at the end.

[Said Abou-Hallawa]

The video does not work. So I am going to attach a screenshot of the page once the page is loaded. And another screenshot of the page after few seconds when the layout, the font and the word spacing change automatically.

[Said Abou-Hallawa]

Created attachment 298028 [details]
Screenshot of the page once it is loaded

[Said Abou-Hallawa]

Created attachment 298029 [details]
Screenshot of the page after a few seconds.

[Myles C. Maxfield]

Created attachment 299587 [details]
Minimized WIP

[Myles C. Maxfield]

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

[Myles C. Maxfield]

This was a super helpful bug report. Thank you so much for filing it. It found a serious problem with our Arabic text handling.

[Myles C. Maxfield]

Created attachment 299723 [details]
Test

[Myles C. Maxfield]

Created attachment 299738 [details]
Test

[Myles C. Maxfield]

Created attachment 299763 [details]
WIP

[Myles C. Maxfield]

Created attachment 299818 [details]
WIP

[Myles C. Maxfield]

Created attachment 299860 [details]
WIP

[Myles C. Maxfield]

Created attachment 299865 [details]
WIP

[Myles C. Maxfield]

Created attachment 299867 [details]
WIP

[Myles C. Maxfield]

Created attachment 299872 [details]
WIP

[Myles C. Maxfield]

Created attachment 299938 [details]
WIP

[Myles C. Maxfield]

fast/ruby/ruby-expansion-cjk-4.html [ ImageOnlyFailure ]
  fast/text/complex-first-glyph-with-initial-advance.html [ ImageOnlyFailure ]
  fast/text/complex-initial-advance.html [ ImageOnlyFailure ]
  imported/blink/fast/text/international/vertical-positioning-with-combining-marks.html [ ImageOnlyFailure ]

[Myles C. Maxfield]

Created attachment 299985 [details]
WIP

[Myles C. Maxfield]

Created attachment 299986 [details]
WIP

[Myles C. Maxfield]

Created attachment 300015 [details]
WIP

[Myles C. Maxfield]

Created attachment 300023 [details]
Patch

[Build Bot]

Comment on attachment 300023 [details]
Patch

Attachment 300023 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/2963464

New failing tests:
fast/text/initial-advance-in-intermediate-run-complex.html

[Build Bot]

Created attachment 300028 [details]
Archive of layout-test-results from ews100 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 300023 [details]
Patch

Attachment 300023 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2963469

New failing tests:
fast/text/initial-advance-in-intermediate-run-complex.html

[Build Bot]

Created attachment 300029 [details]
Archive of layout-test-results from ews104 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 300023 [details]
Patch

Attachment 300023 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/2963470

New failing tests:
fast/text/initial-advance-in-intermediate-run-complex.html

[Build Bot]

Created attachment 300031 [details]
Archive of layout-test-results from ews113 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Myles C. Maxfield]

Created attachment 300047 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 300047 [details]
Patch

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

> Source/WebCore/ChangeLog:11
> +        - The total width of the ComplexTextController didn't match the width if

of then -> computed by?

> Source/WebCore/platform/graphics/mac/ComplexTextController.cpp:124
> +ComplexTextController::ComplexTextController(const FontCascade& font, const TextRun& run, bool mayUseNaturalWritingDirection, HashSet<const Font*>* fallbackFonts, bool forTextEmphasis)

If fallbackFonts is just input data, can it be const?

> Source/WebCore/platform/graphics/mac/ComplexTextController.cpp:614
> +                if (!g) {

Hard to tell what "g" is here.

> Source/WebCore/platform/graphics/mac/ComplexTextController.cpp:618
> +                    paintAdvance.setWidth(paintAdvance.width() - (complexTextRun.initialAdvance().width - currentGlyphOrigin.x));
> +                    paintAdvance.setHeight(paintAdvance.height() - (complexTextRun.initialAdvance().height - currentGlyphOrigin.y));

Could this be done as point/size math?

> Source/WebCore/platform/graphics/mac/ComplexTextController.cpp:621
> +                paintAdvance.setWidth(paintAdvance.width() + glyphOrigin(k + 1).x - currentGlyphOrigin.x);
> +                paintAdvance.setHeight(paintAdvance.height() + glyphOrigin(k + 1).y - currentGlyphOrigin.y);

Ditto.

> Source/WebCore/platform/graphics/mac/ComplexTextController.cpp:627
> +                    paintAdvance.setWidth(paintAdvance.width() - glyphOrigin(k + 1).x + m_complexTextRuns[currentRunIndex + 1]->initialAdvance().width);
> +                    paintAdvance.setHeight(paintAdvance.height() - glyphOrigin(k + 1).y + m_complexTextRuns[currentRunIndex + 1]->initialAdvance().height);

Ditto.

> Source/WebCore/platform/graphics/mac/ComplexTextController.cpp:746
> +                advance.width += complexTextRun.initialAdvance().width;
> +                advance.height += complexTextRun.initialAdvance().height;
> +                if (auto* origins = complexTextRun.glyphOrigins()) {
> +                    advance.width -= origins[0].x;
> +                    advance.height -= origins[0].y;

Use point math?

> Source/WebCore/platform/graphics/mac/ComplexTextController.h:134
> +        CGSize& initialAdvance() { return m_initialAdvance; }

Would prefer an explicit setter.

[Myles C. Maxfield]

Comment on attachment 300047 [details]
Patch

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

>> Source/WebCore/platform/graphics/mac/ComplexTextController.cpp:124
>> +ComplexTextController::ComplexTextController(const FontCascade& font, const TextRun& run, bool mayUseNaturalWritingDirection, HashSet<const Font*>* fallbackFonts, bool forTextEmphasis)
> 
> If fallbackFonts is just input data, can it be const?

It's output data. It's used for computing the line-height of a block. if line-height is normal, the line-height is computed to be the maximum ascent + the maximum descent, and fallback fonts are considered during this calculation. Therefore, text layout needs to gather all the fallback fonts used and report it to the higher-level layout code.

>> Source/WebCore/platform/graphics/mac/ComplexTextController.cpp:618
>> +                    paintAdvance.setHeight(paintAdvance.height() - (complexTextRun.initialAdvance().height - currentGlyphOrigin.y));
> 
> Could this be done as point/size math?

I have a WIP to do this at https://bugs.webkit.org/show_bug.cgi?id=167566 but it's large enough I didn't want to pollute this patch with it.

>> Source/WebCore/platform/graphics/mac/ComplexTextController.cpp:621
>> +                paintAdvance.setHeight(paintAdvance.height() + glyphOrigin(k + 1).y - currentGlyphOrigin.y);
> 
> Ditto.

Ditto.

>> Source/WebCore/platform/graphics/mac/ComplexTextController.cpp:627
>> +                    paintAdvance.setHeight(paintAdvance.height() - glyphOrigin(k + 1).y + m_complexTextRuns[currentRunIndex + 1]->initialAdvance().height);
> 
> Ditto.

Ditto.

>> Source/WebCore/platform/graphics/mac/ComplexTextController.cpp:746
>> +                    advance.height -= origins[0].y;
> 
> Use point math?

Ditto.

[Myles C. Maxfield]

Created attachment 300125 [details]
Patch for committing

[WebKit Commit Bot]

Comment on attachment 300125 [details]
Patch for committing

Clearing flags on attachment: 300125

Committed r211382: <http://trac.webkit.org/changeset/211382>

(In reply to comment #38)
> Comment on attachment 300125 [details]
> Patch for committing
> 
> Clearing flags on attachment: 300125
> 
> Committed r211382: <http://trac.webkit.org/changeset/211382>

This caused bug 169517.