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

Fix a step in 'update the image data' #8182

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

zcorpan
Copy link
Member

@zcorpan zcorpan commented Aug 11, 2022

Fix the 'update the image data' algorithm when src is set to the same value. Previously, it would only restart the animation / return if current request's state is partially available, not if it's completely available.

Fixes #8080.


/images.html ( diff )

Fix the 'update the image data' algorithm when src is set to the same value.

Fixes #8080.
@zcorpan zcorpan requested a review from yoavweiss August 11, 2022 11:02
Copy link
Contributor

@yoavweiss yoavweiss left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

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

LGTM. I would prefer the commit message use a word like "Fix" instead of "Clarify", and mention in particular the problem ("this only restarts the animation / returns if current request's state is partially available, not if it's completely available. That seems wrong") since I thought at first this was editorial.

Tests would be nice if you could, but I can understand that this is probably just an oversight and not worth testing. Maybe worth digging around to see if it's already tested, I guess.

@zcorpan zcorpan changed the title Clarify a step in 'update the image data' Fix a step in 'update the image data' Aug 15, 2022
zcorpan added a commit to web-platform-tests/wpt that referenced this pull request Aug 15, 2022
@zcorpan zcorpan added do not merge yet Pull request must not be merged per rationale in comment needs tests Moving the issue forward requires someone to write tests labels Aug 15, 2022
@zcorpan
Copy link
Member Author

zcorpan commented Aug 15, 2022

See #8080 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do not merge yet Pull request must not be merged per rationale in comment needs tests Moving the issue forward requires someone to write tests topic: img
Development

Successfully merging this pull request may close these issues.

Update the image data - setting src to the same value logic seems incorrect
3 participants