| Summary: | [Freetype] Simple and complex paths are not applied consistently | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Carlos Garcia Campos <cgarcia> | ||||||||||||||||
| Component: | Text | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||
| Severity: | Normal | CC: | bugs-noreply, buildbot, koivisto, mcatanzaro, mitz, mmaxfield, rniwa, simon.fraser, webkit-bug-importer, zalan | ||||||||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||||||||
| Version: | WebKit Nightly Build | ||||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||||
| OS: | Unspecified | ||||||||||||||||||
| See Also: |
https://bugs.webkit.org/show_bug.cgi?id=100050 https://bugs.webkit.org/show_bug.cgi?id=179113 |
||||||||||||||||||
| Attachments: |
|
||||||||||||||||||
|
Description
Carlos Garcia Campos
2017-09-28 05:42:50 PDT
Btw, this doesn't happen with WebKitGTK+ 2.18, it happens only in trunk since we enabled the complex text for the emojis support. Created attachment 322076 [details]
Patch
Awesome! I fear EWS is about to spit several Mac test failures at you, though. Comment on attachment 322076 [details] Patch Attachment 322076 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/4687093 New failing tests: imported/w3c/web-platform-tests/css/css-ui-3/text-overflow-013.html fast/inline/padding-ellipsis-right.html svg/text/select-textLength-spacingAndGlyphs-stretch-3.svg svg/text/select-textLength-spacingAndGlyphs-squeeze-3.svg svg/text/select-textLength-spacingAndGlyphs-stretch-2.svg svg/text/select-textLength-spacingAndGlyphs-squeeze-1.svg svg/text/select-textLength-spacingAndGlyphs-stretch-1.svg svg/text/select-textLength-spacingAndGlyphs-squeeze-4.svg svg/text/select-textLength-spacingAndGlyphs-stretch-4.svg platform/mac/fast/text/ligature-subdivision.html svg/text/select-textLength-spacingAndGlyphs-squeeze-2.svg Created attachment 322077 [details]
Archive of layout-test-results from ews101 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 322076 [details] Patch Attachment 322076 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/4687105 New failing tests: svg/text/select-textLength-spacingAndGlyphs-stretch-3.svg imported/w3c/web-platform-tests/css/css-ui-3/text-overflow-013.html svg/text/select-textLength-spacingAndGlyphs-squeeze-3.svg fast/inline/padding-ellipsis-right.html svg/text/select-textLength-spacingAndGlyphs-stretch-2.svg svg/text/select-textLength-spacingAndGlyphs-squeeze-1.svg svg/text/select-textLength-spacingAndGlyphs-stretch-1.svg svg/text/select-textLength-spacingAndGlyphs-squeeze-4.svg svg/text/select-textLength-spacingAndGlyphs-stretch-4.svg platform/mac/fast/text/ligature-subdivision.html svg/text/select-textLength-spacingAndGlyphs-squeeze-2.svg Created attachment 322078 [details]
Archive of layout-test-results from ews106 for mac-elcapitan-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Comment on attachment 322076 [details] Patch Attachment 322076 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/4687130 New failing tests: imported/w3c/web-platform-tests/css/css-ui-3/text-overflow-013.html fast/inline/padding-ellipsis-right.html Created attachment 322081 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.6
Comment on attachment 322076 [details] Patch Attachment 322076 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/4687145 New failing tests: imported/w3c/web-platform-tests/css/css-ui-3/text-overflow-013.html fast/inline/padding-ellipsis-right.html svg/text/select-textLength-spacingAndGlyphs-stretch-3.svg svg/text/select-textLength-spacingAndGlyphs-squeeze-3.svg svg/text/select-textLength-spacingAndGlyphs-stretch-2.svg svg/text/select-textLength-spacingAndGlyphs-squeeze-1.svg svg/text/select-textLength-spacingAndGlyphs-stretch-1.svg svg/text/select-textLength-spacingAndGlyphs-squeeze-4.svg svg/text/select-textLength-spacingAndGlyphs-stretch-4.svg platform/mac/fast/text/ligature-subdivision.html svg/text/select-textLength-spacingAndGlyphs-squeeze-2.svg Created attachment 322082 [details]
Archive of layout-test-results from ews112 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112 Port: mac-elcapitan Platform: Mac OS X 10.11.6
(In reply to Michael Catanzaro from comment #3) > Awesome! > > I fear EWS is about to spit several Mac test failures at you, though. Yes, there are also failures in GTK+, most of them (if not all) just need to be rebaselined, but I want to get feedback/review first before doing the rebaseline. (In reply to Build Bot from comment #4) > Comment on attachment 322076 [details] > Patch > > Attachment 322076 [details] did not pass mac-ews (mac): > Output: http://webkit-queues.webkit.org/results/4687093 > > New failing tests: > imported/w3c/web-platform-tests/css/css-ui-3/text-overflow-013.html > fast/inline/padding-ellipsis-right.html > svg/text/select-textLength-spacingAndGlyphs-stretch-3.svg > svg/text/select-textLength-spacingAndGlyphs-squeeze-3.svg > svg/text/select-textLength-spacingAndGlyphs-stretch-2.svg > svg/text/select-textLength-spacingAndGlyphs-squeeze-1.svg > svg/text/select-textLength-spacingAndGlyphs-stretch-1.svg > svg/text/select-textLength-spacingAndGlyphs-squeeze-4.svg > svg/text/select-textLength-spacingAndGlyphs-stretch-4.svg > platform/mac/fast/text/ligature-subdivision.html > svg/text/select-textLength-spacingAndGlyphs-squeeze-2.svg Myles, could it be that these tests expect the partial run computations to be done by the complex text path? the non-svg tests pass by running WTR with --complex-text option and the svg ones fail but pass ( the right thing is selected, they would need to be rebaselined). Actually I would say the svg tests are indeed a proof of this bug, the computation match when using --complex-text but the rendering doesn't. Btw, don't pay attention to the image diffs of the svg test failures, because the pixel expectations are outdated. Myles, could you review or comment about this patch? I have to make our first "preview" release of this cycle from trunk next week and selections are completely broken for us due to this bug. If this behavior is not desired for mac (I think it is), I can simply add GTK and WPE ifdefs keeping the current behavior in mac. Or I can do that temporarily until we figure out the test failures. If those tests are actually expected to be run with complex path enabled I can try to add a way to enable complex text for particular tests in WTR/DRT. Or I can simply mark them as expected failures and file a new bug report. I'm pretty busy today but I'll try to review it in the evening. Ping. Myles, if you don't have time to look at this now, I can simply make this change harfbuzz only with a FIXME and then you can decide whether to adopt this in mac too when you find the time, because we definitely want (and need) this change (unless it's wrong for some reason). Comment on attachment 322076 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=322076&action=review How sure are you that there isn't a bug with your fast text codepath? Can you explain why we ever opt into the fast text codepath for a situation where we aren't sure that it would get the same results as the complex path? > Source/WebCore/ChangeLog:20 > + Due to bug #100050, when rendering text, the complex path is forced in case kerning or shaping is enabled and > + only part of the run is going to be rendered. This happens in the GTK+ port when selecting text (except when > + selecting the whole run, of course). The text is initially rendered using the simple path as returned by > + FontCascade::codePath() and then the selection is rendered using the complex path, overriding what > + FontCascade::codePath() returned in that case. This doesn't happen in mac, because the selection is rendered > + differently, so FontCascade::drawText always renders the full run (simple path) when selecting text. Selecting > + text is the most noticeable inconsistency, but it's not the only one. Similar exceptions are applied when > + calculating the text width, or getting the offset of a given position. The rendered text is the simple one, but > + the calculations are performed using the complex path, so depending on the kerning and ligatures we might end up > + with wrong results. If the text has been rendered using the simple path, the selections and all other > + calculations should be performed with the simple path too. This patch moves the condition to force complex text > + to FontCascade::codePath(), and only for ports not enabling advance text rendering mode by default. This ensures > + that all callers to FontCascade::codePath() will get a consistent result. I'm not sure this is the correct approach. The source of truth is the complex text codepath; if the fast one gives results which are not the same as the complex path's results, then the fast path is wrong. It sounds like there's just a bug in your simple text codepath. I think the fast and complex paths cannot, in general, produce matching results due to bug #167557. Maybe more bugs too, of course :) (In reply to Michael Catanzaro from comment #19) > I think the fast and complex paths cannot, in general, produce matching > results due to bug #167557. In that case, this fix should be Freetype-only. I really want to get rid of one of our text codepaths.... then this wouldn't be an issue.... sigh. Yes, the thing is that simple path doesn't support kerning and ligatures, so we don't get the same results in terms of size and position, as you can see in the screenshot I took. What we want is to ensure that the same conditions applied to decide whether a full run should go complex or simple is applied to a partial run, because in our case selections are always partial runs (except when selecting entire lines). I'll make this GTK/WPE only then. Maybe it no longer makes sense for GTK/WPE to support the simple path. What Myles suggested we really need are performance tests to figure out if we still need the simple path. Now, I have no clue, but I suppose it's probably not possible to add ligature and kerning support to our simple path? If it is possible, then that would be the proper fix. In the meantime, it's a real shame. We get complaints about poor kerning not irregularly. (In reply to Michael Catanzaro from comment #23) > If it is possible, then that would be the proper fix. I don't mean to suggest that you should not also make this change. I guess using #if USE(FREETYPE) conditions would be best. (In reply to Michael Catanzaro from comment #23) > Maybe it no longer makes sense for GTK/WPE to support the simple path. What > Myles suggested we really need are performance tests to figure out if we > still need the simple path. > > Now, I have no clue, but I suppose it's probably not possible to add > ligature and kerning support to our simple path? If it is possible, then > that would be the proper fix. In the meantime, it's a real shame. We get > complaints about poor kerning not irregularly. I don’t know if there is API for it in HarfBuzz but, even if there isn’t, I’d be willing to bet the HarfBuzz community would accept a patch to add such an API. I would be very interested to hear any performance numbers you gather from disabling the fast text codepath. (In reply to Michael Catanzaro from comment #24) > (In reply to Michael Catanzaro from comment #23) > > If it is possible, then that would be the proper fix. > > I don't mean to suggest that you should not also make this change. I guess > using #if USE(FREETYPE) conditions would be best. You could also make the “figure out which codepath to use” function platform-specific. Might be cleaner than spraying macros everywhere. (In reply to Myles C. Maxfield from comment #25) > (In reply to Michael Catanzaro from comment #23) > > Maybe it no longer makes sense for GTK/WPE to support the simple path. What > > Myles suggested we really need are performance tests to figure out if we > > still need the simple path. > > > > Now, I have no clue, but I suppose it's probably not possible to add > > ligature and kerning support to our simple path? If it is possible, then > > that would be the proper fix. In the meantime, it's a real shame. We get > > complaints about poor kerning not irregularly. > > I don’t know if there is API for it in HarfBuzz but, even if there isn’t, > I’d be willing to bet the HarfBuzz community would accept a patch to add > such an API. > > I would be very interested to hear any performance numbers you gather from > disabling the fast text codepath. Chrome uses HarfBuzz and I believe they have deleted their fast text codepath. (In reply to Michael Catanzaro from comment #23) > Maybe it no longer makes sense for GTK/WPE to support the simple path. What > Myles suggested we really need are performance tests to figure out if we > still need the simple path. Exactly, we need a way to measure performance to make a decision. > Now, I have no clue, but I suppose it's probably not possible to add > ligature and kerning support to our simple path? If it is possible, then > that would be the proper fix. In the meantime, it's a real shame. We get > complaints about poor kerning not irregularly. I have no idea how to do that. (In reply to Myles C. Maxfield from comment #27) > (In reply to Myles C. Maxfield from comment #25) > > (In reply to Michael Catanzaro from comment #23) > > > Maybe it no longer makes sense for GTK/WPE to support the simple path. What > > > Myles suggested we really need are performance tests to figure out if we > > > still need the simple path. > > > > > > Now, I have no clue, but I suppose it's probably not possible to add > > > ligature and kerning support to our simple path? If it is possible, then > > > that would be the proper fix. In the meantime, it's a real shame. We get > > > complaints about poor kerning not irregularly. > > > > I don’t know if there is API for it in HarfBuzz but, even if there isn’t, > > I’d be willing to bet the HarfBuzz community would accept a patch to add > > such an API. > > > > I would be very interested to hear any performance numbers you gather from > > disabling the fast text codepath. > > Chrome uses HarfBuzz and I believe they have deleted their fast text > codepath. And pango, which is what GTK+ uses for text shaping and rendering, uses harfbuzz unconditionally too. (In reply to Michael Catanzaro from comment #24) > (In reply to Michael Catanzaro from comment #23) > > If it is possible, then that would be the proper fix. > > I don't mean to suggest that you should not also make this change. I guess > using #if USE(FREETYPE) conditions would be best. Yes, let's fix this for 2.19.1, then we can measure and decide. Created attachment 325335 [details]
Patch
Comment on attachment 325335 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=325335&action=review > Source/WebCore/ChangeLog:3 > + [Freetype] Simple and complex paths are not applied consistently FreeType! Committed r224223: <https://trac.webkit.org/changeset/224223> |