Bug 172737 – REGRESSION(r213464): [iOS] Fonts get too bold when the "Bold Text" accessibility setting is enabled

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

[Myles C. Maxfield]

[iOS] Fonts get too bold when the "Bold Text" accessibility setting is enabled

[Myles C. Maxfield]

<rdar://problem/31608236>

[Myles C. Maxfield]

Created attachment 311545 [details]
Needs test

[Myles C. Maxfield]

Created attachment 311567 [details]
WIP

[Myles C. Maxfield]

Created attachment 311573 [details]
WIP

[Myles C. Maxfield]

Created attachment 311576 [details]
Patch

[Build Bot]

Comment on attachment 311576 [details]
Patch

Attachment 311576 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3846821

New failing tests:
fast/text/accessibility-bold-system-font-2.html

[Build Bot]

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

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

[Myles C. Maxfield]

Created attachment 311603 [details]
Patch

[Build Bot]

Comment on attachment 311603 [details]
Patch

Attachment 311603 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3849301

New failing tests:
fast/text/accessibility-bold-system-font-2.html

[Build Bot]

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

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

[Myles C. Maxfield]

Created attachment 311780 [details]
Patch

Comment on attachment 311780 [details]
Patch

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

> Source/WebKit2/Configurations/BaseTarget.xcconfig:116
> +ACCESSIBILITY_LDFLAGS[sdk=iphone*] = -lAccessibility;
> +ACCESSIBILITY_LDFLAGS[sdk=macosx*] = ;

Please prefix new custom build settings’ names with WK_

It would be better to make the first definition unconditional, so that it applies to all of PLATFORM(IOS) and not just TARGET_OS_IOS.

[Myles C. Maxfield]

Created attachment 311786 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 311786 [details]
Patch

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

> Source/WebCore/rendering/RenderThemeIOS.mm:1234
> +    if (result < -0.6)
> +        return FontSelectionValue(100);
> +    if (result < -0.365)
> +        return FontSelectionValue(200);
> +    if (result < -0.115)
> +        return FontSelectionValue(300);
> +    if (result <  0.130)
> +        return FontSelectionValue(400);
> +    if (result <  0.235)
> +        return FontSelectionValue(500);
> +    if (result <  0.350)
> +        return FontSelectionValue(600);
> +    if (result <  0.500)
> +        return FontSelectionValue(700);
> +    if (result <  0.700)
> +        return FontSelectionValue(800);

I feel like this could be a const data table and a for loop.

> Source/WebKit2/Shared/EntryPointUtilities/mac/XPCService/XPCServiceMain.mm:156
> +            _AXSSetEnhanceTextLegibilityEnabled(TRUE);
> +            atexit(&WebKit::unBoldSystemFontForAccessibility);

This part is gross and I think we should try to avoid it.

[Sam Weinig]

Comment on attachment 311786 [details]
Patch

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

>> Source/WebKit2/Shared/EntryPointUtilities/mac/XPCService/XPCServiceMain.mm:156
>> +            atexit(&WebKit::unBoldSystemFontForAccessibility);
> 
> This part is gross and I think we should try to avoid it.

Agreed, I think we need an alternate solution here. Why is this atexit() needed? Your changelog did not say.

[Myles C. Maxfield]

Created attachment 311850 [details]
Patch

[Build Bot]

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


ERROR: LayoutTests/platform/mac/TestExpectations:1548:  Path does not exist.  [test/expectations] [5]
Total errors found: 1 in 11 files


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

[Myles C. Maxfield]

Created attachment 311852 [details]
Patch

[Myles C. Maxfield]

Created attachment 311854 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 311854 [details]
Patch

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

> Source/WebCore/platform/graphics/FontCache.h:230
> +    bool shouldMockBoldSystemFontForAccessibility() { return m_shouldMockBoldSystemFontForAccessibility; }

const

> Source/WebCore/rendering/RenderTheme.h:256
> +    virtual bool shouldMockBoldSystemFontForAccessibility() { return false; }

const

> Source/WebCore/rendering/RenderThemeIOS.h:121
> +    bool shouldMockBoldSystemFontForAccessibility() override { return m_shouldMockBoldSystemFontForAccessibility; }

const

> Source/WebCore/rendering/RenderThemeIOS.mm:1235
> +    if (result < -0.6)
> +        return FontSelectionValue(100);
> +    if (result < -0.365)
> +        return FontSelectionValue(200);
> +    if (result < -0.115)
> +        return FontSelectionValue(300);
> +    if (result <  0.130)
> +        return FontSelectionValue(400);
> +    if (result <  0.235)
> +        return FontSelectionValue(500);
> +    if (result <  0.350)
> +        return FontSelectionValue(600);
> +    if (result <  0.500)
> +        return FontSelectionValue(700);
> +    if (result <  0.700)
> +        return FontSelectionValue(800);
> +    return FontSelectionValue(900);

You didn't like the const data suggestion?

[Myles C. Maxfield]

Created attachment 311856 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 311856 [details]
Patch

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

> Source/WebCore/rendering/RenderThemeIOS.mm:1219
> +    static float weightThresholds[] = { -0.6, -0.365, -0.115, 0.130, 0.235, 0.350, 0.5, 0.7 };

I'd like to see a comment saying where these magic numbers come from.

> Source/WebCore/testing/InternalSettings.cpp:690
> +    RenderTheme::singleton().setShouldMockBoldSystemFontForAccessibility(requires);
> +    FontCache::singleton().setShouldMockBoldSystemFontForAccessibility(requires);

Feels slightly weird to stash this state in two places.

[Myles C. Maxfield]

Created attachment 311857 [details]
Patch for committing

[Simon Fraser (smfr)]

Comment on attachment 311857 [details]
Patch for committing

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

> Source/WebCore/rendering/RenderThemeIOS.mm:1219
> +    // These numbers were experimenatlly gathered from weights of the system font.

"experimenatlly"

[Build Bot]

Comment on attachment 311856 [details]
Patch

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

New failing tests:
fast/text/accessibility-bold-system-font-2.html

[Build Bot]

Created attachment 311859 [details]
Archive of layout-test-results from ews101 for mac-elcapitan

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

[Build Bot]

Comment on attachment 311856 [details]
Patch

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

New failing tests:
fast/text/accessibility-bold-system-font-2.html

[Build Bot]

Created attachment 311861 [details]
Archive of layout-test-results from ews106 for mac-elcapitan-wk2

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

[Build Bot]

Comment on attachment 311856 [details]
Patch

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

New failing tests:
fast/text/accessibility-bold-system-font-2.html

[Build Bot]

Created attachment 311864 [details]
Archive of layout-test-results from ews113 for mac-elcapitan

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

[Build Bot]

Comment on attachment 311857 [details]
Patch for committing

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

New failing tests:
fast/text/accessibility-bold-system-font-2.html

[Build Bot]

Created attachment 311868 [details]
Archive of layout-test-results from ews100 for mac-elcapitan

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

[Build Bot]

Comment on attachment 311857 [details]
Patch for committing

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

New failing tests:
fast/text/accessibility-bold-system-font-2.html

[Build Bot]

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

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

[Build Bot]

Comment on attachment 311857 [details]
Patch for committing

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

New failing tests:
fast/text/accessibility-bold-system-font-2.html

[Build Bot]

Created attachment 311872 [details]
Archive of layout-test-results from ews114 for mac-elcapitan

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

[Myles C. Maxfield]

Created attachment 311879 [details]
Patch for committing

[WebKit Commit Bot]

Comment on attachment 311879 [details]
Patch for committing

Clearing flags on attachment: 311879

Committed r217742: <http://trac.webkit.org/changeset/217742>

[Michael Catanzaro]

Since this test is only intended to work on iOS, I'm going to skip it in the global TestExpectations and mark it as passing in the iOS expectations instead.

[Myles C. Maxfield]

(In reply to Michael Catanzaro from comment #39)
> Since this test is only intended to work on iOS, I'm going to skip it in the
> global TestExpectations and mark it as passing in the iOS expectations
> instead.

👍