feat: Allow users to pause gifs #19909
Conversation
akshaymankar
force-pushed
the
pause-gif
branch
from
775b063 to
da1b8ea
Compare
There was a problem hiding this comment.
Pull request overview
This PR adds a play/pause control for GIF images to reduce distraction from looping animations. The feature extracts the first frame of each GIF to display when paused, using a details/summary HTML element for the toggle control.
Key Changes:
- Exposes
file_typeproperty in FileAsset to identify GIF images - Implements GIF frame extraction using the gifwrap library to create static preview images
- Adds play/pause UI control using HTML details/summary elements with custom CSS styling
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 15 comments.
| File | Description |
|---|---|
| src/script/repositories/entity/message/FileAsset.ts | Adds readonly file_type property to track image MIME types |
| src/script/components/MessagesList/Message/ContentMessage/asset/common/useAssetTransfer/useAssetTransfer.ts | Implements GIF first-frame extraction logic and returns pauseFrameUrl alongside main asset URL |
| src/script/components/Image/Image.tsx | Adds conditional rendering for GIF images with play/pause controls using details/summary elements |
| src/style/components/image.less | Defines CSS for GIF wrapper, play/pause button triangle shapes, and animated GIF layering |
Review Summary: Changes Requested
The PR implements an interesting feature but has several issues that should be addressed before merging:
Critical Issues (2):
- Missing error handling for GIF processing that could crash the app
- Memory leak from unreleased pauseFrameUrl object URLs
Important Issues (7):
- Behavioral inconsistency (starts playing by default despite PR motivation)
- Missing accessibility features (unclear labels, keyboard support concerns)
- Type safety issue with optional fileType
- Performance concerns with large GIF processing
- Insufficient error handling for canvas context
- Missing test coverage
- Hardcoded colors that may not respect themes
Minor Issues (4):
- Code duplication in rendering logic
- Naming clarity improvements needed
- CSS best practices (zero units, hover states)
- Use of loose equality operator
Most critical is addressing the error handling and memory leak to prevent runtime failures. The accessibility and behavioral issues should also be resolved to ensure the feature works as intended for all users.
| } | ||
| }} | ||
| /> | ||
| <details open className="image-asset--gif"> |
There was a problem hiding this comment.
The details element starts in an "open" state, meaning the animated GIF plays by default. This contradicts the PR description stating that GIFs looping while writing messages is jarring. Consider removing the "open" attribute to start in a paused state by default, allowing users to opt-in to the animation.
| <details open className="image-asset--gif"> | |
| <details className="image-asset--gif"> |
| border-width: 1em 0 1em 2em; | ||
| border-style: solid; | ||
|
|
||
| border-color: transparent transparent transparent #202020; |
There was a problem hiding this comment.
The play/pause button uses a hardcoded color (#202020). This may not respect user theme preferences (light/dark mode) or brand color schemes. Consider using a CSS variable or theme token to ensure consistency across the application and proper contrast in different themes.
| border-color: transparent transparent transparent #202020; | |
| border-color: transparent transparent transparent var(--gif-play-button-color, #202020); |
...onents/MessagesList/Message/ContentMessage/asset/common/useAssetTransfer/useAssetTransfer.ts
Outdated
Show resolved
Hide resolved
| const pauseFrameUrl = await (async () => { | ||
| if (blob.type == 'image/gif' || fileType == 'image/gif') { | ||
| const gifArrayBuffer = await blob.arrayBuffer(); | ||
| const gifBuffer = Buffer.from(gifArrayBuffer); | ||
| const frame0 = await GifUtil.read(gifBuffer).then(gifFile => gifFile.frames[0]); | ||
| const canvas = document.createElement('canvas'); | ||
| const ctx = canvas.getContext('2d'); | ||
| if (ctx) { | ||
| canvas.width = frame0.bitmap.width; | ||
| canvas.height = frame0.bitmap.height; | ||
|
|
||
| const imageData = ctx.createImageData(frame0.bitmap.width, frame0.bitmap.height); | ||
| imageData.data.set(frame0.bitmap.data); | ||
| ctx.putImageData(imageData, 0, 0); | ||
| } | ||
|
|
||
| const pauseImageBlob: Blob | null = await new Promise(resolve => { | ||
| canvas.toBlob( | ||
| blob => { | ||
| resolve(blob); | ||
| }, | ||
| 'image/jpeg', | ||
| 0.9, | ||
| ); | ||
| }); | ||
|
|
||
| if (pauseImageBlob) { | ||
| return URL.createObjectURL(pauseImageBlob); | ||
| } | ||
| return undefined; | ||
| } | ||
| return undefined; | ||
| })(); |
There was a problem hiding this comment.
The new GIF pause/play feature lacks test coverage. Given that the repository has comprehensive unit tests and this introduces significant new functionality with GIF processing logic and UI controls, tests should be added to verify the pause/play behavior, frame extraction, and error handling.
| const pauseFrameUrl = await (async () => { | ||
| if (blob.type == 'image/gif' || fileType == 'image/gif') { | ||
| const gifArrayBuffer = await blob.arrayBuffer(); | ||
| const gifBuffer = Buffer.from(gifArrayBuffer); | ||
| const frame0 = await GifUtil.read(gifBuffer).then(gifFile => gifFile.frames[0]); | ||
| const canvas = document.createElement('canvas'); | ||
| const ctx = canvas.getContext('2d'); | ||
| if (ctx) { | ||
| canvas.width = frame0.bitmap.width; | ||
| canvas.height = frame0.bitmap.height; | ||
|
|
||
| const imageData = ctx.createImageData(frame0.bitmap.width, frame0.bitmap.height); | ||
| imageData.data.set(frame0.bitmap.data); | ||
| ctx.putImageData(imageData, 0, 0); | ||
| } | ||
|
|
||
| const pauseImageBlob: Blob | null = await new Promise(resolve => { | ||
| canvas.toBlob( | ||
| blob => { | ||
| resolve(blob); | ||
| }, | ||
| 'image/jpeg', | ||
| 0.9, | ||
| ); | ||
| }); | ||
|
|
||
| if (pauseImageBlob) { | ||
| return URL.createObjectURL(pauseImageBlob); | ||
| } | ||
| return undefined; | ||
| } | ||
| return undefined; | ||
| })(); |
There was a problem hiding this comment.
Performance concern: The entire GIF file is read and the first frame is extracted for every GIF image, which can be expensive for large files. Consider adding a file size check before attempting frame extraction, or implementing caching to avoid re-processing the same GIF multiple times.
...onents/MessagesList/Message/ContentMessage/asset/common/useAssetTransfer/useAssetTransfer.ts
Outdated
Show resolved
Hide resolved
| const url = URL.createObjectURL(blob); | ||
|
|
||
| const pauseFrameUrl = await (async () => { | ||
| if (blob.type == 'image/gif' || fileType == 'image/gif') { |
There was a problem hiding this comment.
The comparison uses loose equality (==) instead of strict equality (===). Use strict equality to avoid type coercion issues.
| if (blob.type == 'image/gif' || fileType == 'image/gif') { | |
| if (blob.type === 'image/gif' || fileType === 'image/gif') { |
|
🔗 Download Full Report Artifact 🧪 Playwright Test Summary
specs/Accessibility/Accessibility.spec.ts (❌ 1 failed,
|
akshaymankar
force-pushed
the
pause-gif
branch
from
da1b8ea to
4c58578
Compare
akshaymankar
force-pushed
the
pause-gif
branch
from
d54bdd5 to
4e48881
Compare
| onClick={event => { | ||
| if (!isLoading) { | ||
| onClick?.(event); | ||
| } | ||
| }} | ||
| /> | ||
| <details open className="image-asset--animated"> | ||
| <summary role="button" aria-label="Toggle animation playback"></summary> | ||
| <img | ||
| css={{...getImageStyle(imageSizes), ...imageStyles}} | ||
| src={assetUrl} | ||
| role="presentation" | ||
| alt={alt} | ||
| data-uie-name={isLoading ? 'image-loader' : 'image-asset-img'} | ||
| onClick={event => { | ||
| if (!isLoading) { | ||
| onClick?.(event); | ||
| } | ||
| }} |
There was a problem hiding this comment.
The onClick handlers on both the pause frame image and the animated image can be triggered simultaneously, potentially causing duplicate modal openings or other unintended behavior. The onClick on line 137-141 will bubble up and could trigger the parent's onClick handler. Consider using event.stopPropagation() or restructure the event handlers.
| resolve(blob); | ||
| }, | ||
| 'image/jpeg', | ||
| 0.9, |
There was a problem hiding this comment.
Using a magic number 0.9 as the JPEG quality parameter without explanation. Consider extracting this to a named constant like 'GIF_PAUSE_FRAME_QUALITY' with a comment explaining why 0.9 was chosen, to improve code maintainability.
| ]; | ||
| const url = await getAssetUrl(image, allowedImageTypes); | ||
| const url = await getAssetUrl(image, allowedImageTypes, fileType); | ||
| if (isUnmouted.current) { |
There was a problem hiding this comment.
The variable name contains a typo - 'isUnmouted' should be 'isUnmounted'. While this is a pre-existing typo, it reduces code readability and should be corrected.
| summary { | ||
| position: absolute; | ||
| z-index: 2; | ||
| top: 0.5rem; | ||
| right: 0.5rem; | ||
| width: 0; | ||
| height: 2em; | ||
| border-width: 1em 0 1em 2em; | ||
| border-style: solid; | ||
|
|
||
| border-color: transparent transparent transparent #202020; | ||
| background: transparent; | ||
| cursor: pointer; | ||
| transition: 100ms all ease; | ||
| } |
There was a problem hiding this comment.
The play/pause button is positioned absolutely but lacks responsive positioning. On very small screens or images, the button may overflow outside the image boundaries or overlap important content. Consider adding media queries or making the button size and position responsive to the image dimensions.
|
|
||
| import {useEffect, useState} from 'react'; | ||
|
|
||
| import {GifUtil} from 'gifwrap'; |
There was a problem hiding this comment.
The 'gifwrap' library is imported but is not listed as a dependency in package.json. This will cause runtime errors when the code tries to process GIF files. Add 'gifwrap' to the dependencies section of package.json.
| cursor: pointer; | ||
| transition: 100ms all ease; | ||
| } | ||
|
|
There was a problem hiding this comment.
The play/pause button lacks visible focus styling. Users navigating by keyboard won't have a visual indication when the button is focused, making it difficult to know where they are on the page. Add focus styles to the summary element.
| summary:focus, | |
| summary:focus-visible { | |
| outline: 2px solid #ffffff; | |
| outline-offset: 2px; | |
| } |
| } | ||
| } | ||
|
|
||
| .image-asset--animated-wrapper { |
There was a problem hiding this comment.
The class name 'image-asset--animated-wrapper' doesn't follow a consistent naming pattern with existing BEM-style classes in the codebase. Consider using a more descriptive name that indicates this is specifically for GIF controls, such as 'image-asset--gif-controls-wrapper' or following the existing pattern more closely.
| .image-asset--animated-wrapper { | |
| .image-asset--gif-controls-wrapper { |
| .image-asset--animated img { | ||
| position: absolute; | ||
| top: 0; | ||
| left: 0; | ||
| display: inline-block; |
There was a problem hiding this comment.
The image uses absolute positioning which removes it from normal document flow. Both images (pause frame and animated) are absolutely positioned at the same location, which could cause layout issues or z-index conflicts. Ensure the positioning strategy doesn't break the layout in edge cases.
| .image-asset--animated img { | |
| position: absolute; | |
| top: 0; | |
| left: 0; | |
| display: inline-block; | |
| .image-asset--animated { | |
| position: relative; | |
| display: inline-block; | |
| } | |
| .image-asset--animated img { | |
| display: block; | |
| width: 100%; | |
| height: auto; |
| public readonly downloadProgress: ko.PureComputed<number | undefined>; | ||
| public readonly cancelDownload: () => void; | ||
| public file_size: number; | ||
| public readonly file_type: string; |
There was a problem hiding this comment.
The field is declared as 'readonly' but initialized to an empty string in the constructor, which contradicts the readonly semantics. Either remove 'readonly' to allow modification, or ensure file_type is set at construction time via a constructor parameter.
| public readonly file_type: string; | |
| public file_type: string; |
| const gifBuffer = Buffer.from(gifArrayBuffer); | ||
| const frame0 = await GifUtil.read(gifBuffer).then(gifFile => gifFile.frames[0]); |
There was a problem hiding this comment.
Buffer is a Node.js global that is not available in browser environments by default. This will cause a runtime error unless a polyfill is configured. Use Uint8Array instead, which is native to browsers, or ensure the Buffer polyfill is properly set up.
| const gifBuffer = Buffer.from(gifArrayBuffer); | |
| const frame0 = await GifUtil.read(gifBuffer).then(gifFile => gifFile.frames[0]); | |
| const gifBytes = new Uint8Array(gifArrayBuffer); | |
| const frame0 = await GifUtil.read(gifBytes).then(gifFile => gifFile.frames[0]); |
akshaymankar
force-pushed
the
pause-gif
branch
from
4e48881 to
1265996
Compare
akshaymankar
force-pushed
the
pause-gif
branch
from
1265996 to
a60c376
Compare
akshaymankar
force-pushed
the
pause-gif
branch
from
a60c376 to
01afa28
Compare
|
| opacity: 1; | ||
| } | ||
|
|
||
| [open] summary { |
There was a problem hiding this comment.
The CSS selector [open] on line 79 is missing the element type qualifier. It should be details[open] to be more specific and match the pattern used elsewhere in the file (e.g., details summary on lines 85 and 89). This improves specificity and makes the selector's intent clearer.
| [open] summary { | |
| details[open] summary { |
| border-color: transparent transparent transparent #202020; | ||
| background: transparent; | ||
| cursor: pointer; | ||
| opacity: 0.1; |
There was a problem hiding this comment.
The opacity value of 0.1 makes the play/pause button nearly invisible by default. This could make it difficult for users to discover this functionality. Consider using a higher opacity value (e.g., 0.5-0.7) or adding a subtle background to ensure the button is more discoverable while still being unobtrusive.
| opacity: 0.1; | |
| opacity: 0.6; |
| const imageData = ctx.createImageData(frame0.bitmap.width, frame0.bitmap.height); | ||
| imageData.data.set(frame0.bitmap.data); | ||
| ctx.putImageData(imageData, 0, 0); | ||
|
|
There was a problem hiding this comment.
Typo in comment: "umounted" should be "unmounted"
| {imageUrl?.pauseFrameUrl ? ( | ||
| <div className="image-asset--animated-wrapper"> | ||
| <ImageElement src={imageUrl?.pauseFrameUrl} /> | ||
| <details open className="image-asset--animated"> |
There was a problem hiding this comment.
The details element is set to open by default, which means GIFs will always start playing automatically. This contradicts the pause/play feature's purpose and may be jarring for users as noted in the PR description. Consider removing the open attribute so GIFs start paused, or make this configurable based on user preference.
| <details open className="image-asset--animated"> | |
| <details className="image-asset--animated"> |
| getAssetUrl: async ( | ||
| resource: AssetRemoteData, | ||
| acceptedMimeTypes?: string[], | ||
| fileType?: string, | ||
| ): Promise<AssetUrl> => { |
There was a problem hiding this comment.
The fileType parameter is optional but there's no documentation explaining when it should be provided or how it relates to blob.type. The condition checks both blob.type === 'image/gif' OR fileType === 'image/gif', which suggests they might differ in some cases. Add a JSDoc comment explaining when and why fileType should be passed as a parameter.
| <details open className="image-asset--animated"> | ||
| <summary role="button" aria-label="Toggle animation playback"></summary> | ||
| <ImageElement src={assetUrl} /> | ||
| </details> |
There was a problem hiding this comment.
Using the HTML details/summary elements for a play/pause toggle is semantically incorrect. These elements are designed for expandable/collapsible content disclosure, not for media controls. Consider using a standard button element with proper ARIA attributes like aria-pressed to indicate the play/pause state, which would better convey the control's purpose to assistive technologies.
Pull Request
Its very jarring when a gif keeps looping while writing messages, so I thought I'll implement a play/pause button.
Somethings to note:
Inspired by:
Summary
What did I change and why?
The
FileAssettype now exposesfile_typewhich is then thread through various types to decide whether to show gif logic.Risks and how to roll out / roll back (e.g. feature flags):
I haven't tested this with large GIFs, there rendering could be slower
Security Checklist (required)
Standards Acknowledgement (required)
Screenshots or demo (if the user interface changed)
Notes for reviewers