-
Notifications
You must be signed in to change notification settings - Fork 645
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
Add AnimationBlocker to prevent autoplay of GIFs and other potentially animated filetypes #16497
base: 5.7
Are you sure you want to change the base?
Conversation
…ous PR to see if it still works
@brandonkelly the user preference has been removed. 👍🏼 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gcamacho079 Looking good! Just a few comments and I pushed up a few minor styling tweaks.
The biggest thing I noticed is that the animated images will play the first time they're inserted into the dom after you upload it (here's a quick video https://share.cleanshot.com/qXWpkXNh).
I think that's because when those are inserted into the DOM, they get an action URL that doesn't have .gif
or .webp
in the src
or srcset
. I'm not sure what the best way around it is. The only thing I can think of would be to output some kind of data-filename
on the img
so we can use that to determine if we should stop the animation.
I suppose another option would be to refactor this into a web component like Shoelace has. Then we could use PHP to determine if an image is animated or not and either output a <craft-animated-image/>
wrapper or not. I'm not sure how involved that change would be though. It could be larger than we want it to be.
public $depends = [ | ||
JqueryAsset::class, | ||
]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we get rid of the jQuery dependency?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Absolutely!
protected get imageAddedObserver() { | ||
return this.#imageAddedObserver; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need this getter? It doesn't look like anything uses it, and if we want the imageAddedObserver
to be available we could just make it public.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call, I may have had a purpose for this at one point, but I'll remove it for now. 👍🏼
@brianjhanson thank you for catching that, and for your style updates. 👍🏼 I'll poke around and see what route makes sense. We could also do a hybrid approach of wrapping the image in a web component during the AnimationBlocker initialization. |
Description
.webp
images, which could be animated.Related issues
Resolves PT-25