Bug 197528 - font-optical-sizing applies the wrong variation value
Summary: font-optical-sizing applies the wrong variation value
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Myles C. Maxfield
URL:
Keywords: InRadar
: 198099 (view as bug list)
Depends on:
Blocks:
 
Reported: 2019-05-02 13:35 PDT by Myles C. Maxfield
Modified: 2019-05-22 21:18 PDT (History)
10 users (show)

See Also:


Attachments
Needs version check (10.58 KB, patch)
2019-05-02 13:38 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews100 for mac-highsierra (3.71 MB, application/zip)
2019-05-02 14:21 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews105 for mac-highsierra-wk2 (3.36 MB, application/zip)
2019-05-02 14:34 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews117 for mac-highsierra (3.41 MB, application/zip)
2019-05-02 15:09 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews126 for ios-simulator-wk2 (3.25 MB, application/zip)
2019-05-02 15:23 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews213 for win-future (13.72 MB, application/zip)
2019-05-02 16:46 PDT, Build Bot
no flags Details
WIP (23.43 KB, patch)
2019-05-17 15:09 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (316.57 KB, patch)
2019-05-17 18:17 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-highsierra (3.67 MB, application/zip)
2019-05-17 19:23 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews104 for mac-highsierra-wk2 (3.21 MB, application/zip)
2019-05-17 19:36 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews115 for mac-highsierra (3.57 MB, application/zip)
2019-05-17 20:09 PDT, Build Bot
no flags Details
Patch (317.34 KB, patch)
2019-05-20 09:33 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews215 for win-future (13.30 MB, application/zip)
2019-05-20 10:48 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews105 for mac-highsierra-wk2 (3.12 MB, application/zip)
2019-05-20 10:51 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews117 for mac-highsierra (3.44 MB, application/zip)
2019-05-20 11:24 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews101 for mac-highsierra (3.53 MB, application/zip)
2019-05-20 13:12 PDT, Build Bot
no flags Details
Patch (322.28 KB, patch)
2019-05-20 16:54 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (322.35 KB, patch)
2019-05-20 16:55 PDT, Myles C. Maxfield
koivisto: review+
Details | Formatted Diff | Diff
Archive of layout-test-results from ews114 for mac-highsierra (2.96 MB, application/zip)
2019-05-20 19:04 PDT, Build Bot
no flags Details
Patch for committing (323.74 KB, patch)
2019-05-22 17:15 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Myles C. Maxfield 2019-05-02 13:35:17 PDT
font-optical-sizing applies the wrong variation value
Comment 1 Myles C. Maxfield 2019-05-02 13:38:38 PDT
Created attachment 368815 [details]
Needs version check
Comment 2 Build Bot 2019-05-02 13:40:07 PDT
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.
Comment 3 Myles C. Maxfield 2019-05-02 13:45:34 PDT
<rdar://problem/50152854>
Comment 4 Myles C. Maxfield 2019-05-02 13:47:29 PDT
I probably need to make a font to test this
Comment 5 Myles C. Maxfield 2019-05-02 14:01:59 PDT
We also might want to consider whether the font is system ui or not
Comment 6 Build Bot 2019-05-02 14:21:15 PDT
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
Comment 7 Build Bot 2019-05-02 14:21:17 PDT
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
Comment 8 Build Bot 2019-05-02 14:34:42 PDT
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
Comment 9 Build Bot 2019-05-02 14:34:43 PDT
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
Comment 10 Build Bot 2019-05-02 15:09:55 PDT
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
Comment 11 Build Bot 2019-05-02 15:09:57 PDT
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
Comment 12 Build Bot 2019-05-02 15:23:23 PDT
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
Comment 13 Build Bot 2019-05-02 15:23:25 PDT
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
Comment 14 Build Bot 2019-05-02 16:46:06 PDT
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
Comment 15 Build Bot 2019-05-02 16:46:09 PDT
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
Comment 16 Myles C. Maxfield 2019-05-17 15:09:19 PDT
Created attachment 370156 [details]
WIP
Comment 17 Myles C. Maxfield 2019-05-17 18:17:47 PDT
Created attachment 370173 [details]
Patch
Comment 18 Build Bot 2019-05-17 19:23:40 PDT
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
Comment 19 Build Bot 2019-05-17 19:23:42 PDT
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
Comment 20 Build Bot 2019-05-17 19:36:39 PDT
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
Comment 21 Build Bot 2019-05-17 19:36:40 PDT
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
Comment 22 Build Bot 2019-05-17 20:09:34 PDT
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
Comment 23 Build Bot 2019-05-17 20:09:36 PDT
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
Comment 24 Daniel Bates 2019-05-17 20:24:15 PDT
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.
Comment 25 Daniel Bates 2019-05-17 20:24:18 PDT
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.
Comment 26 Maciej Stachowiak 2019-05-19 15:30:38 PDT
Looks like the new tests are all failing on High Sierra.
Comment 27 Myles C. Maxfield 2019-05-20 09:33:29 PDT
Created attachment 370255 [details]
Patch
Comment 28 Build Bot 2019-05-20 10:48:04 PDT
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
Comment 29 Build Bot 2019-05-20 10:48:07 PDT
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
Comment 30 Build Bot 2019-05-20 10:51:07 PDT
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
Comment 31 Build Bot 2019-05-20 10:51:09 PDT
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
Comment 32 Build Bot 2019-05-20 11:24:26 PDT
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
Comment 33 Build Bot 2019-05-20 11:24:28 PDT
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
Comment 34 Build Bot 2019-05-20 13:12:35 PDT
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
Comment 35 Build Bot 2019-05-20 13:12:37 PDT
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
Comment 36 Myles C. Maxfield 2019-05-20 16:54:21 PDT
Created attachment 370281 [details]
Patch
Comment 37 Myles C. Maxfield 2019-05-20 16:55:41 PDT
Created attachment 370282 [details]
Patch
Comment 38 Build Bot 2019-05-20 19:04:14 PDT
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
Comment 39 Build Bot 2019-05-20 19:04:16 PDT
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
Comment 40 Myles C. Maxfield 2019-05-20 22:43:39 PDT
Test failure is unrelated.
Comment 41 Antti Koivisto 2019-05-21 13:02:22 PDT
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.
Comment 42 Antti Koivisto 2019-05-21 13:36:55 PDT
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?
Comment 43 Myles C. Maxfield 2019-05-21 16:20:54 PDT
Committed r245598: <https://trac.webkit.org/changeset/245598>
Comment 44 Ryan Haddad 2019-05-22 14:39:21 PDT
Reverted r245598 for reason:

Breaks watchOS build.

Committed r245647: <https://trac.webkit.org/changeset/245647>
Comment 45 Myles C. Maxfield 2019-05-22 17:15:47 PDT
Created attachment 370470 [details]
Patch for committing
Comment 46 Myles C. Maxfield 2019-05-22 21:16:36 PDT
This latest patch includes the fix from https://bugs.webkit.org/show_bug.cgi?id=198099
Comment 47 Myles C. Maxfield 2019-05-22 21:18:20 PDT
*** Bug 198099 has been marked as a duplicate of this bug. ***
Comment 48 Myles C. Maxfield 2019-05-22 21:18:59 PDT
Committed r245672: <https://trac.webkit.org/changeset/245672>