Created attachment 421455 [details] Code reproduction When toggling the "Reduce Motion" preference in the Accessibility pane on iOS, the `prefers-reduced-motion` media query is not updated in WebKit. Steps to reproduce: 1. Open attached code reproduction on an iOS device with the "Reduce Motion" toggle off. This option can be accessed via Settings > Accessibility > Motion > Reduce Motion. 2. Observe that there is a blue square with an animation applied. The animation scales the square as well as fades it out. 3. Turn the "Reduce Motion" toggle on. 4. Observe that the square's animation no longer scales, just fades out. 5. Turn the "Reduce Motion" toggle off. 6. Observe that the square's animation does not begin scaling again. Expected Behavior: I would expect that after turning "Reduce Motion" off in step 5 the scaling would start again in the square's animation. Actual Behavior: The scaling does not start again. Other Info: - I am able to reproduce this on iOS 14.4 as well as the iOS 14.5 developer beta. - This works as expected on Safari and Chrome for macOS.
<rdar://problem/74724161>
Created attachment 427263 [details] Patch
Comment on attachment 427263 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=427263&action=review > Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:1072 > + constexpr ASCIILiteral key { "reduceMotion"_s }; Are these keys that are coming from the system? Can we use the constants if available?
Comment on attachment 427263 [details] Patch Please make a way to test this so it doesn't break again.
Comment on attachment 427263 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=427263&action=review >> Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:1072 >> + constexpr ASCIILiteral key { "reduceMotion"_s }; > > Are these keys that are coming from the system? Can we use the constants if available? kAXSReduceMotionPreference
Comment on attachment 427263 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=427263&action=review >>> Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:1072 >>> + constexpr ASCIILiteral key { "reduceMotion"_s }; >> >> Are these keys that are coming from the system? Can we use the constants if available? > > kAXSReduceMotionPreference ALSO, which one is it in macCatalyst? > Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:1084 > + return kAXInterfaceReduceMotionStatusDidChangeNotification; > +#else Ditto my macCatalyst question.
Comment on attachment 427263 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=427263&action=review >>>> Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:1072 >>>> + constexpr ASCIILiteral key { "reduceMotion"_s }; >>> >>> Are these keys that are coming from the system? Can we use the constants if available? >> >> kAXSReduceMotionPreference > > ALSO, which one is it in macCatalyst? this one iOS and Mac catalyst: kAXSReduceMotionPreference >> Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:1084 >> +#else > > Ditto my macCatalyst question. this one is iOS and catalyst kAXSReduceMotionChangedNotification
*** Bug 225090 has been marked as a duplicate of this bug. ***
Comment on attachment 427263 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=427263&action=review I think this looks like a correct fix (modulo the build errors), but please do use the system constants instead of hard coding the strings. >>>>> Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:1072 >>>>> + constexpr ASCIILiteral key { "reduceMotion"_s }; >>>> >>>> Are these keys that are coming from the system? Can we use the constants if available? >>> >>> kAXSReduceMotionPreference >> >> ALSO, which one is it in macCatalyst? > > this one iOS and Mac catalyst: > kAXSReduceMotionPreference @Chris: So it's the same constant for the preference key on both platforms? > Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:1076 > + static NeverDestroyed<WTF::String> key(MAKE_STATIC_STRING_IMPL(key)); You use 'key' twice here. Maybe label the constant as 'keyString' or something?
Comment on attachment 427263 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=427263&action=review >>>>>> Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:1072 >>>>>> + constexpr ASCIILiteral key { "reduceMotion"_s }; >>>>> >>>>> Are these keys that are coming from the system? Can we use the constants if available? >>>> >>>> kAXSReduceMotionPreference >>> >>> ALSO, which one is it in macCatalyst? >> >> this one iOS and Mac catalyst: >> kAXSReduceMotionPreference > > @Chris: So it's the same constant for the preference key on both platforms? it looks like it's this one on the Mac extern CFStringRef kAXInterfaceReduceMotionKey;
(In reply to Simon Fraser (smfr) from comment #4) > Comment on attachment 427263 [details] > Patch > > Please make a way to test this so it doesn't break again. It looks like there is a test, but it only runs on macOS. Seems like it should be possible to get it working on iOS.
<rdar://75933915>
Created attachment 427413 [details] Patch
(In reply to Simon Fraser (smfr) from comment #4) > Comment on attachment 427263 [details] > Patch > > Please make a way to test this so it doesn't break again. That's a good point. We are currently looking into reusing the attached test case. Thanks for reviewing!
Created attachment 427414 [details] Patch
(In reply to Per Arne Vollan from comment #15) > Created attachment 427414 [details] > Patch A new approach has been taken in the latest patch. Thanks for reviewing, all!
Comment on attachment 427414 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=427414&action=review r=me > Source/WebKit/ChangeLog:10 > + the notification in the WebProcess. Resolve this by updating the cached settings values in Accessibility when a I think it might be more accurate to say: "... race between the notification that an accessibility preference has been changed by the user, and the the in-memory Accessibility preference cache in the WebContent process. We resolve this by using new SPI to clear the cached Accessibility settings when we are notified of an accessibility change so they always reflect the user's most recent setting." > Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:1127 > + WTFLogAlways("Updating Web Accessibility settings."); I suggest using RELEASE_LOG_IF_ALLOWED here, so that the error message is formatted in a way that our triage script can easily consume. > Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:1129 > + } I suggest logging when the SPI doesn't exist so we can review logs and verify this is why a user is not seeing the right behavior. I also suggest using RELEASE_LOG_ERROR_IF_ALLOWED for this non-SPI case so we can catch it in triage.
Comment on attachment 427414 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=427414&action=review I would prefer we don't land this without a test. >> Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:1127 >> + WTFLogAlways("Updating Web Accessibility settings."); > > I suggest using RELEASE_LOG_IF_ALLOWED here, so that the error message is formatted in a way that our triage script can easily consume. Please remove.
Created attachment 427442 [details] Patch
(In reply to Brent Fulgham from comment #17) > Comment on attachment 427414 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=427414&action=review > > r=me > > > Source/WebKit/ChangeLog:10 > > + the notification in the WebProcess. Resolve this by updating the cached settings values in Accessibility when a > > I think it might be more accurate to say: > > "... race between the notification that an accessibility preference has been > changed by the user, and the the in-memory Accessibility preference cache in > the WebContent process. We resolve this by using new SPI to clear the cached > Accessibility settings when we are notified of an accessibility change so > they always reflect the user's most recent setting." > Fixed! > > Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:1127 > > + WTFLogAlways("Updating Web Accessibility settings."); > > I suggest using RELEASE_LOG_IF_ALLOWED here, so that the error message is > formatted in a way that our triage script can easily consume. > > > Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:1129 > > + } > > I suggest logging when the SPI doesn't exist so we can review logs and > verify this is why a user is not seeing the right behavior. > > I also suggest using RELEASE_LOG_ERROR_IF_ALLOWED for this non-SPI case so > we can catch it in triage. I removed the logging, since it was added for debugging purposes. Thanks for reviewing!
(In reply to Simon Fraser (smfr) from comment #18) > Comment on attachment 427414 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=427414&action=review > > I would prefer we don't land this without a test. > Yes, that makes sense. Since it's not trivial to create this test, and we're currently not close to having the test case ready, I filed https://bugs.webkit.org/show_bug.cgi?id=225243 to track this. > >> Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:1127 > >> + WTFLogAlways("Updating Web Accessibility settings."); > > > > I suggest using RELEASE_LOG_IF_ALLOWED here, so that the error message is formatted in a way that our triage script can easily consume. > > Please remove. Done! Thanks for reviewing :)
(In reply to Simon Fraser (smfr) from comment #18) > Comment on attachment 427414 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=427414&action=review > > I would prefer we don't land this without a test. Coming in Bug 225244. > >> Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:1127 > >> + WTFLogAlways("Updating Web Accessibility settings."); > > > > I suggest using RELEASE_LOG_IF_ALLOWED here, so that the error message is formatted in a way that our triage script can easily consume. > > Please remove.
Committed r276852 (237203@main): <https://commits.webkit.org/237203@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 427442 [details].