Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Modify reduce-motion mixin so that it works for transitions too. #15850

Merged
merged 6 commits into from Jun 3, 2019

Conversation

@kjellr
Copy link
Contributor

commented May 27, 2019

This PR expands the reduce-motion mixin created in #14021 to account for transition-based motion as well. props to @m-e-h for the suggestion.

This PR is a blunt instrument in its current state — I've added a transition-duration: 0s !important rule to the mixin and applied it to every single place we're using transition. It seems to work just fine like that, but I'm not sure that's actually the best way to handle this. Some other options:

  1. Break this into a second mixin. There's no need to include both override rules in cases when only one property is being used.
  2. Create mixins to act as standins for the transition and animation properties. This would theoretically allow us to stop using !important, too. (Example)
  3. If we're using !important all over the place anyway, maybe we should switch to using a single global declaration for this media query, rather than placing this in a bunch of different places at once. The downside of this is the chance that the rules cause issues in some places, and go undetected because people aren't testing this option.

Any thoughts are greatly appreciated!

@@ -340,6 +341,7 @@
@mixin reduce-motion {
@media (prefers-reduced-motion: reduce) {
animation-duration: 1ms !important;
transition-duration: 0s !important;

This comment has been minimized.

Copy link
@jasmussen

jasmussen May 28, 2019

Contributor

Why is this 01 when the animation duration is 1ms? Should they both be 1ms?

This comment has been minimized.

Copy link
@kjellr

kjellr May 28, 2019

Author Contributor

In the original PR, I noticed some Safari issues when animation-duration was set to 0s: I couldn't get it to reliably render the end-state, which was preventing menus and panels from opening. 😕

I haven't noticed the same issues with transition for some reason, so I think it's fine to use 0s there.

@jasmussen

This comment has been minimized.

Copy link
Contributor

commented May 28, 2019

This is very clever, and very welcome. It seems to work for me, so 👍 👍 for this. I left a small question, so I could use a sanity check on that one. But otherwise, nice work!

@m-e-h

This comment has been minimized.

Copy link
Member

commented May 28, 2019

Oh yeah, I meant to get back to this. Glad you remembered 😄

  1. Break this into a second mixin. There's no need to include both override rules in cases when only one property is being used.

This makes sense but including both in a single mixin keeps things simple.
"Use this mixin for prefers-reduced-motion"

  1. Create mixins to act as standins for the transition and animation properties. This would theoretically allow us to stop using !important, too. (Example)

This is a cool and efficient idea!

  1. If we're using !important all over the place anyway, maybe we should switch to using a single global declaration for this media query, rather than placing this in a bunch of different places at once. The downside of this is the chance that the rules cause issues in some places, and go undetected because people aren't testing this option.

This is kinda what I was thinking.

@media (prefers-reduced-motion: reduce) {
  * {
    animation-duration: 0.001s !important;
    transition-duration: 0.001s !important;
  }
}

More and more, I think we'll start seeing that as a staple for most websites. Similar to how we see the "global" box-sizing: border-box everywhere today.
https://github.com/mozdevs/cssremedy/blob/master/remedy.css#L11-L16

https://developers.google.com/web/updates/2019/03/prefers-reduced-motion#bonus_forcing_reduced_motion_on_all_websites

I agree that it feels careless to use a global like that but I honestly can't think of one situation where it would cause a problem. It might actually be less problematic since it blankets everything rather than needing to be meticulously added to individual rules.

pointer-events: none;
transition: border-color 0.1s linear, box-shadow 0.1s linear;
@include reduce-motion;

This comment has been minimized.

Copy link
@m-e-h

m-e-h May 28, 2019

Member

So how does SASS compile this? Is the @media query always before or after the original rule?
Maybe it's not consistent enough to count on but I'm wondering if !important is even necessary if the @media comes after...

This comment has been minimized.

Copy link
@kjellr

kjellr May 29, 2019

Author Contributor

Well theoretically the compiled order shouldn't matter, right? Since the added media query adds another level of specificity that would usually override the existing rule.

With that in mind, I the !important only becomes necessary if someone happened to add a competing transition-duration property for the same element, later on in the stylesheet and more specific than this one. Probably rare?

This comment has been minimized.

Copy link
@m-e-h

m-e-h May 29, 2019

Member

Well theoretically the compiled order shouldn't matter, right? Since the added media query adds another level of specificity that would usually override the existing rule.

That's what I thought too until a couple days ago. According to the spec, media queries don't add any weight.
Maybe some browsers don't follow that cause I could swear I've seen otherwise. 😕

I don't think !important is that out of line here. Might as well leave it.

This comment has been minimized.

Copy link
@kjellr

kjellr May 30, 2019

Author Contributor

I tried removing it in 5350c8e, and haven't found any instances where these rules are ignored yet. 🤷‍♂️

Seeing as that's the case, I think we might as well keep it out until we determine we definitely need it.

@kjellr

This comment has been minimized.

Copy link
Contributor Author

commented May 29, 2019

I agree that it feels careless to use a global like that but I honestly can't think of one situation where it would cause a problem. It might actually be less problematic since it blankets everything rather than needing to be meticulously added to individual rules.

Totally agree. I'm mostly scared of the possibility that this breaks something important, and it isn't reported and fixed quickly because people aren't frequently testing with this setting turned on. 😞

For instance, the 0.1ms or 0.001s setting recommended in those reference links would actually generate a highly choppy, problematic result if they were applied to an animation that doesn't use animation-fill-mode: forwards; to resolve on an end state.

I guess for now, I'll look into breaking these out into two separate animation and transform mixins for this PR. Unless you two think the stand-in mixins is a better approach?

@m-e-h

This comment has been minimized.

Copy link
Member

commented May 29, 2019

would actually generate a highly choppy, problematic result

Good point. Change one of the examples at the top of this page to 1ms https://developer.mozilla.org/en-US/docs/Web/CSS/animation-duration
😆 That is far from reduced motion 😵

So yeah, individual mixins would probably be best.
The standins is a good idea but since we're dealing with two different properties, each with it's own long set of $args, it may not be worth it. It would also force us to always use the shorthand property.

@kjellr

This comment has been minimized.

Copy link
Contributor Author

commented May 30, 2019

The idea of two separate mixins seemed a little confusing to me, so I tried out a slightly different idea. Let me know if this is too obscure of a method:

We'd keep one mixin, that'd accept two different values, depending on the type of motion property:

  • @include reduce-motion("animation");
  • @include reduce-motion("transition");

If someone were to leave the property variable empty, we'd fall back to including both. This seems pretty flexible to me, and keeps these overrides together in one place. Here's how it's done:

@mixin reduce-motion($property: "") {
@if $property == "transition" {
@media (prefers-reduced-motion: reduce) {
transition-duration: 0s;
}
}
@else if $property == "animation" {
@media (prefers-reduced-motion: reduce) {
animation-duration: 1ms;
}
}
@else {
@media (prefers-reduced-motion: reduce) {
transition-duration: 0s;
animation-duration: 1ms;
}
}
}

We use somewhat similar mixins for z-index and long-content fades. Let me know your thoughts!

@m-e-h

This comment has been minimized.

Copy link
Member

commented May 30, 2019

We'd keep one mixin, that'd accept two different values

That's perfect!
I wasn't crazy about the touch of complexity that two separate mixins for reduced motion might add. This solves that! 🎉

@jasmussen jasmussen self-requested a review Jun 3, 2019

@jasmussen
Copy link
Contributor

left a comment

Thanks, for this.

Code looks good, and feedback from Marty seems to be mostly addressed. What unknowns that may still be present can surely be addressed in subsequent PRs if need be.

The documentation is also already in place: https://developer.wordpress.org/block-editor/designers/animation/#accessibility-considerations

Nothing comes to mind as to what we can do, more than this, to advise third parties for how to best design with this in mind.

I also took a quick spin to verify the behavior:

cpanel

With animation reduced:
with enabled

@kjellr

This comment has been minimized.

Copy link
Contributor Author

commented Jun 3, 2019

Thanks folks! Happy to get this merged in.

Also, an extra kudos to @ryelle for bringing this up originally over in #8029 (comment). 🎉

@kjellr kjellr merged commit bd76217 into master Jun 3, 2019

1 check passed

Travis CI - Pull Request Build Passed
Details

@kjellr kjellr deleted the try/reduce-motion-for-transitions branch Jun 3, 2019

@youknowriad youknowriad added this to the Gutenberg 5.9 milestone Jun 7, 2019

nicolad added a commit to nicolad/gutenberg that referenced this pull request Jun 15, 2019

Modify reduce-motion mixin so that it works for transitions too. (Wor…
…dPress#15850)

* Add `transition` to the reduce motion mixin.

* Add reduce motion mixin everywhere we're using transitions.

* Modify the reduce-motion mixin to support transition and/or animation.

* Specify whether to inclue animation or transition properties for the reduce-motion mixin instances.

* Remove unnecessary !importants

jg314 added a commit to jg314/gutenberg that referenced this pull request Jul 19, 2019

Modify reduce-motion mixin so that it works for transitions too. (Wor…
…dPress#15850)

* Add `transition` to the reduce motion mixin.

* Add reduce motion mixin everywhere we're using transitions.

* Modify the reduce-motion mixin to support transition and/or animation.

* Specify whether to inclue animation or transition properties for the reduce-motion mixin instances.

* Remove unnecessary !importants
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.