Bug 163250 – Implement prefers-reduced-motion media query

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Dean Jackson]

Implement prefers-reduced-motion media query

[Radar WebKit Bug Importer]

<rdar://problem/28704129>

[Dean Jackson]

Created attachment 291178 [details]
Patch

[Brent Fulgham]

Comment on attachment 291178 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=291178&action=review

Looks good -- aside from failing to build! :-)  r=me if you fix the minor build issue.

> Source/WebCore/ChangeLog:7
> +        Reviewed by NOBODY (OOPS!).

This fails to build on EFL/GTK (and maybe Windows) since they don't have an implementation of "platformTheme()".

It might be enough to just #if/def around PLATFORM(COCOA)?

> Source/WebCore/css/MediaQueryEvaluator.cpp:660
> +{

Maybe:

#if PLATFORM(COCOA)

> Source/WebCore/css/MediaQueryEvaluator.cpp:674
> +    }

#else
    return false;
#endif

[James Craig]

Comment on attachment 291178 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=291178&action=review

> Source/WebCore/css/MediaQueryEvaluator.cpp:670
> +    case CSSValueNone:
> +    case CSSValueReduce:
> +        return userPrefersReducedMotion;
> +    case CSSValueDefault:
> +        return !userPrefersReducedMotion;

I don't think CSSValueNone should be a synonym of CSSValueReduce. "None" is used frequently in MQ4 to mean indicate no preference, or default value, but the term `none` is ambiguous with some media features.  Since "prefers-reduced-motion: none" could mean either "I expect no motion" or "I expect no preference"— we used the `default` value to reduce ambiguity.

The following should be synonymous (Boolean: True):
"(prefers-reduced-motion)" or "(prefers-reduced-motion: reduce)"

Likewise these should be synonymous (Boolean: False):
"not (prefers-reduced-motion)" or "(prefers-reduced-motion: default)"

Due to the ambiguity in this context, the `none` value should remain undefined/invalid.

See "Enabling Media Features in a Boolean Context"
https://www.w3.org/TR/mediaqueries-4/#mq-boolean-context

[Dean Jackson]

Comment on attachment 291178 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=291178&action=review

>> Source/WebCore/css/MediaQueryEvaluator.cpp:660
>> +{
> 
> Maybe:
> 
> #if PLATFORM(COCOA)

I didn't want to limit this part to a platform. It should be the theme that does that.

>> Source/WebCore/css/MediaQueryEvaluator.cpp:670
>> +        return !userPrefersReducedMotion;
> 
> I don't think CSSValueNone should be a synonym of CSSValueReduce. "None" is used frequently in MQ4 to mean indicate no preference, or default value, but the term `none` is ambiguous with some media features.  Since "prefers-reduced-motion: none" could mean either "I expect no motion" or "I expect no preference"— we used the `default` value to reduce ambiguity.
> 
> The following should be synonymous (Boolean: True):
> "(prefers-reduced-motion)" or "(prefers-reduced-motion: reduce)"
> 
> Likewise these should be synonymous (Boolean: False):
> "not (prefers-reduced-motion)" or "(prefers-reduced-motion: default)"
> 
> Due to the ambiguity in this context, the `none` value should remain undefined/invalid.
> 
> See "Enabling Media Features in a Boolean Context"
> https://www.w3.org/TR/mediaqueries-4/#mq-boolean-context

Yeah, I misread that part. In fact, I still find it a bit confusing.

"If the feature would be true for any value other than the number 0, a <dimension> with the value 0, or the keyword none, the media feature evaluates to true. Otherwise, it evaluates to false."

I didn't associate the "other than" with the other clauses, so I read that as "If the feature would be true for the keyword none" and figured it meant that none had to evaluate to true. I'll fix this and the pull request.

[James Craig]

> "If the feature would be true for any value other than the number 0, a <dimension> with the value 0, or the keyword none, the media feature evaluates to true. Otherwise, it evaluates to false."

Looks like you would also need to add, "or the keyword default" to this section, and probably an informative note in the default value definition about how it evaluates to false in the Boolean context.

[James Craig]

For clarity, the last comment is about the related CSS PR:
https://github.com/w3c/csswg-drafts/pull/586

[Dean Jackson]

(In reply to comment #7)
> For clarity, the last comment is about the related CSS PR:
> https://github.com/w3c/csswg-drafts/pull/586

I just updated that to remove 'none' and clarify the 'default' falsiness.

[Dean Jackson]

Created attachment 291288 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 291288 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=291288&action=review

> Source/WebCore/testing/Internals.mm:55
> +#if PLATFORM(IOS)
> +    return UIAccessibilityIsReduceMotionEnabled();
> +#elif PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101200
> +    return [[NSWorkspace sharedWorkspace] accessibilityDisplayShouldReduceMotion];
> +#else
> +    return false;
> +#endif

This is a weird way to test. Shouldn't we set a flag that makes Theme::userPrefersReducedMotion() return true and test that way?

[Dean Jackson]

Comment on attachment 291288 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=291288&action=review

>> Source/WebCore/testing/Internals.mm:55
>> +#endif
> 
> This is a weird way to test. Shouldn't we set a flag that makes Theme::userPrefersReducedMotion() return true and test that way?

I considered this. However, we can't know what setting the tester might have. If you've already selected reduced motion, then we can't test the negative case. Basically, we can't override what the system says, and faking a system result doesn't test everything. So we either have to have a test that fakes reality but doesn't check the actual system, or a test that matches reality but doesn't necessarily check the entire code path. I went for the latter, because at least it can then serve as a manual test.

I also tested manually on macOS and iOS to make sure it works.

[Simon Fraser (smfr)]

(In reply to comment #11)
> Comment on attachment 291288 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=291288&action=review
> 
> >> Source/WebCore/testing/Internals.mm:55
> >> +#endif
> > 
> > This is a weird way to test. Shouldn't we set a flag that makes Theme::userPrefersReducedMotion() return true and test that way?
> 
> I considered this. However, we can't know what setting the tester might
> have.

Right, the test should be independent of the testing device config, which is why forcing a known state via internals is appropriate.

>  faking a system result doesn't test everything

It does; it tests all the WebKit code, which is what we're trying to test.

Simon

[Dean Jackson]

> > I considered this. However, we can't know what setting the tester might
> > have.
> 
> Right, the test should be independent of the testing device config, which is
> why forcing a known state via internals is appropriate.

It is independent of the testing device configuration.

> 
> >  faking a system result doesn't test everything
> 
> It does; it tests all the WebKit code, which is what we're trying to test.

OK. I'll make another test then. I want to keep the existing one.

[Dean Jackson]

Created attachment 291303 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 291303 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=291303&action=review

> Source/WebCore/testing/InternalSettings.cpp:709
> +InternalSettings::ForcedPrefersReducedMotionValue InternalSettings::forcedPrefersReducedMotionValue() const
> +{
> +    switch (settings()->forcedPrefersReducedMotionValue()) {
> +    case Settings::ForcedPrefersReducedMotionValue::System:
> +        return InternalSettings::ForcedPrefersReducedMotionValue::System;
> +    case Settings::ForcedPrefersReducedMotionValue::On:
> +        return InternalSettings::ForcedPrefersReducedMotionValue::On;
> +    case Settings::ForcedPrefersReducedMotionValue::Off:
> +        return InternalSettings::ForcedPrefersReducedMotionValue::Off;
> +    }
> +}
> +
> +void InternalSettings::setForcedPrefersReducedMotionValue(InternalSettings::ForcedPrefersReducedMotionValue value)
> +{
> +    switch (value) {
> +    case InternalSettings::ForcedPrefersReducedMotionValue::System:
> +        settings()->setForcedPrefersReducedMotionValue(Settings::ForcedPrefersReducedMotionValue::System);
> +        break;
> +    case InternalSettings::ForcedPrefersReducedMotionValue::On:
> +        settings()->setForcedPrefersReducedMotionValue(Settings::ForcedPrefersReducedMotionValue::On);
> +        break;
> +    case InternalSettings::ForcedPrefersReducedMotionValue::Off:
> +        settings()->setForcedPrefersReducedMotionValue(Settings::ForcedPrefersReducedMotionValue::Off);
> +        break;
> +    }
> +}

Sad that you have to do manual mapping. But this would be slightly clearer with some C++ conversion operators.

[Dean Jackson]

Committed r207173: <http://trac.webkit.org/changeset/207173>

[Said Abou-Hallawa]

Comment on attachment 291303 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=291303&action=review

> Source/WebCore/testing/InternalSettings.cpp:693
> +    }

The default case is not handled. This broke the Windows build.

[Gyuyoung Kim]

I fix a build break on EFL port since r207173 - https://trac.webkit.org/changeset/207183

[Ryan Haddad]

(In reply to comment #17)
> Comment on attachment 291303 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=291303&action=review
> 
> > Source/WebCore/testing/InternalSettings.cpp:693
> > +    }
> 
> The default case is not handled. This broke the Windows build.

c:\cygwin\home\buildbot\slave\win-release\build\source\webcore\testing\internalsettings.cpp(694): error C2220: warning treated as error - no 'object' file generated [C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\Source\WebCore\WebCoreTestSupport.vcxproj]
c:\cygwin\home\buildbot\slave\win-release\build\source\webcore\testing\internalsettings.cpp(694): warning C4715: 'WebCore::InternalSettings::forcedPrefersReducedMotionValue': not all control paths return a value [C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\Source\WebCore\WebCoreTestSupport.vcxproj]

https://build.webkit.org/builders/Apple%20Win%20Release%20%28Build%29/builds/81093

[Hiroyuki Ikezoe]

I just start implementing this feature in Firefox.  It would be nice to have web-platform test for this.  Would you mind uplifting these tests in wpt?

[Cameron]

View in context: https://bugs.webkit.org/attachment.cgi?id=291178&action=review

Looks good -- aside from failing to build! :-)  r=me if you fix the minor build issue.

> Source/WebCore/ChangeLog:7
> +        Reviewed by NOBODY (OOPS!).


This fails to build on EFL/GTK (and maybe Windows) since they don't have an implementation of "platformTheme()".

It might be enough to just #if/def around PLATFORM(COCOA)?

> Source/WebCore/css/MediaQueryEvaluator.cpp:660
> +{


Maybe: yahoo mail sign up https://www.just4dummies.com/yahoo-mail-sign-up

#if PLATFORM(COCOA)

> Source/WebCore/css/MediaQueryEvaluator.cpp:674
> +    }


#else
    return false;
#endif