web-platform-tests / wpt

Test suites for Web platform specs — including WHATWG, W3C, and others
https://web-platform-tests.org/
Other
4.98k stars 3.09k forks source link

Test /2dcontext/pixel-manipulation/2d.imageData.put.nonfinite.html is wrong #13393

Open nox opened 6 years ago

nox commented 6 years ago

This test checks that passing Infinity etc to ctx.imageData raises a TypeError, but this method takes long values, not unrestricted double ones.

Juanmihd commented 5 years ago

Hey nox,

That's right, the type expected in that function is of type long, and the test is testing precisely that when receiving not a long value (such as inifinity) it should raise a TypeError.

https://html.spec.whatwg.org/multipage/canvas.html#2dcontext This is where the spec talks about the type being long, as you said.

So it is actually correct that this is being tested to ensure it fails with not long values that cannot be converted to long (such as inifnity).

jdm commented 5 years ago

Agreed, step 6 of https://heycam.github.io/webidl/#abstract-opdef-converttoint says this should in fact throw a TypeError.

nox commented 5 years ago

AFAIU, WebIDL will happily convert Infinity to 0 for long arguments. See for example how document.childNodes.item(Infinity) returns the first child of the document.

jdm commented 5 years ago

You're right; I was looking at the wrong branch. This should be hitting step 8 instead.

Juanmihd commented 5 years ago

Yes, it will happily convert Infinity to 0 for long arguments, but not for [EnforceRange] long arguments. There is probably a missmatch between the WPT test and the 2dCanvas interface, that can be fixed either by changing the WPT test to accept infinities, or changing the specs to expect [EnforceRange] long arguments.

I've just created an issue to try to address that by changing the spec to receive [EnforceRange] long arguments. I don't know which one makes more sense, but both seem as valid solutions.

fserb commented 5 years ago

Either we change the signatures to be [EnforceRange] or we remove those tests. Either way, I guess this bug is gone. We can continue the conversation on the issue @Juanmihd created.

jdm commented 5 years ago

I don't think closing the issue before we actually resolve it makes sense.