Summary: | Correct spacing regression on inter-element complex path shaping on some fonts | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Ebrahim Byagowi <ebrahim> | ||||||||||||||||||||||||||||||||||||||||||||||||||||
Component: | Text | Assignee: | Myles C. Maxfield <mmaxfield> | ||||||||||||||||||||||||||||||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
Severity: | Normal | CC: | buildbot, commit-queue, dino, jonlee, mitz, mmaxfield, rniwa, sabouhallawa, simon.fraser, thorton, webkit-bug-importer | ||||||||||||||||||||||||||||||||||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||||||||||||||||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
Hardware: | Mac | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
OS: | macOS 10.12 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=169517 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
Bug Depends on: | 167493 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
Bug Blocks: | |||||||||||||||||||||||||||||||||||||||||||||||||||||||
Attachments: |
|
Description
Ebrahim Byagowi
2016-12-18 11:52:45 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!
(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 (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. Created attachment 297992 [details]
The different part
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.
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.
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. Created attachment 298028 [details]
Screenshot of the page once it is loaded
Created attachment 298029 [details]
Screenshot of the page after a few seconds.
Created attachment 299587 [details]
Minimized WIP
*** Bug 167363 has been marked as a duplicate of this bug. *** This was a super helpful bug report. Thank you so much for filing it. It found a serious problem with our Arabic text handling. Created attachment 299723 [details]
Test
Created attachment 299738 [details]
Test
Created attachment 299763 [details]
WIP
Created attachment 299818 [details]
WIP
Created attachment 299860 [details]
WIP
Created attachment 299865 [details]
WIP
Created attachment 299867 [details]
WIP
Created attachment 299872 [details]
WIP
Created attachment 299938 [details]
WIP
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 ] Created attachment 299985 [details]
WIP
Created attachment 299986 [details]
WIP
Created attachment 300015 [details]
WIP
Created attachment 300023 [details]
Patch
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 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 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 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 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 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
Created attachment 300047 [details]
Patch
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 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. Created attachment 300125 [details]
Patch for committing
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. |