James Craig Reporter
	Description • 7 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/

	James Craig Reporter
	Comment 1 • 7 years ago

Also filed to Chrome: 
https://bugs.chromium.org/p/chromium/issues/detail?id=722548

	James Craig Reporter
	Comment 2 • 7 years ago

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

	James Craig Reporter
	Comment 3 • 7 years ago

Also field against Edge:
https://wpdev.uservoice.com/forums/110705-universal-windows-platform/suggestions/19291531-add-support-for-css-prefers-reduced-motion-media-f

	Alastair Campbell
	Comment 4 • 7 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).

	Marco Zehe (:MarcoZ)
	Comment 5 • 7 years ago

CC'ing David, accessibility team lead.

	github
	Comment 6 • 7 years ago

Ohh this would be great to have in Firefox!

	David MacDonald
	Comment 7 • 7 years ago

I vote for this

	nattarnoff
	Comment 8 • 7 years ago

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

	Laura Carlson
	Comment 9 • 7 years ago

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

+1

	Paul Bohman
	Comment 10 • 7 years ago

I'd like to see this supported

	Jen Simmons [:jensimmons]
	Comment 11 • 7 years ago

This is important. We're going to want to implement this sooner rather than later.

	Denis Boudreau
	Comment 12 • 7 years ago

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

	u599908
	Comment 13 • 7 years ago

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

	Lisa Caniglia
	Comment 14 • 7 years ago

Please prioritize, this is an important accessibility feature.

	hannahnotess
	Comment 15 • 7 years ago

This would be a great accessibility feature to add.

	David Bolter [:davidb] (NeedInfo me for attention)
	Comment 16 • 7 years ago

Jet do you know how much work this would take?

	Jet Villegas (inactive)
	Comment 17 • 7 years ago

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

	Brian Birtles (:birtles, Feb 26~ parental leave)
	Comment 18 • 7 years ago

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.

	Brian Birtles (:birtles, Feb 26~ parental leave)
	Comment 19 • 7 years ago

Added it to my list of things to prioritize in 2018.

	Hiroyuki Ikezoe (:hiro) Assignee
	Comment 20 • 6 years 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

	Hiroyuki Ikezoe (:hiro) Assignee
	Comment 21 • 6 years 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.

	Hiroyuki Ikezoe (:hiro) Assignee
	Comment 22 • 6 years 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*.

	Mike Gifford
	Comment 23 • 6 years 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

	Maire Reavy [:mreavy]
	Comment 24 • 6 years ago

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!

	Hiroyuki Ikezoe (:hiro) Assignee
	Comment 25 • 6 years 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

	James Teh [:Jamie]
	Comment 26 • 6 years 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.

	Brian Birtles (:birtles, Feb 26~ parental leave)
	Comment 27 • 6 years ago

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

?

	James Teh [:Jamie]
	Comment 28 • 6 years 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.)

	Hiroyuki Ikezoe (:hiro) Assignee
	Comment 29 • 6 years 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

	Hiroyuki Ikezoe (:hiro) Assignee
	Comment 30 • 6 years ago

I am going to use this bug only for Windows, filed a new bug for MacOSX (bug 1475462).

	James Teh [:Jamie]
	Comment 31 • 6 years 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.

	Hiroyuki Ikezoe (:hiro) Assignee
	Comment 32 • 6 years 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

	James Teh [:Jamie]
	Comment 33 • 6 years 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

	Hiroyuki Ikezoe (:hiro) Assignee
	Comment 34 • 6 years 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

	James Teh [:Jamie]
	Comment 35 • 6 years 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.

	Hiroyuki Ikezoe (:hiro) Assignee
	Comment 42 • 6 years 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.

	Hiroyuki Ikezoe (:hiro) Assignee
	Comment 43 • 6 years 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.

	Jim Mathies [:jimm]
	Comment 44 • 6 years ago
mozreview-review

Comment on attachment 8992533 [details]
Bug 1365045 - Implement prefers-reduced-motion for Windows.

https://reviewboard.mozilla.org/r/257384/#review265206

	Cameron McCormack (:heycam)
	Comment 45 • 6 years 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.

	Cameron McCormack (:heycam)
	Comment 46 • 6 years 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.

	Hiroyuki Ikezoe (:hiro) Assignee
	Comment 47 • 6 years 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).

	Hiroyuki Ikezoe (:hiro) Assignee
	Comment 52 • 6 years ago

My local settings for MozReview seem to be somehow broken...

	Hiroyuki Ikezoe (:hiro) Assignee
	Comment 57 • 6 years ago

Adding the previous MozReviewId made MozReview confusing. :/

	Cameron McCormack (:heycam)
	Comment 58 • 6 years ago

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.

	Cameron McCormack (:heycam)
	Comment 59 • 6 years ago
mozreview-review

Comment on attachment 8994064 [details]
Bug 1365045 - Reftests for prefers-reduced-motion.

https://reviewboard.mozilla.org/r/258642/#review265584

	Hiroyuki Ikezoe (:hiro) Assignee
	Comment 60 • 6 years 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.

	Cameron McCormack (:heycam)
	Comment 61 • 6 years ago

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

	Hiroyuki Ikezoe (:hiro) Assignee
	Comment 62 • 6 years 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!

	Cameron McCormack (:heycam)
	Comment 67 • 6 years 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?

	Cameron McCormack (:heycam)
	Comment 68 • 6 years 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.

	Cameron McCormack (:heycam)
	Comment 69 • 6 years ago
mozreview-review

Comment on attachment 8994063 [details]
Bug 1365045 - Introduce keywords for prefers-reduced-motion.

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

	Hiroyuki Ikezoe (:hiro) Assignee
	Comment 70 • 6 years 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.

	Cameron McCormack (:heycam)
	Comment 75 • 6 years 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 ...");

	Hiroyuki Ikezoe (:hiro) Assignee
	Comment 80 • 6 years 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

	Hiroyuki Ikezoe (:hiro) Assignee
	Comment 81 • 6 years 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.

	Hiroyuki Ikezoe (:hiro) Assignee
	Comment 82 • 6 years ago
mozreview-review

Comment on attachment 8994065 [details]
Bug 1365045 - Implement prefers-reduced-motion for Windows.

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

Stamping.

	Pulsebot
	Comment 83 • 6 years 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

	Raul Gurzau (:RaulG)
	Comment 84 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/4460ece14442
https://hg.mozilla.org/mozilla-central/rev/bc2538dade14
https://hg.mozilla.org/mozilla-central/rev/786a2e4ea6ea
https://hg.mozilla.org/mozilla-central/rev/928316e14923

	Rachel Andrew
	Comment 85 • 6 years 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!

	Hiroyuki Ikezoe (:hiro) Assignee
	Comment 86 • 6 years ago

Thank you, Rachel. Nice example. I love it. :)

	Patrick H. Lauke
	Comment 87 • 6 years 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?

	Emilio Cobos Álvarez (:emilio)
	Comment 88 • 6 years ago

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

	Patrick H. Lauke
	Comment 89 • 6 years 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

	Emilio Cobos Álvarez (:emilio)
	Comment 90 • 6 years ago

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

	Mitchell Evan
	Comment 91 • 6 years 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".

	Patrick H. Lauke
	Comment 92 • 6 years 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.

	Patrick H. Lauke
	Comment 93 • 6 years ago

https://aka.ms/AA363th

	Charles Belov
	Comment 94 • 6 years 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.

	James Craig Reporter
	Comment 95 • 6 years 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.

	James Craig Reporter
	Comment 96 • 6 years 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.