Closed didoo closed 2 years ago
vercel[bot]
commented
2 years ago This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.
🔍 Inspect: https://vercel.com/hashicorp/flight/9TiKQcjvJbghv5KiM2n7UzgBNaHu
✅ Preview: https://flight-git-282-loading-and-run-animated-icons-hashicorp.vercel.app
MelSumner
commented
2 years ago @didoo I see your point that the animation has a purpose.
Did you try to slow down the animation? That would still respect a reduced animation preference without removing it entirely. That could be an appropriate compromise and I believe it would still meet accessibility criteria.
didoo
commented
2 years ago @didoo I see your point that the animation has a purpose.
👍
Did you try to slow down the animation? That would still respect a reduced animation preference without removing it entirely. That could be an appropriate compromise and I believe it would still meet accessibility criteria.
Good idea! Didn't think of it :) Let me try and see if it works
didoo
commented
2 years ago @MelSumner nope, doesn't work (I suspect because these are SMIL animations, and can't be controlled via CSS). See codepen here: https://codepen.io/didoo/pen/yLzrKxL?editors=1100
MelSumner
commented
2 years ago Ok, I will do some research. We should not use this approach if we cannot make it respect the prefers-reduced-motion flag, this will be particularly troublesome for folks with attention or vestibular issues.
I will do some more digging and see if there is an accessible way to do this. 👍
didoo
commented
2 years ago @MelSumner this is the problem, so you have more context.
At the moment we have 3 different formats in output:
FlightIcon Ember addon/components and used in the Ember product applicationsNow, how do we apply animations to all of them?
If we use CSS, we can control the CSS of the FlightIcon (format 1) but not of the inlined SVGs (formats 2 and 3); this means not only that we are asking the marketing team to implement their own animations (so they would not be provided by the system) but we should also find a way to share the animation specs (in particular the duration value) between the different platforms (we can use design tokens).
That's why I went with the second option, use SMIL/SVG animation: because in this case the animation is "embedded" inside the icons, consumers just need to use the icon and that's it, it works out of the box without them worrying to implement custom animations.
Potentially we could decide to split the output, have the SMIL/SVG animation for marketing websites, and CSS animation for product applications, but in this case we add some extra complexity to the generation pipeline, but also would not solve the problem for them.
My thinking, as I told you before, is that this is one of the cases in which making an exception makes sense. The reasons I see are these:
PS: I know is a delicate matter, don't take mine as insensibility towards the accessibility issue.
MelSumner
commented
2 years ago @didoo let's get @ashleemboyer to weigh in on this, too.
Thinking about more options:
Also maybe Ashlee has some other ideas.
didoo
commented
2 years ago @didoo let's get @ashleemboyer to weigh in on this, too.
Good idea.
- ask the folks who are consuming the raw SVGs to add a CSS class (like they would do for something like typography)
We should consider also the React/SVG component.
- is there a way to pre-ship a css class on the SVG from Figma?
Potentially, yes.
ashleemboyer
commented
2 years ago Hey y'all! Thanks for the tag. I'm just getting out of all-day MKO activities and it's after 6pm where I am. I'll write up a response tomorrow morning. 😊
ashleemboyer
commented
2 years ago Alright, I've gone through the description and comments on this PR, checked out the linked codepen, and have done a little research as well. Ready to give my take on this. 😊
As someone that is negatively impacted by animations on a frequent (but not constant) basis, I have to say that I think it's absolutely necessary to make animations work with prefers-reduced-motion. Accessibility is about empowering users to make their own choices for their needs--we all have different needs and those needs can even fluctuate on a day-to-day basis. Device settings like prefers-reduced-motion are really important for enabling users to customize their experience to fit their needs.
As for how we can make this work with prefers-reduced-motion, I'd like to propose that a CSS file is distributed alongside the base icons without animations being applied to them. In the CSS file, there could be classnames defined for each kind of animation. In this case, maybe spin is a good name. For each animation class a media query could be applied for prefers-reduced-motion that either slows the animation or turns it off completely. This would enable us to import the CSS file where we need (probably our main/global CSS file for a project), and then we could apply a provided animation classname inline. This is the experience I've had with FontAwesome in the past and it's a smooth experience. They have an Animating Icons doc that gets into the details. This approach also seems like it would be less maintenance on y'alls part, and you wouldn't have to do a release each time an animated icon needed to be added.
I think another topic here is what should happen in the prefers-reduced-motion media query. The options are to slow an animation or turn it off completely. I'd like to push for the latter. The animations on these icons doesn't feel critical to me if they are accompanied by text like "loading..." or "running...". I don't have all the context yet for how these icons are intended to be used though, are there designs I could view to get that context? Or maybe documentation where intended use has been discussed?
didoo
commented
2 years ago @MelSumner @ashleemboyer as anticipated yesterday to Melanie, I have pushed the last changes to use CSS animations instead of SVG/SMIL animations for the loading and running icons. It works, as you can see below:
That said, I am still not 100% sure this is what we should to. Both solutions have pros and cons, and I'll try to list them below (from my point of view). Both solutions have been implemented, I can revert to a previous commit in git (SVG/SMIL) or I can leave things as they are now (CSS). But I'll leave to you the last call. Let me know what you think, thanks.
prefers-reduced-motion (question: shouldn't be the user agent to simply disable SMIL animations, in this case?)prefers-reduced-motion
Notice: I am totally aware that choosing SMIL over CSS means favouring the Dev Experience vs the User Experience, but quoting Melanie:
Our goal is quality work that pragmatically meets the highest level of accessibility as possible.
and I think the key word in this case is "pragmatic". My opinion is that in this very specific case, the pragmatic choice is to go with SMIL. But it's my opinion, and in a Cap Watkins scale I am on a 3 out of 10 on this, so as I said above your opinion prevails on mine.
PS: @ashleemboyer if you decide to go with the CSS implementation, can you check the documentation for React? I tested locally using DotDev, so the code I added in the snippets is the one that worked there, but maybe I missed something or something is not clearly explained. Thanks
johncowen
commented
2 years ago 👋
Will work out of the box also in the custom Consul implementation of the icons (see ui: Alias all our Structure Icons to Flight Icons consul#12209) - cc @johncowen
Not sure what's involved in the SMIL impl. but guessing it would work? I'd prefer to use CSS though.
Will not work out of the box in the custom Consul implementation of the icons
I just gave this a quick go with some CSS:
Guessing if you go with a CSS route we can use it also, but I think I'm easy with whatever you do here as long as I can use an SVG with CSS.
P.S. Oh i forgot to say, I would imagine we'll have this as a user setting in our settings page along with theme, so it will work with @media query being set, and/or just the user selecting "reduced motion" from our settings page also.
johncowen
commented
2 years ago why are we doing something different for Consul?
@MelSumner as far as I'm aware you are not doing something different for Consul, we are fine with what is here. We take a similar approach to Boundary and just use the svg icons. I can't really speak for Boundary but this works for all of us and is something I was very loud about from the outset that we "just wanted some svg icons", nothing overly complicated.
Please make sure you include me in your team discussion if one happens.
P.S.
I'm easy with whatever you do here as long as I can use an SVG with CSS.
didoo
commented
2 years ago Not sure what's involved in the SMIL impl. but guessing it would work? I'd prefer to use CSS though.
@johncowen look at this commit, you will see how all the SVGs (SVG standalone, SVG React, SVG sprite) have an <animateTransform> embedded: https://github.com/hashicorp/flight/pull/383/commits/94aa462a5eff7ca216215ab5e1919c7ca4772fc6
This means that you would have received animated icons, and you would have nothing to do on your side.
I just gave this a quick go with some CSS:
@johncowen of course you can do it, but if in the future we have change an animation parameter (eg the duration), you will have to do the same (and someone will need to remember to do it/tell you); same if we add a new animated icon.
it will work with @media query being set, and/or just the user selecting "reduced motion" from our settings page also.
As it is now, for the "targets" supported, will work with the prefers-reduced-motion media query. In your case you will have to implement it yourself.
- why are we doing something different for Consul?
@MelSumner see JC response. we're not doing something different, we're just taking in account a consumer that is using the SVG standalone icons in a slightly different way than other consumers.
- animated icons are small and are also informative, so slowing them down for
prefers-reduced-motionis better than turning the animation all the way off (in these cases, users are not receiving feedback that the loading is still occurring, since a non-animated state is the same as a frozen state). We should document this information for consistency and so if it comes up later we can discuss with context.
Not sure if you're proposing to add an extra media query (for when the user sets the prefers-reduced-motion) or just to document that we completely stop the animation. If it's the first, please sync with @cveigt and decide the "slow" durations and I'll update the CSS accordingly.
- If consuming teams want to add extra options in their user menus, the CSS is easy enough to change/override. So I don't think we are blocking anyone there.
No, in theory consumers should not mess with the animation of the icons. @cveigt has done the work of fine-tuning these values (and the icons), I think we should consider (and respect) their decisions.
- I am still not sure why we are shipping React components (vs providing CSS and allowing them to build their own as they wish), since I understood previously that they would just be consuming the tokens and/or stylesheets and writing their own components, and the design system itself would be shipping components for product use (in Ember).
Two things.
- Let's revisit why we needed to include cheerio- we worked really hard to get jQuery out of things and I would like to see us avoid re-introducing.
Sorry, but these are two completely different cases. JQuery is for DOM manipulation in a browser. Cheerio is used in a Node.js script run at build time to manipulate the SVG (as XML). Will never end in the users' browser. So why should we not use it?
Again, these are topics that we should discuss in our next team meeting and should not block merging this code.
See the comments above, and for the ones that are not clear happy to discuss with the whole team.
ashleemboyer
commented
2 years ago
- Requires the React consumers to import a specific CSS in their codebase to have the icons animate
As a consumer on the React side, I don't see this as a con. The import only needs to be made one time per project, so once it's in the code there's nothing extra for us to do.
Notice: I am totally aware that choosing SMIL over CSS means favouring the Dev Experience vs the User Experience, but quoting Melanie:
Our goal is quality work that pragmatically meets the highest level of accessibility as possible.
and I think the key word in this case is "pragmatic". My opinion is that in this very specific case, the pragmatic choice is to go with SMIL. But it's my opinion, and in a Cap Watkins scale I am on a 3 out of 10 on this, so as I said above your opinion prevails on mine.
I want to be very clear about why this is such an important topic and why Developer Experience vs. User Experience is an unfair comparison to make in this case. Animations can physically harm many people including myself, which is why I have such a strong opinion. Doing what we can as developers to prevent harm to users is always pragmatic in my opinion.
PS: @ashleemboyer if you decide to go with the CSS implementation, can you check the documentation for React? I tested locally using DotDev, so the code I added in the snippets is the one that worked there, but maybe I missed something or something is not clearly explained. Thanks
Docs look good and make sense!
johncowen
commented
2 years ago Hey just catching up here briefly:
@johncowen look at this commit, you will see how all the SVGs (SVG standalone, SVG React, SVG sprite) have an
embedded: 94aa462 This means that you would have received animated icons, and you would have nothing to do on your side.
Ah perfect! Thanks for pointing me to that 🙇 Yeah, so this would work for us also. But just to be clear, I definitely prefer the CSS approach.
I just gave this a quick go with some CSS:
@johncowen of course you can do it, but if in the future we have change an animation parameter (eg the duration), you will have to do the same (and someone will need to remember to do it/tell you); same if we add a new animated icon.
What I did is using all the code you've provided here, i.e. if you change that code we will automatically get the updates also, if/when we decide to yarn upgrade of course.
If you "add a new animated icon" they automatically get picked up so they are available to us. Whichever way though, somebody probably needs to make us aware they are there so we know we have those available to us to use, either by just communicating that to us, by us seeing them in our designs, or by us finding them by searching the icons docs page. If you add an icon called 'fantastic-icon' I won't just magically know I can write 'fantastic-icon' somebody has to tell me that 'fantastic-icon' is an identifier I can use for icons, either by just telling me, seeing a new icon in a design or searching for it in the docs page and coming across it (unlikely as I should 'find out' via the design).
My more overall opinion is, having new icons automatically added to a npm package in my eyes is not a 'feature'. They are 'added' when I see them in a design I'm building. On the other hand, existing icons being tweaked and me not having to know that they have been tweaked that I can opt in/out of via semver is a 'feature'.
it will work with @media query being set, and/or just the user selecting "reduced motion" from our settings page also. As it is now, for the "targets" supported, will work with the prefers-reduced-motion media query. In your case you will have to implement it yourself.
Cool, thanks for letting me know, we can add that feature from our side then (the in UI setting to turn it on/off addition to the @media switch)
Separately, overall it doesn't seem like its much extra effort to support switchable reduced motion icons, so while I'm here, I'm a big +1 to @ashleemboyer 's comments.
didoo
commented
2 years ago OK, the matter is settled then: we go with CSS animations. Thanks all @ashleemboyer @MelSumner @johncowen for the comments and feedback, really appreciated.
@cveigt can you sync with @MelSumner and @ashleemboyer and come up with a value for the duration of the two animations, for when the user sets the prefers-reduced-motion preference? Thanks.
ashleemboyer
commented
2 years ago Wanted to leave an answer for this question in case it comes up again in a future conversation on animations and prefers-reduced-motion:
question: shouldn't be the user agent to simply disable SMIL animations, in this case?
Browsers don't handle disabling animations whether CSS, GIFs, JavaScript, etc. If browsers started automatically disabling animations, countless sites are sure to break. It's also not possible for browsers to detect if JavaScript is being used to animate, so while it would be nice if browsers handled it for us, they just don't and that's what we have to work with.
cveigt
commented
2 years ago I couldn't find any particular recommendation or prescription for when users set prefers-reduced-motion, other than stopping the animation entirely.
I did some testing, and think 9s linear infinite for both works. I'd like to hear @MelSumner and @ashleemboyer 's thoughts.
Because there are several types of vestibular disorders and different levels of each of them, it seems safer to go with a very, very slow animation as long as it is still perceived as a loading/running state within the UI.
johncowen
commented
2 years ago As theres a nice convo going on here on the 'loading' reduced motion thing I had a couple of questions, but first a little background:
I vaguely remembered that browsers used to show like a 'barber shop' striped bar when a <progress> was :indeterminate. So I checked what they do now and they seem to have changed to be like a 'knight rider' progress bar. Interestingly this is always animated, well, at least when I use the macOS reduced motion setting it doesn't stop animating.
So my first curiosity question is, if the browser vendors aren't stopping these smaller type of animations should we be? Fully understand if the answer here is simply 'yes' - but then that does make me wonder why browser/OS vendors don't stop the native animation, I wondered if thats a conscious decision or is it because they just didn't get there or something. Does anyone know if there is anything at an OS level outside of the browser that stop animating when the 'Reduce motion' check box is selected in Settings? (btw I am still pro stopping/slowing the animations for reasons given above)
Second question is, would it be an idea to swap out the icon for a different one, similar to the old style indeterminate progress bar, maybe a static icon that looks like the barber shop pole thing. To me means 'loading' even though it's static.
That ^ to me means 'loading' more than a quarter filled circle, but maybe its just me.
Just to be clear I'm still pro doing something (anything) for reduced motion, but I'd like to understand more what the 'done thing' is and why (if folks know). Also wanted to put out there the static barber shop pole thing so see what other insights/thoughts folks here had on that.
cveigt
commented
2 years ago I'll let our accessibility experts chime in, but maybe this helps answer your questions. One of WCAG recommendations that applies specifically to animation is Pause, Stop Hide.
This states that one exception for this recommendation is essential animations.
Essential: if removed, would fundamentally change the information or functionality of the content, and information and functionality cannot be achieved in another way that would conform
There is a particular example that describes our case:
An animation that occurs as part of a preload phase or similar situation can be considered essential if interaction cannot occur during that phase for all users and if not indicating progress could confuse users or cause them to think that content was frozen or broken.
A "loading" animation A preloader animation is shown on a page which requires a certain percentage of a large file to be downloaded before playback can begin. The animation is the only content on the page and instructs the user to please wait while the video loads. Because the moving content is not presented in parallel with other content, no mechanism to pause, stop or hide it needs to be provided, even though the animation may run for more than 5 seconds for users with slower connections.
So that might be the reason or one of the reasons why browsers/OSs are not disabling them.
I'd like to try to keep the animation as long as it is feasible from an a11y perspective. As @didoo pointed out, the "animation for this icons is informative and not just decorative".
johncowen
commented
2 years ago Great find @cveigt !
From reading that example, initial thoughts are - it sounds like in some cases keeping an animated loader is fine if:
So it all depends on context. I think we mostly use this with other content on the page, so does that mean we should stop it? Or maybe leave it up to the designer/engineer to decide whether to stop it on a case by case basis? Off the top of my head I can't think of anywhere in our products where we use this particular icon on its own with no other content, but I could be wrong. And also sods/murphys law comes into play here, if we say it will never happen then it surely will.
As @didoo pointed out, the "animation for this icons is informative and not just decorative".
Whilst I agree with this to some extent, I would argue that it's not completely informative, the animation itself is not informative. It's an indeterminate loader i.e. the progress of the bar around the circle does not mean anything, the animation and icon is decorative. The fact that this signifies "this is loading" is the information it replaces.
This information could also be portrayed with text or a static image/icon (as mentioned above). In the same way that the text "Information" could be replaced with a little 'i circle' icon. The icon is decorative, the word "Information" (hidden from vision) is the information that is optionally read out to a screen reader. Using that comparison the word "Loading" (or maybe sometimes "In progress") is the information that would be hidden from vision and replaced with the decorative image/icon.
Overall I'd say an image/icon is informative if it has meaningful content, in this case I don't think it does. (opinions are my own etc etc)
Again thanks for the links @cveigt, exactly the type of stuff I was after 🙌 gonna read it in more detail. Let me know if you or anyone else has more thoughts 🙏
johncowen
commented
2 years ago Wow, off on a little tangent, but this is super interesting for me personally
Auto-updating For any auto-updating information that (1) starts automatically and (2) is presented in parallel with other content, there is a mechanism for the user to pause, stop, or hide it or to control the frequency of the update unless the auto-updating is part of an activity where it is essential.
didoo
commented
2 years ago @MelSumner @ashleemboyer @cveigt + @johncowen
to get this PR to an end, I am going with the animation-duration time of 9s as proposed by @cveigt
if in the future we want to update this value, it will be easy to do it.
:pushpin: Summary
This resolves #282.
In this PR I have:
srcpreprocess) to include also adding custom SVG animations to the “loading” and “running” files (usingcheeriofor XML/SVG manipulation)flight-iconsto2.1.0Notice: at the beginning I was thinking of using CSS for the animation; but then when working on the ticket I realized this would have meant that the standalone SVG icons and the React icons consumed by the marketing/dotdev team would not be animated (and require custom animation on the consumers side). So I decided to use add the animation directly in the SVG files, so that every format (standalone SVG, SVG sprite/Ember icon, React icons) would be animated.
@MelSumner I tried to use the
prefers-reduced-animationmedia query to disable the SVG animation, but without success. do you know a way to do it? I have also tried to activate the setting in MacOS and check if the browser would disable the SVG animations, but it didn't. My thinking at this point is that because animation for this icons is informative and not just decorative, makes sense to leave it as is (otherwise the complexity of support different formats would be much more). If you feel strongly about it let me know and we can discuss alternatives./cc @hashicorp/design-systems @cveigt @heatherlarsen
👉 Review by files changed
when doing the review, is not possible to view the icons in the preview branch because they're not been released yet as package so not installed during the website build for the preview; you can see them in the videos below
https://user-images.githubusercontent.com/686239/149758296-e627834d-cfde-4557-8f5a-f7daada38c6c.mov https://user-images.githubusercontent.com/686239/149758301-f9d6a18a-56be-4d9c-9950-3a03d6729b87.mov