Skip to content

Conversation

whsieh
Copy link
Member

@whsieh whsieh commented Aug 13, 2023

04c640b

[macOS] drawFocusIfNeeded() should not expose the user's system accent color
https://bugs.webkit.org/show_bug.cgi?id=260102
rdar://105554669

Reviewed by Tim Nguyen.

On macOS, `drawFocusIfNeeded()` currently exposes the user's system accent color via
`RenderTheme::focusRingColor()`. To mitigate fingerprinting risk due to this API for users that have
chosen a non-default system accent color, we make `RenderTheme::focusRingColor()` respect the given
`UseSystemAppearance` state by returning the default system focus ring color on macOS, in the case
where that option is absent. As a result, this means that quirks-mode webpages that use
`-webkit-focus-ring-color` will also no longer be able to determine the user's accent color. This
aligns with existing behavior for the "activeborder" CSS value.

Tests:  fast/canvas/do-not-expose-non-default-focus-ring-color.html
        fast/css/mac/focus-ring-color-should-not-expose-accent-color.html

* LayoutTests/TestExpectations:
* LayoutTests/fast/canvas/do-not-expose-non-default-focus-ring-color-expected.html: Added.
* LayoutTests/fast/canvas/do-not-expose-non-default-focus-ring-color.html: Added.
* LayoutTests/fast/canvas/resources/do-not-expose-non-default-focus-ring-color.js: Added.
(paintIntoSwatch):

Add a test to verify that accent colors can't be read back using canvas 2D; to test this, we render
a simple focus ring to a 2D canvas, use `getImageData` to read it back, and verify that the average
non-transparent pixel values in the resulting image data match even when the accent color is
different (customized using the new `UIScriptController` hook below).

* LayoutTests/fast/css/mac/focus-ring-color-should-not-expose-accent-color-expected-mismatch.html: Added.
* LayoutTests/fast/css/mac/focus-ring-color-should-not-expose-accent-color.html: Added.

Add another test to verify that accent colors (1) are not directly leaked through the use of the
`-webkit-focus-ring-color` CSS property, and (2) enabling system appearance is sufficient to expose
the real focus ring color again.

* LayoutTests/platform/mac-wk2/TestExpectations:
* LayoutTests/resources/ui-helper.js:
(window.UIHelper.isMac):
(window.UIHelper.setAppAccentColor):
* Source/WebCore/PAL/pal/spi/ios/UIKitSPI.h:

Drive-by fix: remove an unnecessary UIKit SPI method declaraction.

* Source/WebCore/rendering/RenderElement.cpp:
(WebCore::RenderElement::paintFocusRing const):

Set `UseSystemAppearance` here to ensure that focus rings still paint with the correct appearance.

* Source/WebCore/rendering/RenderImage.cpp:
(WebCore::RenderImage::paintAreaElementFocusRing):

Set `UseSystemAppearance` here to ensure that focus rings still paint with the correct appearance.

* Source/WebCore/rendering/RenderThemeIOS.mm:
(WebCore::RenderThemeIOS::systemFocusRingColor):
* Source/WebCore/rendering/RenderThemeMac.mm:
(WebCore::defaultFocusRingColor):
(WebCore::RenderThemeMac::platformFocusRingColor const):

This is the main fix — pull the hard-coded value for the focus ring color out into a separate helper
function, which we use in `platformFocusRingColor` if `UseSystemAppearance` is unset.

(WebCore::RenderThemeMac::systemColor const):
* Source/WebCore/testing/Internals.cpp:
(WebCore::Internals::focusRingColor):
* Tools/TestRunnerShared/UIScriptContext/Bindings/UIScriptController.idl:
* Tools/TestRunnerShared/UIScriptContext/UIScriptController.h:
(WTR::UIScriptController::setAppAccentColor):

Add a `UIScriptController` hook to set a custom accent color, using `-_setAccentColor:`. This is
reset to the default value (computed upon initializing the test runner and stored in
`m_defaultAppAccentColor`) between test runs.

* Tools/WebKitTestRunner/InjectedBundle/Bindings/TestRunner.idl:
* Tools/WebKitTestRunner/InjectedBundle/TestRunner.h:
(WTR::TestRunner::isMac const):
* Tools/WebKitTestRunner/TestController.h:
* Tools/WebKitTestRunner/mac/TestControllerMac.mm:
(WTR::TestController::platformInitialize):
(WTR::TestController::platformResetStateToConsistentValues):
* Tools/WebKitTestRunner/mac/UIScriptControllerMac.h:
* Tools/WebKitTestRunner/mac/UIScriptControllerMac.mm:
(WTR::UIScriptControllerMac::setAppAccentColor):

Canonical link: https://commits.webkit.org/266881@main

f9bef82

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 wincairo
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug 🧪 wpe-wk2
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🛠 gtk
✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🧪 gtk-wk2
✅ 🧪 api-ios ✅ 🧪 mac-wk2 ❌ 🧪 api-gtk
✅ 🛠 tv ✅ 🧪 mac-AS-debug-wk2
✅ 🛠 tv-sim ✅ 🧪 mac-wk2-stress
✅ 🛠 🧪 merge ✅ 🛠 watch
✅ 🛠 watch-sim

@whsieh whsieh self-assigned this Aug 13, 2023
@whsieh whsieh added the Canvas Bugs related to the canvas element. label Aug 13, 2023
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Aug 13, 2023
@whsieh whsieh removed the merging-blocked Applied to prevent a change from being merged label Aug 14, 2023
@whsieh whsieh marked this pull request as ready for review August 14, 2023 14:25
@whsieh whsieh requested a review from JonWBedard as a code owner August 14, 2023 14:25
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I'd use auto instead of Color here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch — changed to auto.

Thanks for the review!

@whsieh whsieh added the merge-queue Applied to send a pull request to merge-queue label Aug 14, 2023
…t color

https://bugs.webkit.org/show_bug.cgi?id=260102
rdar://105554669

Reviewed by Tim Nguyen.

On macOS, `drawFocusIfNeeded()` currently exposes the user's system accent color via
`RenderTheme::focusRingColor()`. To mitigate fingerprinting risk due to this API for users that have
chosen a non-default system accent color, we make `RenderTheme::focusRingColor()` respect the given
`UseSystemAppearance` state by returning the default system focus ring color on macOS, in the case
where that option is absent. As a result, this means that quirks-mode webpages that use
`-webkit-focus-ring-color` will also no longer be able to determine the user's accent color. This
aligns with existing behavior for the "activeborder" CSS value.

Tests:  fast/canvas/do-not-expose-non-default-focus-ring-color.html
        fast/css/mac/focus-ring-color-should-not-expose-accent-color.html

* LayoutTests/TestExpectations:
* LayoutTests/fast/canvas/do-not-expose-non-default-focus-ring-color-expected.html: Added.
* LayoutTests/fast/canvas/do-not-expose-non-default-focus-ring-color.html: Added.
* LayoutTests/fast/canvas/resources/do-not-expose-non-default-focus-ring-color.js: Added.
(paintIntoSwatch):

Add a test to verify that accent colors can't be read back using canvas 2D; to test this, we render
a simple focus ring to a 2D canvas, use `getImageData` to read it back, and verify that the average
non-transparent pixel values in the resulting image data match even when the accent color is
different (customized using the new `UIScriptController` hook below).

* LayoutTests/fast/css/mac/focus-ring-color-should-not-expose-accent-color-expected-mismatch.html: Added.
* LayoutTests/fast/css/mac/focus-ring-color-should-not-expose-accent-color.html: Added.

Add another test to verify that accent colors (1) are not directly leaked through the use of the
`-webkit-focus-ring-color` CSS property, and (2) enabling system appearance is sufficient to expose
the real focus ring color again.

* LayoutTests/platform/mac-wk2/TestExpectations:
* LayoutTests/resources/ui-helper.js:
(window.UIHelper.isMac):
(window.UIHelper.setAppAccentColor):
* Source/WebCore/PAL/pal/spi/ios/UIKitSPI.h:

Drive-by fix: remove an unnecessary UIKit SPI method declaraction.

* Source/WebCore/rendering/RenderElement.cpp:
(WebCore::RenderElement::paintFocusRing const):

Set `UseSystemAppearance` here to ensure that focus rings still paint with the correct appearance.

* Source/WebCore/rendering/RenderImage.cpp:
(WebCore::RenderImage::paintAreaElementFocusRing):

Set `UseSystemAppearance` here to ensure that focus rings still paint with the correct appearance.

* Source/WebCore/rendering/RenderThemeIOS.mm:
(WebCore::RenderThemeIOS::systemFocusRingColor):
* Source/WebCore/rendering/RenderThemeMac.mm:
(WebCore::defaultFocusRingColor):
(WebCore::RenderThemeMac::platformFocusRingColor const):

This is the main fix — pull the hard-coded value for the focus ring color out into a separate helper
function, which we use in `platformFocusRingColor` if `UseSystemAppearance` is unset.

(WebCore::RenderThemeMac::systemColor const):
* Source/WebCore/testing/Internals.cpp:
(WebCore::Internals::focusRingColor):
* Tools/TestRunnerShared/UIScriptContext/Bindings/UIScriptController.idl:
* Tools/TestRunnerShared/UIScriptContext/UIScriptController.h:
(WTR::UIScriptController::setAppAccentColor):

Add a `UIScriptController` hook to set a custom accent color, using `-_setAccentColor:`. This is
reset to the default value (computed upon initializing the test runner and stored in
`m_defaultAppAccentColor`) between test runs.

* Tools/WebKitTestRunner/InjectedBundle/Bindings/TestRunner.idl:
* Tools/WebKitTestRunner/InjectedBundle/TestRunner.h:
(WTR::TestRunner::isMac const):
* Tools/WebKitTestRunner/TestController.h:
* Tools/WebKitTestRunner/mac/TestControllerMac.mm:
(WTR::TestController::platformInitialize):
(WTR::TestController::platformResetStateToConsistentValues):
* Tools/WebKitTestRunner/mac/UIScriptControllerMac.h:
* Tools/WebKitTestRunner/mac/UIScriptControllerMac.mm:
(WTR::UIScriptControllerMac::setAppAccentColor):

Canonical link: https://commits.webkit.org/266881@main
@webkit-commit-queue
Copy link
Collaborator

Committed 266881@main (04c640b): https://commits.webkit.org/266881@main

Reviewed commits have been landed. Closing PR #16649 and removing active labels.

@webkit-commit-queue webkit-commit-queue merged commit 04c640b into WebKit:main Aug 14, 2023
@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Aug 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Canvas Bugs related to the canvas element.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants