Implement prefers-reduced-motion media query
<rdar://problem/28704129>
Created attachment 291178 [details] Patch
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
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
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.
> "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.
For clarity, the last comment is about the related CSS PR: https://github.com/w3c/csswg-drafts/pull/586
(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.
Created attachment 291288 [details] Patch
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?
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.
(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
> > 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.
Created attachment 291303 [details] Patch
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.
Committed r207173: <http://trac.webkit.org/changeset/207173>
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.
I fix a build break on EFL port since r207173 - https://trac.webkit.org/changeset/207183
(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
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?
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