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

Support for prefers-reduced-motion #8494

Merged
merged 12 commits into from
Jul 19, 2019
Merged

Conversation

arindam1993
Copy link
Contributor

@arindam1993 arindam1993 commented Jul 16, 2019

  • briefly describe the changes in this PR
  • document any changes to public APIs
  • manually test the debug page

Addresses #8288.
Defaults the animation duration to 0 when prefer-reduced-motion is set to reduce.

How to set this on your local machine:
https://developers.google.com/web/updates/2019/03/prefers-reduced-motion

Copy link
Member

@mourner mourner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This generally looks good, but with a few considerations:

  • With this change, animation will never happen with prefersReducedMotion even if the developer sets it explicitly. Should we instead only set the default duration to 0, while any explicitly set duration is respected? Not fully sure about this, @CharlesBelov maybe you have thoughts on this?
  • Does this need an equivalent change in flyTo?

@@ -19,6 +19,8 @@ const cancel = window.cancelAnimationFrame ||

let linkEl;

let _reducedMotionQuery: MediaQueryList;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: maybe remove the dangling _ in the name for consistency? This will be private anyway since it's not exposed.

@CharlesBelov
Copy link

CharlesBelov commented Jul 17, 2019

@mourner: Actually, that is the whole point of prefersReducedMotion, that the developer can use as much animation as they want and the person who doesn't want to see animation will never see it but others who are okay with animation will see it.

So, no, it's not appropriately a default, it is an override, similar to using !important on a user CSS style sheet.

As for the flyTo question, my original issue specifically mentioned:

What parts of the Mapbox GL ecosystem will need to change to accommodate this design?

flyTo, jumpTo, and easeTo

so if flyTo is missing from this change then the change is incomplete.

@arindam1993
Copy link
Contributor Author

Thanks for the quick review @mourner !
I interpreted the desired behavior to be as @CharlesBelov mentioned, a global force-disabling of all animations.

I added the change to flyTo as well!

@andrewharvey
Copy link
Collaborator

@mourner: Actually, that is the whole point of prefersReducedMotion, that the developer can use as much animation as they want and the person who doesn't want to see animation will never see it but others who are okay with animation will see it.

So, no, it's not appropriately a default, it is an override, similar to using !important on a user CSS style sheet.

I agree, the whole point is a developer can build with full animations and if the user asks for no animations via prefersReducedMotion, they don't need to do any checks themself, mapboxgl will automatically comply with the users request.

Copy link
Member

@mourner mourner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the explanations, agreed! I'm 👍 on this, pending a small nit and a green build (fixing flow errors).

src/ui/camera.js Outdated
pitch: options.pitch,
around: options.around
}, eventData);
return;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be return this (or return this.jumpTo(...)) for consistency/chaining.

@arindam1993
Copy link
Contributor Author

Thanks @mourner ! changes made 👍

Copy link
Contributor

@asheemmamoowala asheemmamoowala left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a way to add a unit/render test for this?

src/ui/camera.js Outdated
@@ -857,6 +857,17 @@ class Camera extends Evented {
* @see [Fly to a location based on scroll position](https://www.mapbox.com/mapbox-gl-js/example/scroll-fly-to/)
*/
flyTo(options: Object, eventData?: Object) {
// Fall throwugh to jumpTo is user has set prefers-reduced-motion
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: typos

@arindam1993
Copy link
Contributor Author

Is there a way to add a unit/render test for this?

I think I can write a test and model it after the can be called from within a moveend event handler in camera.test.js, it does asynchronous timing based things as well.

@arindam1993
Copy link
Contributor Author

Added unit tests 👍

@CharlesBelov
Copy link

Possibly one more typo:
// Fall through to jumpTo is user has set prefers-reduced-motion
might correctly be
// Fall through to jumpTo if user has set prefers-reduced-motion

Copy link
Contributor

@asheemmamoowala asheemmamoowala left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good to go, just a last typo left.

@@ -869,6 +881,14 @@ test('camera', (t) => {
}, 0);
});

t.test('duration is 0 when prefers-reduced-motions: reduce is set', (t) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: typo motions

@dieterdreist
Copy link

I agree it can be very consistent to remove flyTo and easeTo animations, in certain circumstances (particularly, rather short animations, "eye gimmicks", when the animations are not required for functional reasons.
But if you use the animations as integral part of your app (e.g. to fly somewhere in many seconds, to give the user an overview over a bigger area with an automatic flight...), then this will break the app.

@andrewharvey
Copy link
Collaborator

But if you use the animations as integral part of your app (e.g. to fly somewhere in many seconds, to give the user an overview over a bigger area with an automatic flight...), then this will break the app.

But still the user has requested reduced motion, I would think that means no more than 1 frame per second. What about just reducing the animation to larger one second steps? That avoids animations but still gives the user a view of the the fly operation?

@dieterdreist
Copy link

"reduced motion" is not about frames per second. It is about reducing "useless" motion (eye candy).

If you activate the function, would you expect your videos to playback with 1 frame per second?

Did you notice how Apple values this setting? They use less "effects" (e.g. no parallax effects or icon animations on the home screen, or simpler text animations in the keyboard), but some UI elements like the drawers for notifications (from top) or settings (from bottom) animate just the same. The latter don't "jump" or pop up instantly, they smoothly move into the screen as if you hadn't requested reduced motion.

I had activated the reduced motion, because it offered my a cleaner and less distracting experience while supposedly also saving on battery. I read this often as advice in the web, it is not just a feature for people with visual problems. Core functionality should keep working.

If you still believe activating "reduced motion" means that any flyTo and easeTo animation in mapbox-gl must be converted into a jumpTo, then please make this customizable / a setting.

@andrewharvey
Copy link
Collaborator

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

Indicates that user has notified the system that they prefer an interface that minimizes the amount of movement or animation, preferably to the point where all non-essential movement is removed.

My interpretation of this is it depends on the app, if you're building an animation with a slow ease fly to different locations, then I could consider that essential as it's core to the experience or app, if you're building an app where you search for a location and the map flys to that location, then I could consider that animation non-essential as you can just skip straight to the destination. Sure the animation is nicer and is a better UX, but still non-essential.

With that in mind it could be we expose some kind of option so the app developer can say if this transition is considered essential or not?

The smooth zoom in you get when you click the navigation zoom in button is another example, I would classify as non-essential and should respect prefers-reduced-motion.

@CharlesBelov
Copy link

In short, the three issues that are covered by this are:

  1. Motion sickness
  2. Attention disruption
  3. Battery life

Distinguishing between essential motion and non-essential motion:

  1. User has requested a different view - non-essential
  2. User has requested an animation going from this view to that view - essential

So, a developer might offer two options:

  1. Go to point B
  2. Fly from point A to point B

The problem becomes if the developer doesn't give a choice and assumes the user wants the fly option.

So, yes, it would be fine if the developer can specify that a particular animation is essential, but it would not properly be the default and there needs to be a non-animating option.

As for the Apple UX drawer slide, it is non-essential, so I view it as a failure in implementation.

@andrewharvey
Copy link
Collaborator

See the proposal at #8743.

@CharlesBelov
Copy link

I have an update to my immediately previous comment.

Actually, with iOS 13, Apple has now added a separate preference, Settings > Accessibility > Motion > Prefer cross-fade transitions, described as "Reduce the motion for user interface controls that slide in when appearing and disappearing."

I'm not sure how they expose this to applications or how applications would in turn expose this to websites.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants