Bug 149774 – font-variant-caps does not work if the font does not support font features

• Component: Text
• Status: RESOLVED FIXED

[Myles C. Maxfield]

We need to implement synthesis in this case.

[Radar WebKit Bug Importer]

<rdar://problem/22959746>

[Myles C. Maxfield]

I'm going to break this into two bugs - one for font-variant-position and one for font-variant-caps.

[Myles C. Maxfield]

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.

[Myles C. Maxfield]

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.

[Myles C. Maxfield]

(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)

[Myles C. Maxfield]

(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.

[Myles C. Maxfield]

Created attachment 266028 [details]
WIP

[Myles C. Maxfield]

Created attachment 266034 [details]
WIP

[Myles C. Maxfield]

(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

[Myles C. Maxfield]

Created attachment 266604 [details]
First draft

[Myles C. Maxfield]

Created attachment 266711 [details]
Patch

[Myles C. Maxfield]

Created attachment 266712 [details]
Patch

[Myles C. Maxfield]

Created attachment 266721 [details]
Patch

[Myles C. Maxfield]

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"

[Myles C. Maxfield]

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

[Myles C. Maxfield]

Created attachment 266849 [details]
Patch

[Build Bot]

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.

[Build Bot]

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

[Build Bot]

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.

[Build Bot]

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

[Build Bot]

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.

[Build Bot]

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

[Myles C. Maxfield]

Created attachment 266859 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Antti Koivisto]

r=me

[Myles C. Maxfield]

Created attachment 266961 [details]
Patch for committing

[Myles C. Maxfield]

Created attachment 266962 [details]
Patch for committing

[Myles C. Maxfield]

Created attachment 267076 [details]
Patch for committing

[WebKit Commit Bot]

Comment on attachment 267076 [details]
Patch for committing

Clearing flags on attachment: 267076

Committed r193894: <http://trac.webkit.org/changeset/193894>

[Myles C. Maxfield]

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

[Ryan Haddad]

The tests added with this change are failing on Windows:

<https://build.webkit.org/builders/Apple%20Win%207%20Release%20%28Tests%29/builds/55487>

[Myles C. Maxfield]

(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.)

[Ryan Haddad]

Done in r193914.

[Michael Catanzaro]

Should this bug be closed?