Add support for CSS prefers-reduced-motion media feature for Windows

RESOLVED FIXED in Firefox 63

Status

()

enhancement
P3
normal
RESOLVED FIXED
2 years ago
6 months ago

People

(Reporter: james, Assigned: hiro)

Tracking

(Blocks 3 bugs, {access, dev-doc-complete, DevAdvocacy})

Trunk
mozilla63
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox63 fixed)

Details

(Whiteboard: [devRel:P1][FFI?])

Attachments

(5 attachments, 11 obsolete attachments)

5.08 KB, patch
Details | Diff | Splinter Review
59 bytes, text/x-review-board-request
heycam
: review+
Details
59 bytes, text/x-review-board-request
heycam
: review+
Details
59 bytes, text/x-review-board-request
hiro
: review+
Details
59 bytes, text/x-review-board-request
heycam
: review+
Details
(Reporter)

Description

2 years ago
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_6) AppleWebKit/603.2.4 (KHTML, like Gecko) Version/10.1.1 Safari/603.2.4




Expected results:

Add support for CSS prefers-reduced-motion media feature.

New in CSS Media Queries Level 5:
https://drafts.csswg.org/mediaqueries-5/#prefers-reduced-motion

WebKit blog post covering the feature, justification, and examples:
https://webkit.org/blog/7551/responsive-design-for-motion/
(Reporter)

Comment 2

2 years ago
Initial WebKit patch (using the oolder "default" value):
https://bugs.webkit.org/show_bug.cgi?id=163250

Updated

2 years ago
Severity: normal → enhancement
Component: Untriaged → CSS Parsing and Computation
Product: Firefox → Core
Priority: -- → P3

Comment 4

2 years ago
There is potentially a new WCAG success criteria coming in 2018 about animations/motion: 
https://github.com/w3c/wcag21/issues/18

It would be very handy for developers to 'have their cake and eat it', i.e. have the animations requested by some stakeholders, but allow users to turn it off. Otherwise an on-screen mechanism might be needed to meet the guideline (depending on how it proceeds through the process).

Comment 6

2 years ago
Ohh this would be great to have in Firefox!

Comment 7

2 years ago
I vote for this

Comment 8

2 years ago
As a vestibular disorder having person, this would be amazing to have in place.

Comment 9

2 years ago
(In reply to github from comment #6)
> Ohh this would be great to have in Firefox!

+1

Comment 10

2 years ago
I'd like to see this supported
This is important. We're going to want to implement this sooner rather than later.
Keywords: DevAdvocacy
Whiteboard: [devRel:P1]

Comment 12

2 years ago
I think this is important and would love to see this be prioritized and supported.

Comment 13

2 years ago
Please implement. It’s important for users to be able to use this feature consistently, cross-browser/platform.

Comment 14

2 years ago
Please prioritize, this is an important accessibility feature.

Comment 15

2 years ago
This would be a great accessibility feature to add.
Jet do you know how much work this would take?
Flags: needinfo?(bugs)
(In reply to David Bolter [:davidb] (NeedInfo me for attention) from comment #16)
> Jet do you know how much work this would take?

Brian is the best person to ask. This looks like something his team can hook into Stylo.
Flags: needinfo?(bugs) → needinfo?(bbirtles)
This shouldn't be too hard except we'll need platform-specific integration for each platform and ideally we'll want DevTool support too (which is probably the hardest part since we'll have two sources of truth). As far as I can tell there's no overlap with the animation code at all.

Leaving needinfo for now so I remember to make a note of this when I get back to work next week.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(bbirtles)
Flags: needinfo?(bbirtles)
Added it to my list of things to prioritize in 2018.
Flags: needinfo?(bbirtles)
Whiteboard: [devRel:P1] → [devRel:P1][FFI?]

Updated

a year ago
(Assignee)

Comment 20

a year ago
Here is a patch to try to implement it but this patch couldn't be built on try server[1], since, the SDK on the build environment 10.11 (glandium told me on IRC), but the API we need to use is supported 10.12 or higher.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=fd149e55ce2f27577fbc758f42938854ef5e8dff
(Assignee)

Updated

a year ago
Depends on: 1391023
(Assignee)

Comment 21

a year ago
I don't have Mac environment, so I can't confirm the patch can be built. :)

For other platforms setting ui.prefersReducedMotion to 1 is equivalent with the state that reduced motion is off on MacOSX.
(Assignee)

Comment 22

a year ago
(In reply to Hiroyuki Ikezoe (:hiro) from comment #21)
> I don't have Mac environment, so I can't confirm the patch can be built. :)
> 
> For other platforms setting ui.prefersReducedMotion to 1 is equivalent with
> the state that reduced motion is off on MacOSX.

reduced motion is *on*.
Keywords: access

Comment 23

a year ago
I'd love to see this addressed. We have VIMS issues tracked in Drupal here 
https://www.drupal.org/project/issues/search?issue_tags=VIMS
Hiro -- Can you take this bug?  Product really want this to land as soon as possible (it's on their shortlist of most important CSS features).  Thank you!
Flags: needinfo?(hikezoe)
(Assignee)

Comment 25

10 months ago
OK, I can take this.  Note that as for MacOSX, attachment 8948796 [details] [diff] [review] should work as it is, but unfortunately it's not being able to compile on our vuild infrastructures due to bug 1391023.  And as for bug 1391023, I did try to solve bug 1391023 before and it turns out we need to change cctools [1] to support SDK 10.12 or later and I don't have enough knowledge about the tools yet.

Anyway, I am going to start looking into Windows case as well.

[1] https://github.com/tpoechtrager/cctools-port
Assignee: nobody → hikezoe
Status: NEW → ASSIGNED
Flags: needinfo?(hikezoe)

Comment 26

10 months ago
(In reply to Hiroyuki Ikezoe (:hiro) from comment #25)
> Anyway, I am going to start looking into Windows case as well.

The Windows system level setting for this can be retrieved using the SPI_GETCLIENTAREAANIMATION parameter to the SystemParametersInfo function:
https://msdn.microsoft.com/en-us/library/windows/desktop/ms724947(v=vs.85).aspx
I just confirmed that on Windows 10, changing Settings -> Ease of Access -> Display -> Show animations in Windows does affect the value of this API parameter.
(In reply to James Teh [:Jamie] from comment #26)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #25)
> > Anyway, I am going to start looking into Windows case as well.
> 
> The Windows system level setting for this can be retrieved using the
> SPI_GETCLIENTAREAANIMATION parameter to the SystemParametersInfo function:
> https://msdn.microsoft.com/en-us/library/windows/desktop/ms724947(v=vs.85).
> aspx
> I just confirmed that on Windows 10, changing Settings -> Ease of Access ->
> Display -> Show animations in Windows does affect the value of this API
> parameter.

Is that the same as this option:

  https://cdn1.tekrevue.com/wp-content/uploads/2016/04/disable-windows-10-animations-control-panel.jpg

i.e.

Ease of Access -> Make the computer easier to see -> Make things on the screen easier to see -> Turn of all unnecessary animations (when possible)

?
Flags: needinfo?(jteh)

Comment 28

10 months ago
(In reply to Brian Birtles (:birtles) from comment #27)
> Is that the same as this option:
> Ease of Access -> Make the computer easier to see -> Make things on the
> screen easier to see -> Turn of all unnecessary animations (when possible)

Yes. That's the old UI, which you can now only access if you explicitly choose to open Control Panel instead of the new Settings app. I just double checked and this setting also affects the API I mentioned in the expected way. (Obviously, this setting is inversed; checking the box exposes false for the parameter.)
Flags: needinfo?(jteh)
(Assignee)

Comment 29

10 months ago
Huh..  I did find another solution using registry.
https://superuser.com/questions/1052763/windows-10-disable-animations-via-regedit-script

Does this provide the same functionality of the API?

Anyway, I've finished it now.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d1b077e04839d1fba6d24d6a97717c6cd4b6d5bd
(Assignee)

Updated

10 months ago
Blocks: 1475462
(Assignee)

Comment 30

10 months ago
I am going to use this bug only for Windows, filed a new bug for MacOSX (bug 1475462).
No longer depends on: 1391023
Summary: Add support for CSS prefers-reduced-motion media feature → Add support for CSS prefers-reduced-motion media feature for Windows

Comment 31

10 months ago
(In reply to Hiroyuki Ikezoe (:hiro) from comment #29)
> Huh..  I did find another solution using registry.
> https://superuser.com/questions/1052763/windows-10-disable-animations-via-
> regedit-script
> Does this provide the same functionality of the API?

It seems to. However, I'd suggest you may want to use the API instead. I am not certain, but it may be possible to use SystemParametersInfo to change this setting *without* applying it persistently. Assistive technology products might use this only while they're running, for example. At the very least, you'll want to verify this using SystemParametersInfo from another process to see what happens. Note the fWinIni parameter in particular.
(Assignee)

Comment 32

10 months ago
(In reply to James Teh [:Jamie] from comment #31)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #29)
> > Huh..  I did find another solution using registry.
> > https://superuser.com/questions/1052763/windows-10-disable-animations-via-
> > regedit-script
> > Does this provide the same functionality of the API?
> 
> It seems to. However, I'd suggest you may want to use the API instead.

Yeah, agree. I've pushed another try using the API instead, let's see.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=499d92f9201e61041c8b84984ac1ee9787e345a1

> products might use this only while they're running, for example. At the very
> least, you'll want to verify this using SystemParametersInfo from another
> process to see what happens. Note the fWinIni parameter in particular.

That's interesting.  I am totally unfamiliar with Windows APIs, but with the parameter, system notifies us the setting changes?  So we can do change contents dynamically when the setting changes?  WebKit didn't implement such features (at least in https://bugs.webkit.org/show_bug.cgi?id=163250), but they did after that?

Note that I've noticed GNOME has a similar feature, you can get/set the preference value with gsettings, it's actually out of scope of this bug, though.

 gsettings get org.gnome.desktop.interface enable-animations

Comment 33

10 months ago
(In reply to Hiroyuki Ikezoe (:hiro) from comment #32)
> Yeah, agree. I've pushed another try using the API instead, let's see.
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=499d92f9201e61041c8b84984ac1ee9787e345a1

Patch looks good to me. :)

> That's interesting.  I am totally unfamiliar with Windows APIs, but with the
> parameter, system notifies us the setting changes?  So we can do change
> contents dynamically when the setting changes? 

Yes. I've never tested this, but according to the docs, we should get a WM_SETTINGCHANGE notification with wParam set to SPI_SETCLIENTAREAANIMATION. It seems we already have code to respond to WM_SETTINGCHANGE messages:
https://searchfox.org/mozilla-central/source/widget/windows/nsWindow.cpp#5314
(Assignee)

Updated

10 months ago
No longer blocks: 1475462
(Assignee)

Comment 34

10 months ago
I did a try with capturing SPI_SETCLIENTAREAANIMATION change [1], but it doesn't work well.

Given that WebKit doesn't support the dynamic change yet (as far as I can tell, to support it, it needs to observe accessibilityDisplayOptionsDidChange, but as of today, WebKit doesn't observe it yet), I thinkwe can land this without it.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=1c66801a030370388695d19edeff47839e271552

Comment 35

10 months ago
(In reply to Hiroyuki Ikezoe (:hiro) from comment #34)
> I did a try with capturing SPI_SETCLIENTAREAANIMATION change [1], but it
> doesn't work well.

Can you explain in what way it didn't work well? (I'm totally blind, so can't test the visual output myself.)

> Given that WebKit doesn't support the dynamic change yet...
> I thinkwe can land this without it.

I think that's reasonable. Might be good to file a follow up to support it, though.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

10 months ago
Blocks: 1475462
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

10 months ago
Attachment #8992519 - Attachment is obsolete: true
Attachment #8992519 - Flags: review?(cam)
(Assignee)

Updated

10 months ago
Attachment #8992520 - Attachment is obsolete: true
Attachment #8992520 - Flags: review?(cam)
(Assignee)

Updated

10 months ago
Attachment #8992521 - Attachment is obsolete: true
Attachment #8992521 - Flags: review?(jmathies)
(Assignee)

Comment 42

10 months ago
(In reply to James Teh [:Jamie] from comment #35)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #34)
> > I did a try with capturing SPI_SETCLIENTAREAANIMATION change [1], but it
> > doesn't work well.
> 
> Can you explain in what way it didn't work well? (I'm totally blind, so
> can't test the visual output myself.)

I don't have build/debug environments for Window, so I have no idea what's going on there.  What I can tell is that with the binary in the try in comment 29 contents having prefers-reduced-motion were not affected by the change ofthe setting on Windows, I had to reload by hand to update the content.  Anyway I am going to revisit it in a later bug.
(Assignee)

Updated

10 months ago
Depends on: 1476212
(Assignee)

Comment 43

10 months ago
(In reply to Hiroyuki Ikezoe (:hiro) from comment #42)
> (In reply to James Teh [:Jamie] from comment #35)
> > (In reply to Hiroyuki Ikezoe (:hiro) from comment #34)
> > > I did a try with capturing SPI_SETCLIENTAREAANIMATION change [1], but it
> > > doesn't work well.
> > 
> > Can you explain in what way it didn't work well? (I'm totally blind, so
> > can't test the visual output myself.)
> 
> I don't have build/debug environments for Window, so I have no idea what's
> going on there.  What I can tell is that with the binary in the try in
> comment 29 contents having prefers-reduced-motion were not affected by the
> change ofthe setting on Windows, I had to reload by hand to update the
> content.  Anyway I am going to revisit it in a later bug.

OK, I think I know what's going on there.  We have to call nsPresContext::MediaFeatureValuesChanged() instead of NotifyThemeChanged to notify the change to all descendant documents.  Anyway filed bug 1476212 for that.

Comment 44

10 months ago
mozreview-review
Comment on attachment 8992533 [details]
Bug 1365045 - Implement prefers-reduced-motion for Windows.

https://reviewboard.mozilla.org/r/257384/#review265206
Attachment #8992533 - Flags: review?(jmathies) → review+

Comment 45

10 months ago
mozreview-review
Comment on attachment 8992531 [details]
Bug 1365045 - Introduce keywords for prefers-reduced-motion.

https://reviewboard.mozilla.org/r/257380/#review265310

::: layout/style/nsMediaFeatures.cpp:838
(Diff revision 1)
> +  {
> +    &nsGkAtoms::prefersReducedMotion,
> +    nsMediaFeature::eMinMaxNotAllowed,
> +    nsMediaFeature::eEnumerated,
> +    nsMediaFeature::eNoRequirements,
> +    { kPrefersReducedMotionKeywords },
> +    GetPrefersReducedMotion
> +  },

This is defined as eEnumerated, which I think means that `@media (prefers-reduced-motion)` would evaluate to true even when the system setting is "no preference", but the spec says that should evaluate to false in a Boolean context.  To fix this MediaFeatureExpression::evaluate_against needs to update its check in the `match self.value` bit to somehow return false in this case.
Attachment #8992531 - Flags: review?(cam) → review-

Comment 46

10 months ago
mozreview-review
Comment on attachment 8992532 [details]
Bug 1365045 - Reftests for prefers-reduced-motion.

https://reviewboard.mozilla.org/r/257382/#review265318

Looks good but please add a test for `@media (prefers-reduced-motion)` too.
Attachment #8992532 - Flags: review?(cam) → review+
(Assignee)

Comment 47

10 months ago
(In reply to Cameron McCormack (:heycam) from comment #45)
> Comment on attachment 8992531 [details]
> Bug 1365045 - Introduce keywords for prefers-reduced-motion.
> 
> https://reviewboard.mozilla.org/r/257380/#review265310
> 
> ::: layout/style/nsMediaFeatures.cpp:838
> (Diff revision 1)
> > +  {
> > +    &nsGkAtoms::prefersReducedMotion,
> > +    nsMediaFeature::eMinMaxNotAllowed,
> > +    nsMediaFeature::eEnumerated,
> > +    nsMediaFeature::eNoRequirements,
> > +    { kPrefersReducedMotionKeywords },
> > +    GetPrefersReducedMotion
> > +  },
> 
> This is defined as eEnumerated, which I think means that `@media
> (prefers-reduced-motion)` would evaluate to true even when the system
> setting is "no preference", but the spec says that should evaluate to false
> in a Boolean context.

Thank you, Cameron. You are absolutely right. :)  I didn't understand about the Boolean context.

>  To fix this MediaFeatureExpression::evaluate_against
> needs to update its check in the `match self.value` bit to somehow return
> false in this case.

It was relatively easy to be done to fix that, thanks to this pointer.
In my local patch I did add an additional enum in nsCSSKTableEntry to represent the value in the Boolean context.  It might be possible that other value types (e.g. eInteger) would need to do something similar to this mechanism in future, I am not sure though, I would be happy to do that. (probably neMediaFeature would need a function called from Rust side).
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

10 months ago
Attachment #8992531 - Attachment is obsolete: true
(Assignee)

Updated

10 months ago
Attachment #8992532 - Attachment is obsolete: true
(Assignee)

Updated

10 months ago
Attachment #8992533 - Attachment is obsolete: true
(Assignee)

Comment 52

10 months ago
My local settings for MozReview seem to be somehow broken...
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

10 months ago
Attachment #8994057 - Attachment is obsolete: true
Attachment #8994057 - Flags: review?(cam)
(Assignee)

Updated

10 months ago
Attachment #8994058 - Attachment is obsolete: true
Attachment #8994058 - Flags: review?(cam)
(Assignee)

Updated

10 months ago
Attachment #8994059 - Attachment is obsolete: true
Attachment #8994059 - Flags: review?(cam)
(Assignee)

Updated

10 months ago
Attachment #8994060 - Attachment is obsolete: true
Attachment #8994060 - Flags: review?(jmathies)
(Assignee)

Comment 57

10 months ago
Adding the previous MozReviewId made MozReview confusing. :/
It seems a shame to make nsCSSKTableEntry larger for all entries, and not just those that are used for media features.  What do you think about adding this to nsMediaFeature instead?  I think it would be fine just to add a boolean to represent whether eEnumerated value 0 should be treated as false in a boolean context.  Or a new ValueType eBoolEnumerated that has that behavior.
Flags: needinfo?(hikezoe)

Comment 59

10 months ago
mozreview-review
Comment on attachment 8994064 [details]
Bug 1365045 - Reftests for prefers-reduced-motion.

https://reviewboard.mozilla.org/r/258642/#review265584
Attachment #8994064 - Flags: review?(cam) → review+
(Assignee)

Comment 60

10 months ago
(In reply to Cameron McCormack (:heycam) from comment #58)
> It seems a shame to make nsCSSKTableEntry larger for all entries, and not
> just those that are used for media features.

As of today we have only three entries for nsCSSKTableEntry so I think the size won't be a big deal.  Whereas,

> What do you think about adding
> this to nsMediaFeature instead?  I think it would be fine just to add a
> boolean to represent whether eEnumerated value 0 should be treated as false
> in a boolean context.  Or a new ValueType eBoolEnumerated that has that
> behavior.

Adding a boolean into nsMediaFeature affects all media feature I think, no?  Actually I did initially add the boolean there, but moved it into nsCSSKTableEntry for the reason above.
Flags: needinfo?(hikezoe)
(In reply to Hiroyuki Ikezoe (:hiro) from comment #60)
> As of today we have only three entries for nsCSSKTableEntry so I think the
> size won't be a big deal.

Three entries in that table, but by my count we have 45 tables in nsCSSProps.cpp, with a total of 384 table entries.

> Adding a boolean into nsMediaFeature affects all media feature I think, no?

Yes, but I think you can at least fit that in nsMediaFeature without making that object any bigger.
(Assignee)

Comment 62

10 months ago
(In reply to Cameron McCormack (:heycam) from comment #61)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #60)
> > As of today we have only three entries for nsCSSKTableEntry so I think the
> > size won't be a big deal.
> 
> Three entries in that table, but by my count we have 45 tables in
> nsCSSProps.cpp, with a total of 384 table entries.

Oh right.  I didn't notice them at all.  I will moving the flag into nsMediaFeature.
Thanks!
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

10 months ago
Attachment #8994062 - Attachment is obsolete: true
Attachment #8994062 - Flags: review?(cam)

Comment 67

10 months ago
mozreview-review
Comment on attachment 8994143 [details]
Bug 1365045 - Introduce nsCSSKeywordAndBoolTableEntry.

https://reviewboard.mozilla.org/r/258760/#review265934

Looks good, but I'd like to see some code duplication reduced.

::: layout/style/nsMediaFeatures.h:21
(Diff revision 1)
> +  enum ValueInBooleanContext {
> +    eFalse,
> +    eTrue,
> +  };

I don't think there's much value in making this an enum.  Let's just use bool values.

::: servo/components/style/gecko/media_queries.rs:528
(Diff revision 1)
> -unsafe fn find_in_table<F>(
> -    mut current_entry: *const nsCSSKTableEntry,
> -    mut f: F,
> +trait FindInTable {
> +    fn value(&self) -> i16;
> +    fn keyword(&self) -> nsCSSKeyword;
> +}
> +
> +impl FindInTable for nsCSSKTableEntry {

I think a better name for this trait would be "TableEntry".

::: servo/components/style/gecko/media_queries.rs:573
(Diff revision 1)
>          current_entry = current_entry.offset(1);
>      }
>  }
>  
> +
> +unsafe fn find_value_in_boolean_context<F>(

Can you factor out the commonality between find_in_table and find_value_in_boolean_context?

::: servo/components/style/gecko/media_queries.rs:651
(Diff revision 1)
> +        nsMediaFeature_ValueType::eBoolEnumerated => {
> +            let location = input.current_source_location();
> +            let keyword = input.expect_ident()?;
> +            let keyword = unsafe {
> +                bindings::Gecko_LookupCSSKeyword(keyword.as_bytes().as_ptr(), keyword.len() as u32)
> +            };
> +
> +            let first_table_entry: *const nsCSSKeywordAndBoolTableEntry =
> +                unsafe { *feature.mData.mKeywordAndBoolTable.as_ref() };
> +
> +            let value = match unsafe { find_in_table(first_table_entry, |kw, _| kw == keyword) } {
> +                Some((_kw, value)) => value,
> +                None => {
> +                    return Err(location.new_custom_error(StyleParseErrorKind::UnspecifiedError))
> +                },
> +            };
> +
> +            MediaExpressionValue::BoolEnumerated(value)
> +        },

Can you share more code with the eEnumerated?
Attachment #8994143 - Flags: review?(cam) → review-

Comment 68

10 months ago
mozreview-review
Comment on attachment 8994063 [details]
Bug 1365045 - Introduce keywords for prefers-reduced-motion.

https://reviewboard.mozilla.org/r/258640/#review265938

r=me with bools instead of nsCSSKeywordAndBoolTableEntry::ValueInBooleanContext.

Comment 69

10 months ago
mozreview-review
Comment on attachment 8994063 [details]
Bug 1365045 - Introduce keywords for prefers-reduced-motion.

https://reviewboard.mozilla.org/r/258640/#review265940
Attachment #8994063 - Flags: review?(cam) → review+
(Assignee)

Comment 70

10 months ago
Thanks for the review!

(In reply to Cameron McCormack (:heycam) from comment #67)
> Comment on attachment 8994143 [details]
> Bug 1365045 - Introduce nsCSSKeywordAndBoolTableEntry.
> 
> https://reviewboard.mozilla.org/r/258760/#review265934
> 
> Looks good, but I'd like to see some code duplication reduced.
> 
> ::: layout/style/nsMediaFeatures.h:21
> (Diff revision 1)
> > +  enum ValueInBooleanContext {
> > +    eFalse,
> > +    eTrue,
> > +  };
> 
> I don't think there's much value in making this an enum.  Let's just use
> bool values.

I'd want to avoid writing boolean literal directly in the table in the next patch. Something like this;

static const nsCSSKeywordAndBoolTableEntry kPrefersReducedMotionKeywords[] = {
 { eCSS... , Style... , false },
 { eCSS... , Style... , true },
};

I am going to leave a comment at the top of the table, saying that 'the third value is the value in the Boolean Context'.

> ::: servo/components/style/gecko/media_queries.rs:573
> (Diff revision 1)
> >          current_entry = current_entry.offset(1);
> >      }
> >  }
> >  
> > +
> > +unsafe fn find_value_in_boolean_context<F>(
> 
> Can you factor out the commonality between find_in_table and
> find_value_in_boolean_context?

I am not sure but I will try it.
 
> ::: servo/components/style/gecko/media_queries.rs:651
> (Diff revision 1)
> > +        nsMediaFeature_ValueType::eBoolEnumerated => {
> > +            let location = input.current_source_location();
> > +            let keyword = input.expect_ident()?;
> > +            let keyword = unsafe {
> > +                bindings::Gecko_LookupCSSKeyword(keyword.as_bytes().as_ptr(), keyword.len() as u32)
> > +            };
> > +
> > +            let first_table_entry: *const nsCSSKeywordAndBoolTableEntry =
> > +                unsafe { *feature.mData.mKeywordAndBoolTable.as_ref() };
> > +
> > +            let value = match unsafe { find_in_table(first_table_entry, |kw, _| kw == keyword) } {
> > +                Some((_kw, value)) => value,
> > +                None => {
> > +                    return Err(location.new_custom_error(StyleParseErrorKind::UnspecifiedError))
> > +                },
> > +            };
> > +
> > +            MediaExpressionValue::BoolEnumerated(value)
> > +        },
> 
> Can you share more code with the eEnumerated?

Sure.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 75

10 months ago
mozreview-review
Comment on attachment 8994143 [details]
Bug 1365045 - Introduce nsCSSKeywordAndBoolTableEntry.

https://reviewboard.mozilla.org/r/258760/#review265944

Looks great, thanks!

::: servo/components/style/gecko/media_queries.rs:650
(Diff revision 2)
> +    match unsafe { find_in_table(first_table_entry,
> +                                 |kw, _| kw == keyword,
> +                                 |e| e.value()) } {
> +        Some(value) => Ok(value),
> +        None => Err(location.new_custom_error(StyleParseErrorKind::UnspecifiedError)),
> +    }

Nit: if we want something that looks a bit more rustfmt-y, maybe this:

let value = unsafe {
    find_in_table(first_table_entry, |kw, _| kw == keyword, |e| e.value())
};

match value {
    ...
}

::: servo/components/style/gecko/media_queries.rs:911
(Diff revision 2)
> +                        unsafe { find_in_table(
> +                                *self.feature.mData.mKeywordAndBoolTable.as_ref(),
> +                                |_kw, val| val == value,
> +                                |e| e.mValueInBooleanContext,
> +                            ).expect("Value not found in the keyword table?")
> +                        }

Nit: and:

let value = unsafe {
    find_in_table(
        *self.feature.mData.mKeywordAndBoolTable.as_ref(),
        |_kw, val| val == value,
        |e| e.mValueInBooleanContext,
    )
};
value.expect("Value not found ...");
Attachment #8994143 - Flags: review?(cam) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 80

10 months ago
Thank you Cameron, I am going to land the patches to inbound once after I confirm this try result.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c7e3099046da44195c6765890d093b2578a12b9f
(Assignee)

Updated

10 months ago
Blocks: 1477920
(Assignee)

Comment 81

10 months ago
I did sent an intent to implement to dev-platform, but it's not appeared yet.
Anyway I am going to land the patch soon.
(Assignee)

Comment 82

10 months ago
mozreview-review
Comment on attachment 8994065 [details]
Bug 1365045 - Implement prefers-reduced-motion for Windows.

https://reviewboard.mozilla.org/r/258644/#review265984

Stamping.
Attachment #8994065 - Flags: review+

Comment 83

10 months ago
Pushed by hikezoe@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4460ece14442
Introduce nsCSSKeywordAndBoolTableEntry. r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/bc2538dade14
Introduce keywords for prefers-reduced-motion. r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/786a2e4ea6ea
Reftests for prefers-reduced-motion. r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/928316e14923
Implement prefers-reduced-motion for Windows. r=jimm
(Assignee)

Updated

10 months ago
Attachment #8994065 - Flags: review?(jmathies)
(Assignee)

Updated

10 months ago
Depends on: 1478158
(Assignee)

Updated

10 months ago
Blocks: 1478505
(Assignee)

Updated

10 months ago
Blocks: 1478519

Updated

10 months ago
Blocks: 1478597
Depends on: 1479239

Comment 85

9 months ago
I have written the documentation for this feature. 

https://developer.mozilla.org/en-US/docs/Web/CSS/@media/prefers-reduced-motion

I am waiting for two PRs to be merged and pushed to MDN in order that the spec data and compat data will be linked:

https://github.com/mdn/browser-compat-data/pull/2700
https://github.com/mdn/kumascript/pull/795

The above PRs will also update the two tables on: https://developer.mozilla.org/en-US/docs/Web/CSS/@media

I would appreciate a review - thanks!
Flags: needinfo?(hikezoe)

Updated

9 months ago
(Assignee)

Comment 86

9 months ago
Thank you, Rachel. Nice example. I love it. :)
Flags: needinfo?(hikezoe)

Comment 87

6 months ago
Coming in late on this, but for Windows, this has led to some surprised users who, like myself, used the "Ease of Access > Show animations in Windows" to turn off animations in Windows only (i.e. the visual effects when an app window get maximized/minimized), but not necessarily as an indication that they don't want any animations at all in apps.

Arguably, this is a problem with Windows' settings, which should offer two separate settings: turn off animations in Windows itself, and indicate that overall I would like reduced/no animations at all across all apps - in fact, this is the case in macOS, which has Settings > Dock > Animate opening applications, distinct from Settings > Accessibility > Display > Reduce motion. But in the meantime, is there perhaps a way to offer an additional setting/override in Firefox itself?
(In reply to Patrick H. Lauke from comment #87)
> Arguably, this is a problem with Windows' settings, which should offer two
> separate settings: turn off animations in Windows itself, and indicate that
> overall I would like reduced/no animations at all across all apps - in fact,
> this is the case in macOS, which has Settings > Dock > Animate opening
> applications, distinct from Settings > Accessibility > Display > Reduce
> motion. But in the meantime, is there perhaps a way to offer an additional
> setting/override in Firefox itself?

You should be able to manually set the ui.prefersReducedMotion pref to 0 from about:config.

Comment 89

6 months ago
sorry, i meant a setting for mere mortals/regular users, not something to change in about:config
context for the above, btw: https://twitter.com/patrick_h_lauke/status/1056179985025773569 / https://twitter.com/patrick_h_lauke/status/1056492294826811392
See Also: → 1506218
(In reply to Patrick H. Lauke from comment #89)
> sorry, i meant a setting for mere mortals/regular users, not something to
> change in about:config

Filed bug 1506218 for that.

Comment 91

6 months ago
(In reply to Patrick H. Lauke from comment #87)
> Coming in late on this, but for Windows, this has led to some surprised
> users who, like myself, used the "Ease of Access > Show animations in
> Windows" to turn off animations in Windows only (i.e. the visual effects
> when an app window get maximized/minimized), but not necessarily as an
> indication that they don't want any animations at all in apps.
> 
> Arguably, this is a problem with Windows' settings, which should offer two
> separate settings: turn off animations in Windows itself, and indicate that
> overall I would like reduced/no animations at all across all apps
[snip]

Windows does offer those separate settings. https://m.wikihow.com/Disable-Animations-in-Windows-10

Still I agree there's an issue with the label "Show animations in Windows". It would be much clearer to add "and some applications".

Comment 92

6 months ago
Mitchell,

Ah, you're right...Windows does seem to have a more granular distinction between turning off animations specific to Windows (the window manager) itself and the global setting. The former are still tucked away inside the old-style Control Panel > System Properties > Advanced > Performance Options > Visual Effects. I had forgotten about those / assumed that in the new Windows 8/10 "metro" transition that those were meant to be covered by the one option in settings.  To an extent then, this problem is a combination of this sort of option not being available in the new-style Settings, and the label in Settings "Show animations in Windows" maybe not being clear enough that that setting applies to *all* apps, not just Windows-specific animations.

For whatever little good that might do, I've submitted the above via the Microsoft Feedback Hub.

Comment 94

6 months ago
Actually, I would prefer that I only have to find one setting to turn of all unrequested animation. Animation bothers me without regard to who is providing it. The only time I want to see animation is if I click a Play Animation button for a specific instance of animation, to see that animation one time.

If there is to be granularity, it belongs in a single place, not scattered across the OS and apps for me to have to hunt it down.

The standard, as apparently set by Safari and Firefox 63, is to use that one setting, Reduce Animation (in MacOS or iOS) or Show animation in Windows (Windows 10). Ideally, a change to that would set all animations on or off, and then if there was any granularity you could then go in and override individual settings.

That said, how Windows or MacOS preferences are set up is outside the scope of this bug. I am happy with the way this bug was implemented.
(Reporter)

Comment 95

6 months ago
Clarifying again because the intention has been misrepresented a few times as a user preference to stop *all* animation.

The Mac and iOS settings don’t turn off animations, because switching off all animations would likely result in many confusing user interfaces. The setting is a specific accessibility accommodation for vestibular disorders resulting in a hypersensitivity to certain types of motion. Common symptoms include motion-induced nausea. The setting allows developers to remove any extraneous animation, or create alternate animations for any animations that confer meaningful changes in the user interface. Most of the changes on macOS and iOS alternate animations, rather than entirely removed animations.

As mentioned in the original WebKit blog post, I’d encourage you to consider each specific animation in context. Those that are known to be problematic motion triggers should be removed or changed. Those that are not known to be problematic could be left alone until you know otherwise. 

Use good judgement. Don’t overdo it. Cheers.
(Reporter)

Comment 96

6 months ago
Two examples where macOS and iOS have more than one setting for motion: 

• auto play videos is a stand-alone Safari setting on Mac 
• auto play message effects is a stand-alone setting on iOS.

Both of these were detemined to not overlap directly with the vestibular setting, so settings were added to account for customizations outside the accessibility-specific use case.
You need to log in before you can comment on or make changes to this bug.