Bug 155004 – Whitespace causes font-variant: all-small-caps to synthesize

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Myles C. Maxfield]

Whitespace causes font-variant: all-small-caps to synthesize

[Myles C. Maxfield]

Created attachment 272820 [details]
Patch

[Build Bot]

Comment on attachment 272820 [details]
Patch

Attachment 272820 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/920110

New failing tests:
fast/text/all-small-caps-whitespace.html

[Build Bot]

Created attachment 272823 [details]
Archive of layout-test-results from ews101 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 272820 [details]
Patch

Attachment 272820 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/920135

New failing tests:
fast/text/all-small-caps-whitespace.html

[Build Bot]

Created attachment 272824 [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 272820 [details]
Patch

Attachment 272820 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/920185

New failing tests:
fast/text/all-small-caps-whitespace.html

[Build Bot]

Created attachment 272825 [details]
Archive of layout-test-results from ews116 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Myles C. Maxfield]

<rdar://problem/24630796>

[Myles C. Maxfield]

Created attachment 272828 [details]
Patch

[Darin Adler]

Comment on attachment 272828 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=272828&action=review

> Source/WebCore/platform/graphics/mac/ComplexTextController.cpp:309
> +    bool result = nextFont && nextFont != Font::systemFallback();

I suggest using various early return false statements instead of the bool and all the &&-ing.

> Source/WebCore/platform/graphics/mac/ComplexTextController.cpp:311
> +        result &= !u_isUWhiteSpace(baseCharacter);

Is this really the check we want? I know, for example, that it’s not a particularly fast function. Since what we are really doing is working around a bug in a font’s coverage data, it might be more efficient to just check for the space character, or maybe isASCIISpace. Using u_isUWhiteSpace seems both too broad and too narrow.

I’m not sure I understand why whitespace is special. Maybe all uncased characters are special?

[Myles C. Maxfield]

Comment on attachment 272828 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=272828&action=review

>> Source/WebCore/platform/graphics/mac/ComplexTextController.cpp:311
>> +        result &= !u_isUWhiteSpace(baseCharacter);
> 
> Is this really the check we want? I know, for example, that it’s not a particularly fast function. Since what we are really doing is working around a bug in a font’s coverage data, it might be more efficient to just check for the space character, or maybe isASCIISpace. Using u_isUWhiteSpace seems both too broad and too narrow.
> 
> I’m not sure I understand why whitespace is special. Maybe all uncased characters are special?

The difference is that smcp/c2sc does cover uncased characters (and it needs to in order for the uncased character not to look out of place). It doesn't, however, need to cover whitespace (presumably because, since there is no outline data, shrinking it would be a no-op)

[Myles C. Maxfield]

Comment on attachment 272828 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=272828&action=review

>>> Source/WebCore/platform/graphics/mac/ComplexTextController.cpp:311
>>> +        result &= !u_isUWhiteSpace(baseCharacter);
>> 
>> Is this really the check we want? I know, for example, that it’s not a particularly fast function. Since what we are really doing is working around a bug in a font’s coverage data, it might be more efficient to just check for the space character, or maybe isASCIISpace. Using u_isUWhiteSpace seems both too broad and too narrow.
>> 
>> I’m not sure I understand why whitespace is special. Maybe all uncased characters are special?
> 
> The difference is that smcp/c2sc does cover uncased characters (and it needs to in order for the uncased character not to look out of place). It doesn't, however, need to cover whitespace (presumably because, since there is no outline data, shrinking it would be a no-op)

It still should, though, because it isn't really a no-op - the glyph's advance should change too.

[Myles C. Maxfield]

Committed r197573: <http://trac.webkit.org/changeset/197573>