Bug 197528 – font-optical-sizing applies the wrong variation value

• Component: New Bugs
• Status: RESOLVED FIXED

[Myles C. Maxfield]

font-optical-sizing applies the wrong variation value

[Myles C. Maxfield]

Created attachment 368815 [details]
Needs version check

[EWS Watchlist]

Attachment 368815 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:17:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Myles C. Maxfield]

<rdar://problem/50152854>

[Myles C. Maxfield]

I probably need to make a font to test this

[Myles C. Maxfield]

We also might want to consider whether the font is system ui or not

[EWS Watchlist]

Comment on attachment 368815 [details]
Needs version check

Attachment 368815 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/12064977

New failing tests:
fast/text/justify-ideograph-complex.html

[EWS Watchlist]

Created attachment 368820 [details]
Archive of layout-test-results from ews100 for mac-highsierra

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

[EWS Watchlist]

Comment on attachment 368815 [details]
Needs version check

Attachment 368815 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/12065111

New failing tests:
fast/text/justify-ideograph-complex.html

[EWS Watchlist]

Created attachment 368823 [details]
Archive of layout-test-results from ews105 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 368815 [details]
Needs version check

Attachment 368815 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/12065220

New failing tests:
fast/text/justify-ideograph-complex.html

[EWS Watchlist]

Created attachment 368826 [details]
Archive of layout-test-results from ews117 for mac-highsierra

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

[EWS Watchlist]

Comment on attachment 368815 [details]
Needs version check

Attachment 368815 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/12065267

New failing tests:
platform/ios/ios/fast/text/opticalFontWithWeight.html
platform/ios/ios/fast/text/opticalFontWithTextStyle.html
fast/forms/auto-fill-button/input-strong-password-auto-fill-button.html

[EWS Watchlist]

Created attachment 368827 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.14.4

[EWS Watchlist]

Comment on attachment 368815 [details]
Needs version check

Attachment 368815 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/12067490

New failing tests:
security/contentSecurityPolicy/video-with-file-url-allowed-by-media-src-star.html
legacy-animation-engine/fast/layers/no-clipping-overflow-hidden-hardware-acceleration.html

[EWS Watchlist]

Created attachment 368845 [details]
Archive of layout-test-results from ews213 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews213  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit

[Myles C. Maxfield]

Created attachment 370156 [details]
WIP

[Myles C. Maxfield]

Created attachment 370173 [details]
Patch

[EWS Watchlist]

Comment on attachment 370173 [details]
Patch

Attachment 370173 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/12221141

New failing tests:
fast/text/variations/case-axis-names.html
fast/text/variations/font-selection-properties.html
fast/text/variations/gx-width.html
fast/text/variations/font-face-clamp.html
fast/text/justify-ideograph-complex.html
fast/text/optical-sizing-trak-2.html

[EWS Watchlist]

Created attachment 370176 [details]
Archive of layout-test-results from ews103 for mac-highsierra

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

[EWS Watchlist]

Comment on attachment 370173 [details]
Patch

Attachment 370173 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/12221189

New failing tests:
fast/text/variations/case-axis-names.html
fast/text/variations/font-selection-properties.html
fast/text/variations/gx-width.html
fast/text/variations/font-face-clamp.html
fast/text/justify-ideograph-complex.html
fast/text/optical-sizing-trak-2.html

[EWS Watchlist]

Created attachment 370178 [details]
Archive of layout-test-results from ews104 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 370173 [details]
Patch

Attachment 370173 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/12221231

New failing tests:
fast/text/variations/case-axis-names.html
fast/text/variations/font-selection-properties.html
fast/text/variations/gx-width.html
fast/text/variations/font-face-clamp.html
fast/text/justify-ideograph-complex.html
fast/text/optical-sizing-trak-2.html

[EWS Watchlist]

Created attachment 370179 [details]
Archive of layout-test-results from ews115 for mac-highsierra

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

[Daniel Bates]

Comment on attachment 370173 [details]
Patch

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

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:513
> +            case 'trak':

Ok as is. Wow. Wow. Wow. A multi character constant. I have never run across this in practice until now... its impl defined. Clever. Very clever. I actually don't know which I prefer the cleverness or using the k-type constant. Kinda leaning towards the later if I were to ever touch this function for source compatibility. Likely never will or could be changed though :/ could in theory be made safe for all compilers though :/

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:514
> +                foundTrak = true;

Ok as is. Missing a break.

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:529
> +    enum class TrackingType {

Ok as is. Missing a width. In my opinion, looks prettier in one line.

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:532
> +        Manual

Ok as-is. Missing comma for future proofing.

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:555
>      const auto& variations = fontDescription.variationSettings();

^^^^ remove the unused_param for size above this line.

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:558
> +    FontType fontType(originalFont);

Ok as is. Uniform initializer syntax is prettier in my opinion.

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:560
> +    bool forceOpticalSizingOn = fontOpticalSizing == FontOpticalSizing::Enabled && (fontType.variationType == FontType::VariationType::TrueTypeGX && defaultValues.contains({{'o', 'p', 's', 'z'}}));

Ok as is. Parens not needed. All you have are &&s

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:563
> +    if (!originalFont || (!features.size() && variations.isEmpty() && textRenderingMode == TextRenderingMode::AutoTextRendering && variantSettings.isAllNormal() && fontOpticalSizing == FontOpticalSizing::Enabled && !forceOpticalSizingOn

Ok as is. No, features.isEmpty? I would add one in my opinion.

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:564
>          && (!fontFaceFeatures || !fontFaceFeatures->size()) && (!fontFaceVariantSettings || fontFaceVariantSettings->isAllNormal())))

Ok as is. Again, no isEmpty()?

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:565
>          return originalFont;

^^^^ in my opinion if condition could be simplified and made more readable by extracting into local vars.

[Daniel Bates]

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

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:513
> +            case 'trak':

Ok as is. Wow. Wow. Wow. A multi character constant. I have never run across this in practice until now... its impl defined. Clever. Very clever. I actually don't know which I prefer the cleverness or using the k-type constant. Kinda leaning towards the later if I were to ever touch this function for source compatibility. Likely never will or could be changed though :/ could in theory be made safe for all compilers though :/

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:514
> +                foundTrak = true;

Ok as is. Missing a break.

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:529
> +    enum class TrackingType {

Ok as is. Missing a width. In my opinion, looks prettier in one line.

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:532
> +        Manual

Ok as-is. Missing comma for future proofing.

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:555
>      const auto& variations = fontDescription.variationSettings();

^^^^ remove the unused_param for size above this line.

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:558
> +    FontType fontType(originalFont);

Ok as is. Uniform initializer syntax is prettier in my opinion.

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:560
> +    bool forceOpticalSizingOn = fontOpticalSizing == FontOpticalSizing::Enabled && (fontType.variationType == FontType::VariationType::TrueTypeGX && defaultValues.contains({{'o', 'p', 's', 'z'}}));

Ok as is. Parens not needed. All you have are &&s

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:563
> +    if (!originalFont || (!features.size() && variations.isEmpty() && textRenderingMode == TextRenderingMode::AutoTextRendering && variantSettings.isAllNormal() && fontOpticalSizing == FontOpticalSizing::Enabled && !forceOpticalSizingOn

Ok as is. No, features.isEmpty? I would add one in my opinion.

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:564
>          && (!fontFaceFeatures || !fontFaceFeatures->size()) && (!fontFaceVariantSettings || fontFaceVariantSettings->isAllNormal())))

Ok as is. Again, no isEmpty()?

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:565
>          return originalFont;

^^^^ in my opinion if condition could be simplified and made more readable by extracting into local vars.

[Maciej Stachowiak]

Looks like the new tests are all failing on High Sierra.

[Myles C. Maxfield]

Created attachment 370255 [details]
Patch

[EWS Watchlist]

Comment on attachment 370255 [details]
Patch

Attachment 370255 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/12237507

New failing tests:
fast/text/optical-sizing-units-2.html
fast/text/optical-sizing-trak-2.html

[EWS Watchlist]

Created attachment 370262 [details]
Archive of layout-test-results from ews215 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews215  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit

[EWS Watchlist]

Comment on attachment 370255 [details]
Patch

Attachment 370255 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/12237448

New failing tests:
fast/text/justify-ideograph-complex.html
fast/text/optical-sizing-trak-2.html

[EWS Watchlist]

Created attachment 370263 [details]
Archive of layout-test-results from ews105 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 370255 [details]
Patch

Attachment 370255 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/12237569

New failing tests:
fast/text/justify-ideograph-complex.html
fast/text/optical-sizing-trak-2.html

[EWS Watchlist]

Created attachment 370266 [details]
Archive of layout-test-results from ews117 for mac-highsierra

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

[EWS Watchlist]

Comment on attachment 370255 [details]
Patch

Attachment 370255 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/12238574

New failing tests:
fast/text/justify-ideograph-complex.html
fast/text/optical-sizing-trak-2.html

[EWS Watchlist]

Created attachment 370269 [details]
Archive of layout-test-results from ews101 for mac-highsierra

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

[Myles C. Maxfield]

Created attachment 370281 [details]
Patch

[Myles C. Maxfield]

Created attachment 370282 [details]
Patch

[EWS Watchlist]

Comment on attachment 370282 [details]
Patch

Attachment 370282 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/12240975

New failing tests:
webaudio/silent-audio-interrupted-in-background.html

[EWS Watchlist]

Created attachment 370291 [details]
Archive of layout-test-results from ews114 for mac-highsierra

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

[Myles C. Maxfield]

Test failure is unrelated.

[Antti Koivisto]

Comment on attachment 370282 [details]
Patch

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

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:557
> +    if (!originalFont || (features.isEmpty() && !forceVariations && variations.isEmpty() && textRenderingMode == TextRenderingMode::AutoTextRendering && variantSettings.isAllNormal() && fontOpticalSizing == FontOpticalSizing::Enabled && !forceOpticalSizingOn
> +        && (!fontFaceFeatures || fontFaceFeatures->isEmpty()) && (!fontFaceVariantSettings || fontFaceVariantSettings->isAllNormal())))

The test would read better with some sensibly named helper booleans, or as a lambda function.

[Antti Koivisto]

Comment on attachment 370282 [details]
Patch

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

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:527
> +    enum class TrackingType : uint8_t { None, Automatic, Manual, };
> +    TrackingType trackingType { TrackingType::None };

We have this new code to compute trackingType but I don't see trackingType actually being used anywhere? Can you just remove the code and simplify the patch?

[Myles C. Maxfield]

Committed r245598: <https://trac.webkit.org/changeset/245598>

[Ryan Haddad]

Reverted r245598 for reason:

Breaks watchOS build.

Committed r245647: <https://trac.webkit.org/changeset/245647>

[Myles C. Maxfield]

Created attachment 370470 [details]
Patch for committing

[Myles C. Maxfield]

This latest patch includes the fix from https://bugs.webkit.org/show_bug.cgi?id=198099

[Myles C. Maxfield]

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

[Myles C. Maxfield]

Committed r245672: <https://trac.webkit.org/changeset/245672>