Bug 222381 – prefers-reduced-motion is not reactive on iOS

• Status: RESOLVED
• Resolution: FIXED
• Component: Accessibility

[Liam DeBeasi]

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.

[Radar WebKit Bug Importer]

<rdar://problem/74724161>

[Per Arne Vollan]

Created attachment 427263 [details]
Patch

[chris fleizach]

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?

[Simon Fraser (smfr)]

Comment on attachment 427263 [details]
Patch

Please make a way to test this so it doesn't break again.

[chris fleizach]

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

[Tim Horton]

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.

[chris fleizach]

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

[Per Arne Vollan]

*** Bug 225090 has been marked as a duplicate of this bug. ***

[Brent Fulgham]

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?

[chris fleizach]

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;

[Brent Fulgham]

(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.

[Per Arne Vollan]

<rdar://75933915>

[Per Arne Vollan]

Created attachment 427413 [details]
Patch

[Per Arne Vollan]

(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!

[Per Arne Vollan]

Created attachment 427414 [details]
Patch

[Per Arne Vollan]

(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!

[Brent Fulgham]

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.

[Simon Fraser (smfr)]

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.

[Per Arne Vollan]

Created attachment 427442 [details]
Patch

[Per Arne Vollan]

(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!

[Per Arne Vollan]

(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 :)

[Brent Fulgham]

(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.

[EWS]

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].