Bug 222381 - prefers-reduced-motion is not reactive on iOS
Summary: prefers-reduced-motion is not reactive on iOS
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: Safari 14
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Per Arne Vollan
URL:
Keywords: InRadar
: 225090 (view as bug list)
Depends on:
Blocks:
 
Reported: 2021-02-24 13:54 PST by Liam DeBeasi
Modified: 2021-04-30 14:04 PDT (History)
7 users (show)

See Also:


Attachments
Code reproduction (1008 bytes, text/html)
2021-02-24 13:54 PST, Liam DeBeasi
no flags Details
Patch (3.67 KB, patch)
2021-04-28 07:16 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (2.66 KB, patch)
2021-04-30 06:55 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (2.67 KB, patch)
2021-04-30 06:59 PDT, Per Arne Vollan
bfulgham: review+
Details | Formatted Diff | Diff
Patch (2.76 KB, patch)
2021-04-30 13:25 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Liam DeBeasi 2021-02-24 13:54:10 PST
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.
Comment 1 Radar WebKit Bug Importer 2021-02-24 18:08:39 PST
<rdar://problem/74724161>
Comment 2 Per Arne Vollan 2021-04-28 07:16:45 PDT
Created attachment 427263 [details]
Patch
Comment 3 chris fleizach 2021-04-28 08:21:31 PDT
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 4 Simon Fraser (smfr) 2021-04-28 09:51:24 PDT
Comment on attachment 427263 [details]
Patch

Please make a way to test this so it doesn't break again.
Comment 5 chris fleizach 2021-04-28 10:09:22 PDT
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 6 Tim Horton 2021-04-28 11:57:33 PDT
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 7 chris fleizach 2021-04-28 12:02:55 PDT
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
Comment 8 Per Arne Vollan 2021-04-28 16:24:10 PDT
*** Bug 225090 has been marked as a duplicate of this bug. ***
Comment 9 Brent Fulgham 2021-04-29 09:30:30 PDT
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 10 chris fleizach 2021-04-29 09:33:17 PDT
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;
Comment 11 Brent Fulgham 2021-04-29 09:51:22 PDT
(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.
Comment 12 Per Arne Vollan 2021-04-30 06:52:48 PDT
<rdar://75933915>
Comment 13 Per Arne Vollan 2021-04-30 06:55:16 PDT
Created attachment 427413 [details]
Patch
Comment 14 Per Arne Vollan 2021-04-30 06:56:56 PDT
(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!
Comment 15 Per Arne Vollan 2021-04-30 06:59:14 PDT
Created attachment 427414 [details]
Patch
Comment 16 Per Arne Vollan 2021-04-30 07:00:39 PDT
(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 17 Brent Fulgham 2021-04-30 09:51:35 PDT
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 18 Simon Fraser (smfr) 2021-04-30 09:53:23 PDT
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.
Comment 19 Per Arne Vollan 2021-04-30 13:25:23 PDT
Created attachment 427442 [details]
Patch
Comment 20 Per Arne Vollan 2021-04-30 13:27:30 PDT
(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!
Comment 21 Per Arne Vollan 2021-04-30 13:35:24 PDT
(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 :)
Comment 22 Brent Fulgham 2021-04-30 13:40:41 PDT
(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.
Comment 23 EWS 2021-04-30 14:04:26 PDT
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].