Skip to content

Add AnimationBlocker to prevent autoplay of GIFs and other potentially animated filetypes#16497

Merged
brandonkelly merged 39 commits into5.7from
feature/prevent-autoplay-new
Mar 7, 2025
Merged

Add AnimationBlocker to prevent autoplay of GIFs and other potentially animated filetypes#16497
brandonkelly merged 39 commits into5.7from
feature/prevent-autoplay-new

Conversation

@gcamacho079
Copy link
Copy Markdown
Contributor

@gcamacho079 gcamacho079 commented Jan 23, 2025

Description

  • Pauses all GIFs added to the control panel by adding a canvas of the first frame over the image. Also does this for .webp images, which could be animated.
  • Adds a play/pause button when the image size allows for comfortable placement

Related issues

Resolves PT-25

@gcamacho079 gcamacho079 added the accessibility 👤 features related to accessibility label Jan 23, 2025
@linear
Copy link
Copy Markdown

linear Bot commented Jan 27, 2025

@gcamacho079 gcamacho079 marked this pull request as ready for review January 27, 2025 22:06
@gcamacho079
Copy link
Copy Markdown
Contributor Author

@brandonkelly the user preference has been removed. 👍🏼

Copy link
Copy Markdown
Contributor

@brianjhanson brianjhanson left a 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.

Comment thread src/web/assets/animationblocker/AnimationBlockerAsset.php Outdated
Comment thread src/web/assets/animationblocker/src/AnimationBlocker.ts Outdated
@gcamacho079
Copy link
Copy Markdown
Contributor Author

@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.

@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.

Copy link
Copy Markdown
Contributor

@brianjhanson brianjhanson left a comment

Choose a reason for hiding this comment

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

Just one minor "take it or leave it" note. Otherwise, looks good!

Comment thread src/base/Element.php Outdated
@gcamacho079
Copy link
Copy Markdown
Contributor Author

@brianjhanson all done! 👍🏼 Thanks for the review!

@brandonkelly brandonkelly merged commit 5aff92b into 5.7 Mar 7, 2025
@brandonkelly brandonkelly deleted the feature/prevent-autoplay-new branch March 7, 2025 00:52
@brandonkelly
Copy link
Copy Markdown
Member

Nice work!

@brianjhanson brianjhanson mentioned this pull request Apr 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

accessibility 👤 features related to accessibility

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants