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

Jump to bottom

Fix "relevant mutations" tests for <img> crossorigin attribute #16357

Merged
merged 1 commit into from Apr 18, 2019
Merged

Fix "relevant mutations" tests for <img> crossorigin attribute #16357

merged 1 commit into from Apr 18, 2019

Conversation

spinda
Copy link
Contributor

@spinda 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 April 16, 2019 09:03
zcorpan
zcorpan requested changes Apr 16, 2019
View reviewed changes
Copy link
Member

@zcorpan zcorpan left a comment

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

spinda commented Apr 16, 2019 •
edited

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!

@spinda spinda force-pushed the fix-img-cross-origin-tests branch from 42a133b to b387321 Compare April 16, 2019 22:24
zcorpan
zcorpan approved these changes Apr 16, 2019
View reviewed changes
Copy link
Member

@zcorpan zcorpan left a comment

Choose a reason for hiding this comment

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

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
Copy link
Member

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
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 fix-img-cross-origin-tests branch from b387321 to 15d6ea0 Compare April 17, 2019 23:50
@spinda
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...

@spinda
Add rel-mutations tests for crossorigin with src
9ecffbf 
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 fix-img-cross-origin-tests branch from 15d6ea0 to 9ecffbf Compare April 17, 2019 23:59
@spinda
Copy link
Contributor Author

spinda commented Apr 17, 2019

Okay, fixed.

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

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
Reviewers

@annevk annevk annevk left review comments

@zcorpan zcorpan zcorpan approved these changes

@domenic domenic Awaiting requested review from domenic

@foolip foolip Awaiting requested review from foolip

@jdm jdm Awaiting requested review from jdm

@jgraham jgraham Awaiting requested review from jgraham

@zqzhang zqzhang Awaiting requested review from zqzhang

Assignees

@zcorpan zcorpan

Labels
html
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@spinda @zcorpan @annevk @wpt-pr-bot