[iOS] Fonts get too bold when the "Bold Text" accessibility setting is enabled
<rdar://problem/31608236>
Created attachment 311545 [details] Needs test
Created attachment 311567 [details] WIP
Created attachment 311573 [details] WIP
Created attachment 311576 [details] Patch
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
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
Created attachment 311603 [details] Patch
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
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
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.
Created attachment 311786 [details] Patch
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.
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.
Created attachment 311850 [details] Patch
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.
Created attachment 311852 [details] Patch
Created attachment 311854 [details] Patch
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?
Created attachment 311856 [details] Patch
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.
Created attachment 311857 [details] Patch for committing
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"
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
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
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
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
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
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
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
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
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
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
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
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
Created attachment 311879 [details] Patch for committing
Comment on attachment 311879 [details] Patch for committing Clearing flags on attachment: 311879 Committed r217742: <http://trac.webkit.org/changeset/217742>
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.
(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. 👍