Bug 166013 - Correct spacing regression on inter-element complex path shaping on some fonts
Summary: Correct spacing regression on inter-element complex path shaping on some fonts
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Text (show other bugs)
Version: WebKit Nightly Build
Hardware: Mac macOS 10.12
: P2 Normal
Assignee: Myles C. Maxfield
URL:
Keywords: InRadar
: 167363 (view as bug list)
Depends on: 167493
Blocks:
  Show dependency treegraph
 
Reported: 2016-12-18 11:52 PST by Ebrahim Byagowi
Modified: 2017-03-11 22:01 PST (History)
11 users (show)

See Also:


Attachments
Browser comparison (368.72 KB, image/png)
2017-01-03 19:39 PST, Myles C. Maxfield
no flags Details
The different part (8.39 KB, image/png)
2017-01-03 22:43 PST, Ebrahim Byagowi
no flags Details
Selection screenshot (98.25 KB, image/png)
2017-01-04 10:42 PST, Said Abou-Hallawa
no flags Details
Font and word spacing change (1.08 MB, video/quicktime)
2017-01-04 10:47 PST, Said Abou-Hallawa
no flags Details
Screenshot of the page once it is loaded (203.94 KB, image/png)
2017-01-04 10:53 PST, Said Abou-Hallawa
no flags Details
Screenshot of the page after a few seconds. (211.99 KB, image/png)
2017-01-04 10:53 PST, Said Abou-Hallawa
no flags Details
Minimized WIP (3.50 KB, patch)
2017-01-24 01:32 PST, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Test (21.21 KB, patch)
2017-01-25 12:04 PST, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Test (22.34 KB, patch)
2017-01-25 13:59 PST, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
WIP (27.00 KB, patch)
2017-01-25 16:22 PST, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
WIP (50.71 KB, patch)
2017-01-26 11:00 PST, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
WIP (33.85 KB, patch)
2017-01-26 15:02 PST, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
WIP (34.61 KB, patch)
2017-01-26 15:42 PST, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
WIP (35.30 KB, patch)
2017-01-26 15:45 PST, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
WIP (37.91 KB, patch)
2017-01-26 16:06 PST, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
WIP (13.09 KB, patch)
2017-01-27 09:54 PST, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
WIP (20.71 KB, patch)
2017-01-27 17:28 PST, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
WIP (21.62 KB, patch)
2017-01-27 17:31 PST, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
WIP (21.39 KB, patch)
2017-01-28 02:19 PST, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (32.84 KB, patch)
2017-01-28 04:10 PST, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews100 for mac-elcapitan (784.50 KB, application/zip)
2017-01-28 05:17 PST, Build Bot
no flags Details
Archive of layout-test-results from ews104 for mac-elcapitan-wk2 (903.52 KB, application/zip)
2017-01-28 05:20 PST, Build Bot
no flags Details
Archive of layout-test-results from ews113 for mac-elcapitan (1.67 MB, application/zip)
2017-01-28 05:27 PST, Build Bot
no flags Details
Patch (35.64 KB, patch)
2017-01-28 12:16 PST, Myles C. Maxfield
simon.fraser: review+
Details | Formatted Diff | Diff
Patch for committing (39.23 KB, patch)
2017-01-30 11:15 PST, Myles C. Maxfield
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ebrahim Byagowi 2016-12-18 11:52:45 PST
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
Comment 1 Radar WebKit Bug Importer 2016-12-19 11:41:47 PST
<rdar://problem/29738514>
Comment 2 Myles C. Maxfield 2017-01-03 19:39:48 PST
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!
Comment 3 Myles C. Maxfield 2017-01-03 19:40:12 PST
(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
Comment 4 Ebrahim Byagowi 2017-01-03 22:15:15 PST
(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.
Comment 5 Ebrahim Byagowi 2017-01-03 22:43:33 PST
Created attachment 297992 [details]
The different part
Comment 6 Said Abou-Hallawa 2017-01-04 10:42:09 PST
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.
Comment 7 Said Abou-Hallawa 2017-01-04 10:47:42 PST
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.
Comment 8 Said Abou-Hallawa 2017-01-04 10:52:27 PST
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.
Comment 9 Said Abou-Hallawa 2017-01-04 10:53:03 PST
Created attachment 298028 [details]
Screenshot of the page once it is loaded
Comment 10 Said Abou-Hallawa 2017-01-04 10:53:33 PST
Created attachment 298029 [details]
Screenshot of the page after a few seconds.
Comment 11 Myles C. Maxfield 2017-01-24 01:32:39 PST
Created attachment 299587 [details]
Minimized WIP
Comment 12 Myles C. Maxfield 2017-01-24 01:32:52 PST
*** Bug 167363 has been marked as a duplicate of this bug. ***
Comment 13 Myles C. Maxfield 2017-01-24 01:33:28 PST
This was a super helpful bug report. Thank you so much for filing it. It found a serious problem with our Arabic text handling.
Comment 14 Myles C. Maxfield 2017-01-25 12:04:20 PST
Created attachment 299723 [details]
Test
Comment 15 Myles C. Maxfield 2017-01-25 13:59:39 PST
Created attachment 299738 [details]
Test
Comment 16 Myles C. Maxfield 2017-01-25 16:22:52 PST
Created attachment 299763 [details]
WIP
Comment 17 Myles C. Maxfield 2017-01-26 11:00:15 PST
Created attachment 299818 [details]
WIP
Comment 18 Myles C. Maxfield 2017-01-26 15:02:41 PST
Created attachment 299860 [details]
WIP
Comment 19 Myles C. Maxfield 2017-01-26 15:42:28 PST
Created attachment 299865 [details]
WIP
Comment 20 Myles C. Maxfield 2017-01-26 15:45:29 PST
Created attachment 299867 [details]
WIP
Comment 21 Myles C. Maxfield 2017-01-26 16:06:41 PST
Created attachment 299872 [details]
WIP
Comment 22 Myles C. Maxfield 2017-01-27 09:54:47 PST
Created attachment 299938 [details]
WIP
Comment 23 Myles C. Maxfield 2017-01-27 09:55:10 PST
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 ]
Comment 24 Myles C. Maxfield 2017-01-27 17:28:12 PST
Created attachment 299985 [details]
WIP
Comment 25 Myles C. Maxfield 2017-01-27 17:31:18 PST
Created attachment 299986 [details]
WIP
Comment 26 Myles C. Maxfield 2017-01-28 02:19:10 PST
Created attachment 300015 [details]
WIP
Comment 27 Myles C. Maxfield 2017-01-28 04:10:11 PST
Created attachment 300023 [details]
Patch
Comment 28 Build Bot 2017-01-28 05:17:37 PST
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
Comment 29 Build Bot 2017-01-28 05:17:41 PST
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
Comment 30 Build Bot 2017-01-28 05:19:58 PST
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
Comment 31 Build Bot 2017-01-28 05:20:01 PST
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
Comment 32 Build Bot 2017-01-28 05:26:58 PST
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
Comment 33 Build Bot 2017-01-28 05:27:02 PST
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
Comment 34 Myles C. Maxfield 2017-01-28 12:16:52 PST
Created attachment 300047 [details]
Patch
Comment 35 Simon Fraser (smfr) 2017-01-30 10:46:18 PST
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.
Comment 36 Myles C. Maxfield 2017-01-30 10:51:05 PST
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.
Comment 37 Myles C. Maxfield 2017-01-30 11:15:19 PST
Created attachment 300125 [details]
Patch for committing
Comment 38 WebKit Commit Bot 2017-01-30 12:44:37 PST
Comment on attachment 300125 [details]
Patch for committing

Clearing flags on attachment: 300125

Committed r211382: <http://trac.webkit.org/changeset/211382>
Comment 39 mitz 2017-03-11 22:01:45 PST
(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.