| Summary: | font-variant-caps does not work if the font does not support font features | ||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Myles C. Maxfield <mmaxfield> | ||||||||||||||||||||||||||||||||||||
| Component: | Text | Assignee: | Myles C. Maxfield <mmaxfield> | ||||||||||||||||||||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||||||||||
| Severity: | Normal | CC: | buildbot, commit-queue, dino, jonlee, koivisto, mcatanzaro, nickshanks, rniwa, ryanhaddad, simon.fraser, thorton, webkit-bug-importer | ||||||||||||||||||||||||||||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||||||||||||||||
| Version: | WebKit Nightly Build | ||||||||||||||||||||||||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||||||||||||||||||||||||
| OS: | Unspecified | ||||||||||||||||||||||||||||||||||||||
| Bug Depends on: | |||||||||||||||||||||||||||||||||||||||
| Bug Blocks: | 149779 | ||||||||||||||||||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||||||||||||||||||
|
Description
Myles C. Maxfield
2015-10-02 16:55:42 PDT
I'm going to break this into two bugs - one for font-variant-position and one for font-variant-caps. True small caps requires font features, which will force us down our complex text code path. I need to make sure this won't be a performance regression. Let's assume that it is not a performance regression, for now. In that case, one way to do it would be: FontCascade::variantFont() and FontCascade::fontForCombiningCharacterSequence() should return information regarding if a real font or a synthesized font was returned. Then, when ComplexTextController::collectComplexTextRuns() notices that a synthesized font was returned, it should scan backward for the longest contiguous collection of fonts which match the real font, and turn them all into the the synthesized font, in addition to setting some state which will trigger any subsequent uses of the true font to be replaced as well. When ComplexTextController::collectComplexTextRuns() reaches a font which doesn't match this contiguous run, it should reset that state. In this case where FontCascade::variantFont() returns the synthesized font, this means that it should actually return two fonts - one is the font to scan back for, and the other is the replacement font. Getting this first is "free," since we always need to try the real font before trying the synthesized font. (In reply to comment #4) > Let's assume that it is not a performance regression, for now. In that case, > one way to do it would be: > > FontCascade::variantFont() and > FontCascade::fontForCombiningCharacterSequence() should return information > regarding if a real font or a synthesized font was returned. Then, when > ComplexTextController::collectComplexTextRuns() notices that a synthesized > font was returned, it should scan backward for the longest contiguous > collection of fonts which match the real font, and turn them all into the > the synthesized font, in addition to setting some state which will trigger > any subsequent uses of the true font to be replaced as well. When > ComplexTextController::collectComplexTextRuns() reaches a font which doesn't > match this contiguous run, it should reset that state. > > In this case where FontCascade::variantFont() returns the synthesized font, > this means that it should actually return two fonts - one is the font to > scan back for, and the other is the replacement font. Getting this first is > "free," since we always need to try the real font before trying the > synthesized font. Yeah, I think this approach is the best solution. If the complex code path does turn out to be a regression, this approach should also work with WidthIterator (though we would have bigger problems to get CTFontTransformGlyphs() to work .... probably) (In reply to comment #3) > True small caps requires font features, which will force us down our complex > text code path. I need to make sure this won't be a performance regression. Verified this is not a regression. Created attachment 266028 [details]
WIP
Created attachment 266034 [details]
WIP
(In reply to comment #6) > (In reply to comment #3) > > True small caps requires font features, which will force us down our complex > > text code path. I need to make sure this won't be a performance regression. > > Verified this is not a regression. This means I should remove small-caps support entire from the simple codepath Created attachment 266604 [details]
First draft
Created attachment 266711 [details]
Patch
Created attachment 266712 [details]
Patch
Created attachment 266721 [details]
Patch
Comment on attachment 266721 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=266721&action=review > Source/WebCore/platform/graphics/cocoa/FontCocoa.mm:393 > + // <rdar://problem/23770219>: Unable to remove font feature from web font. Until that radar is fixed, re-use the small-caps font for synthesis. Instead of removing the feature, I should just explicitly set it to "off" Comment on attachment 266721 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=266721&action=review >> Source/WebCore/platform/graphics/cocoa/FontCocoa.mm:393 >> + // <rdar://problem/23770219>: Unable to remove font feature from web font. Until that radar is fixed, re-use the small-caps font for synthesis. > > Instead of removing the feature, I should just explicitly set it to "off" kDefaultLowerCaseSelector Created attachment 266849 [details]
Patch
Comment on attachment 266849 [details] Patch Attachment 266849 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/531737 Number of test failures exceeded the failure limit. Created attachment 266853 [details]
Archive of layout-test-results from ews103 for mac-yosemite
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 266849 [details] Patch Attachment 266849 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/531740 Number of test failures exceeded the failure limit. Created attachment 266854 [details]
Archive of layout-test-results from ews107 for mac-yosemite-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
Comment on attachment 266849 [details] Patch Attachment 266849 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/531727 Number of test failures exceeded the failure limit. Created attachment 266855 [details]
Archive of layout-test-results from ews115 for mac-yosemite
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115 Port: mac-yosemite Platform: Mac OS X 10.10.5
Created attachment 266859 [details]
Patch
Comment on attachment 266859 [details] Patch Attachment 266859 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/531892 New failing tests: css3/font-variant-petite-caps-synthesis.html Created attachment 266863 [details]
Archive of layout-test-results from ews102 for mac-yosemite
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 266859 [details] Patch Attachment 266859 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/531928 New failing tests: css3/font-variant-petite-caps-synthesis.html Created attachment 266865 [details]
Archive of layout-test-results from ews106 for mac-yosemite-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
Comment on attachment 266859 [details] Patch Attachment 266859 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/531929 New failing tests: css3/font-variant-petite-caps-synthesis.html Created attachment 266867 [details]
Archive of layout-test-results from ews115 for mac-yosemite
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115 Port: mac-yosemite Platform: Mac OS X 10.10.5
r=me Created attachment 266961 [details]
Patch for committing
Created attachment 266962 [details]
Patch for committing
Created attachment 267076 [details]
Patch for committing
Comment on attachment 267076 [details] Patch for committing Clearing flags on attachment: 267076 Committed r193894: <http://trac.webkit.org/changeset/193894> *** Bug 4355 has been marked as a duplicate of this bug. *** The tests added with this change are failing on Windows: <https://build.webkit.org/builders/Apple%20Win%207%20Release%20%28Tests%29/builds/55487> (In reply to comment #36) > The tests added with this change are failing on Windows: > > <https://build.webkit.org/builders/Apple%20Win%207%20Release%20%28Tests%29/ > builds/55487> These are expected to fail. Do you think you could mark them as such? (There should be other similar css3/font-variant-* tests that are already marked as failing on Windows.) Done in r193914. Should this bug be closed? |