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 "relevant mutations" tests for <img> crossorigin attribute #16357

Merged

Conversation

Projects
None yet
4 participants
@spinda
Copy link
Contributor

spinda commented Apr 15, 2019

These test cases are supposed to verify that changing an <img> tag's crossorigin attribute state causes the image resource to be reloaded (as per "4.8.4.3.2 Reacting to DOM mutations" in the HTML spec). However, they were set to expect a 'timeout', which doesn't make any sense: the expected result should be a 'load'. They were passing regardless of implementation correctness because the corresponding <img> tags were missing src attributes, which prevented load events from being generated.

See also: whatwg/html#4533

@annevk annevk requested a review from zcorpan Apr 16, 2019

@zcorpan
Copy link
Contributor

zcorpan left a comment

These are intentionally omitting src. They are testing that load and error are not fired as a result of mutating crossorigin when there's no src (or srcset/picture).

It could be useful to also have tests for mutating crossorigin with src, and certainly there could be source comments explaining things.

A previous PR that touched these tests: #2136

@spinda

This comment has been minimized.

Copy link
Contributor Author

spinda commented Apr 16, 2019

I see, that clears things up a bit. Thanks!

It could be useful to also have tests for mutating crossorigin with src

It certainly would, since both Firefox and Chromium fail these tests :)

I'll put the old tests back in addition to these "new" tests.

edit: I'm not sure why adding this comment caused the bot to ping a whole bunch of people all at once. Excuse me if I've done something wrong!

@wpt-pr-bot wpt-pr-bot added the html label Apr 16, 2019

@wpt-pr-bot wpt-pr-bot requested review from annevk, domenic, foolip, jdm, jgraham and zqzhang Apr 16, 2019

@spinda spinda force-pushed the spinda:fix-img-cross-origin-tests branch from 42a133b to b387321 Apr 16, 2019

@zcorpan
Copy link
Contributor

zcorpan left a comment

Thanks! The relevant mutations says "The element's crossorigin attribute's state is changed.", and these tests all change the state. (empty to "anonymous", or "foo" to "bar", would not change state.)

Can you report bugs on browsers that fail the new tests?

@zcorpan

This comment has been minimized.

Copy link
Contributor

zcorpan commented Apr 16, 2019

@wpt-pr-bot should have done its thing when the PR was created. I'm not sure why it didn't, but it has happened before.

@spinda

This comment has been minimized.

Copy link
Contributor Author

spinda commented Apr 17, 2019

Thanks! The relevant mutations says "The element's crossorigin attribute's state is changed.", and these tests all change the state. (empty to "anonymous", or "foo" to "bar", would not change state.)

Yep! The existing "crossorigin state not changed" cases cover the other side of this.

Can you report bugs on browsers that fail the new tests?

Can do!

@spinda spinda force-pushed the spinda:fix-img-cross-origin-tests branch from b387321 to 15d6ea0 Apr 17, 2019

@spinda

This comment has been minimized.

Copy link
Contributor Author

spinda commented Apr 17, 2019

Hmm, looking at the diff, seems that last push got mangled somewhere along the way. One sec...

Add rel-mutations tests for crossorigin with src
These test cases verify that changing an <img> tag's crossorigin
attribute state causes the image resource to be reloaded (as per
"4.8.4.3.2 Reacting to DOM mutations" in the HTML spec).

@spinda spinda force-pushed the spinda:fix-img-cross-origin-tests branch from 15d6ea0 to 9ecffbf Apr 17, 2019

@spinda

This comment has been minimized.

Copy link
Contributor Author

spinda commented Apr 17, 2019

Okay, fixed.

@zcorpan zcorpan merged commit efdf64c into web-platform-tests:master Apr 18, 2019

1 check passed

Taskcluster (pull_request) TaskGroup: success
Details
@zcorpan

This comment has been minimized.

Copy link
Contributor

zcorpan commented Apr 18, 2019

Thank you! Please add a comment linking to bugs for this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.