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

animations: Support reduced motion #26376

Closed
wants to merge 2 commits into from

Conversation

yukijoou
Copy link

@yukijoou yukijoou commented Aug 7, 2023

Most animations in gitea are handled by fomantic-ui, but as it doesn't support reduced motion, this commit adds a hacky workaround to force-disable motion animations. It also makes gitea's built-in animations respect reduced-motion.


I tried patching the issue upstream, but gave up after a few hours because of how messy and hard to work with their codebase is. This fixes #15709.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Aug 7, 2023
@pull-request-size pull-request-size bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Aug 7, 2023
@silverwind
Copy link
Member

silverwind commented Aug 7, 2023

We can likely get rid of the whole transition module of fomantic which would eliminate all the menu transitions and such. Only problem with that is that it needs a few JS "shims" so that fomantic won't error when the module is absent. But I do see it as a long-term goal to remove all these transitions, for all users.

In case you want to try it, remove this line and run make fomantic to see the errors.

@yukijoou
Copy link
Author

yukijoou commented Aug 7, 2023

But I do see it as a long-term goal to remove all these transitions, for all users.

From the issue thread linked though it seems some people like the animations… which is why I wen that route instead

whatever the long term solution is, could this be applied in the meantime? it's an accessibility issue, and though it probably doesn't impact many people, and we could work around it with css applied in the useragent, it'd be great to do something about it!

I'll look into removing the animation module in the meantime

edit: Looked into fomantic's issues when disabling transitions, and it seems like it uses javascript tricks that obscure how it works… I'd probably need to figure out how to set up a proper dev environment, and really don't feel like digging into that right now. I've already spend hours debugging their libraries for issues like this, and gave up

@yukijoou yukijoou marked this pull request as draft August 7, 2023 21:07
@yukijoou
Copy link
Author

yukijoou commented Aug 7, 2023

Removed the unused rules for the static transitions, and simplified the pulse animation :D will work on the spinner

@pull-request-size pull-request-size bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 7, 2023
@yukijoou
Copy link
Author

yukijoou commented Aug 7, 2023

Added a first draft for SVG loading indicators. More info in the commit!

Is it fine to load the SVG from the CSS? I did it this way because it seemed simpler and shouldn't break any existing code, but maybe it should be done as a reusable template/component…
Also, these images should be themed to more closely match the older spinner!

If bigger changes are needed, I'll let someone more familiar with the codebase take over. I can't really do any major refactor as this is my first PR and I don't know the codebase well yet, if at all. As far as I'm concerned, in its current state, this PR solves the motion issue, though the latest commit likely introduces other issues.

I'll write a better commit message for the second commit before this gets merged, or is it better if I squash the two commits?

Thanks for your help!

Most animations in gitea are handled by fomantic-ui, but as it doesn't
support reduced motion, this commit adds a hacky workaround to
force-disable motion animations. It also makes gitea's built-in animations
respect reduced-motion.
@pull-request-size pull-request-size bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 8, 2023
@yukijoou yukijoou marked this pull request as ready for review August 8, 2023 22:23
@silverwind
Copy link
Member

silverwind commented Aug 11, 2023

I think maybe this PR should focus only on the loading icon replacement and we defer the animation removal to later. I want to try removing the transitions module soon, and if I succeed, it will be much cleaner than these style overrides.

This is based on the work of Utkarsh Verma (n3r4zzurr0 on GitHub), under
the MIT license.
@yukijoou
Copy link
Author

I'm not really interested on working on this further if the main focus isn't reduced motion support… I mostly wanted to do something about that year-old issue, and don't have much interest on doing anything else. I've spent a lot more time on this than I wanted to, and don't really want to do any more work. Feel free to cherrypick and merge my spinners commit if you want to, I'll leave this PR as a draft if someone else wants to pick back up where I left off

@yukijoou yukijoou marked this pull request as draft August 11, 2023 14:06
@silverwind
Copy link
Member

Maybe we should close then, I can not really accept these CSS hacks for the animation removal. I think we are clear we don't want these animations, even when the user has the reduced motion preference disabled. As I said, I will investigate separately what it takes to remove the Fomantic module, which is imho the proper solution.

As for the spinner, I imagine it will be a tricky endevour because it will likely need some size adjustments based on location in use.

@silverwind
Copy link
Member

Transition module removal in #26469.

@wxiaoguang
Copy link
Contributor

#26469 has been merged

@silverwind
Copy link
Member

The spinner replacement is still valuable here, need to check if it works.

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Aug 27, 2023

The Fomantic loader (spinner) has been removed by #26670

If I understand correctly, (this PR) it's willing to use SVG spinner instead of CSS spinner in the future?

@silverwind
Copy link
Member

SVG spinners would be ideal yes, because these can be swapped easily to static SVGs for prefers-reduced-motion, unlike the current border-based spinners.

@silverwind
Copy link
Member

I think we can close this as I assume the original author may have lost interest. We can re-open anytime of course.

@silverwind silverwind closed this Nov 3, 2023
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Feb 1, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature Request] option to disable animations
5 participants