Re: Feedback on Philip Taylors Canvas Tests

Ms2ger wrote:
> On 01/04/2011 04:50 AM, Kris Krueger wrote:
>>
>> http://test.w3.org/html/tests/submission/PhilipTaylor/canvas/2d.imageData.put.wrongtype.html 
>>
>> Test case doesn't actually test what is stated in the spec...
>>> From the spec: "If the first argument to the method is null or not an 
>>> ImageData object then the putImageData() method must raise a 
>>> TYPE_MISMATCH_ERR exception."

(From which variation of the spec? That seems to be missing the fix from 
http://www.w3.org/Bugs/Public/show_bug.cgi?id=10403)

>  From the spec:
>> If the first argument to the method is null, then the putImageData() 
>> method must raise a TYPE_MISMATCH_ERR exception.

(That's the same as from the variation at 
http://whatwg.org/html5#dom-context-2d-putimagedata which is what the 
tests are intended to be written to.)

> However, this can only apply to putImageData(ImageData, ...). WebIDL 
> defines that a TypeError is thrown if the JS values don't match those 
> given in the interface, and the actual method defined in HTML is never 
> called. [1]

Agreed.

>> http://test.w3.org/html/tests/submission/PhilipTaylor/canvas/2d.drawImage.wrongtype.html 
>>
>> [...]
> 
> This is exactly the same case as above.

Agreed.

>> http://test.w3.org/html/tests/submission/PhilipTaylor/canvas/size.attributes.parse.minus.html 
>>
>> This test seems odd - what is it testing?
> 
> The "rules for parsing non-negative integers", and the rules for 
> reflecting an unsigned long IDL attribute. [2]

Indeed, plus the part of the canvas spec saying "If an attribute is 
missing, or if parsing its value returns an error, then the default 
value must be used instead". (The test case includes a link to that text.)

The parsing of non-negative integers isn't canvas-specific and should 
probably be tested by a wider IDL/reflection/microsyntax/etc test suite 
for all HTML elements and attributes, but I think it's better to test 
features too many times than too few, and I wanted to test all features 
that were relevant for canvas usage, so I added all the tests at 
http://test.w3.org/html/tests/submission/PhilipTaylor/canvas/index.size.attributes.parse.html 
for what I guessed might be potential implementation errors. It's not an 
exhaustive test of the integer parsing but it seems to find plenty of 
bugs in browsers so I think it's useful to keep them.

>> http://test.w3.org/html/tests/submission/PhilipTaylor/canvas/size.attributes.setAttribute.percent.html 
>>
>> http://test.w3.org/html/tests/submission/PhilipTaylor/canvas/size.attributes.parse.percent.html 
>>
>> These tests description indicates that it's a test for 'Parsing of 
>> non-negative integers'.
>> Though I see no negative attributes? Test case bug?
> 
> The test case is correct, the description isn't.
> 
> I approve these tests if the description is fixed.

They're testing that the string "100%" is parsed as a non-negative 
integer according to 
http://whatwg.org/html5/#rules-for-parsing-non-negative-integers , which 
should return the integer value 100 in this case. What's inaccurate 
about the description?

The same description is used for all of size.attributes.parse.* since 
they're all testing that same spec requirement. If the description for 
*.percent should be changed, why just that one and why not request all 
the others should be changed too?

>> [...]
>>
>> There is no interoperable consensus among the browsers for how strings 
>> should be parsed with a null suffix.
> 
> That's why we have tests. If browsers were interoperable on everything, 
> we wouldn't be discussing these tests.
> 
>> It's also not a part of HTML5 (though seems like a fine test for 
>> javascript or webidl, given the interop issues)
> 
> It is part of HTML; the specification defines which values are allowed 
> for these APIs, and it doesn't allow any trailing garbage, not even NULL 
> characters.
> 
>> I believe we can test canvas without resorting to using null suffixes.
> 
> We certainly can. I don't believe we should.

Agreed - these tests find bugs, which is the purpose of tests, so 
they're good to have. It's likely that most bugs will be generic 
IDL-layer bugs, but it's theoretically possible there'd be 
canvas-specific bugs (maybe some browser uses some fancy RPC-based 
context lookup system that doesn't like null characters, or whatever) so 
it's good to test them specifically here.

(It's not the test suite's responsibility to care whether e.g. some 
browser architectures use null-terminated strings internally and would 
find it very difficult to safely handle strings from JS containing 
embedded null characters. (I think Opera had (has?) that problem, and 
Acid3 caused pain by testing null characters). If it would be 
prohibitively expensive to pass the test, someone should propose spec 
changes so we can get interoperable behaviour in the future, rather than 
removing or ignoring the test.)

>> http://test.w3.org/html/tests/submission/PhilipTaylor/canvas/2d.text.draw.space.collapse.space.html 
>>
>> http://test.w3.org/html/tests/submission/PhilipTaylor/canvas/2d.text.draw.space.collapse.other.html 
>>
>> http://test.w3.org/html/tests/submission/PhilipTaylor/canvas/2d.text.draw.space.collapse.start.html 
>>
>> http://test.w3.org/html/tests/submission/PhilipTaylor/canvas/2d.text.draw.space.collapse.end.html 
>>
>> http://test.w3.org/html/tests/submission/PhilipTaylor/canvas/2d.text.measure.width.space.html 
>>
>> Spec does not have normative requirements for "space characters" that 
>> should be parsed by canvas.

The fillText/etc definition links to 
http://whatwg.org/html5#space-character to say what "space characters" 
are, and then says they all get replaced with U+0020, then the 
referenced CSS 2.1 says how the U+0020s are collapsed before the text is 
laid out. So it seems normatively required to me, and the tests are 
necessary to ensure the interaction between fillText and the CSS 
renderer is implemented correctly.

>> http://test.w3.org/html/tests/submission/PhilipTaylor/canvas/toDataURL.jpeg.quality.basic.html 
>>
>> This test needs to have its tolerance updated to allow more UA's to 
>> pass, if this test gets approved.
>> Might be better to change this test in some fashion to not have a 
>> tolerance value, since the image format is lossy.

Was the change in http://dvcs.w3.org/hg/html/rev/32eecc4e4d1c not 
sufficient?

The purpose of the test is to ensure the browser doesn't completely 
ignore the quality parameter (by checking that very low quality images 
should be smaller than very high quality), and that it still produces 
valid images at extreme high/low qualities rather than returning 
something completely bogus and unusable (which is tested by drawing them 
and checking they're vaguely the expected colour). I think it's useful 
to test those things so that we can notice when browsers are totally 
broken, but I'm not sure how else it could be done without some 
hardcoded tolerance value to verify the result.

>> http://test.w3.org/html/tests/submission/PhilipTaylor/canvas/toDataURL.jpeg.quality.notnumber.html 
>>
>> '0.01' should be valid input....
>> Canvas Spec (http://www.w3.org/TR/html5/the-canvas-element.html)
>>   "For the purposes of these rules, an argument is considered to be a 
>> number if it is converted to an IDL double value by the rules for 
>> handling arguments of type any in the Web IDL specification. [WEBIDL]".
>> The WebIDL for converting an argument to double 
>> (http://dev.w3.org/2006/webapi/WebIDL/#es-double) states that the 
>> first step is to compute ToNumber on the argument passed in, '0.01', 
>> which should be valid."
> 
> That's not what the specification refers to. The "rules for handling 
> arguments of type any" return a DOMString. [3]

Agreed.

>> http://test.w3.org/html/tests/submission/PhilipTaylor/canvas/2d.missingargs.html 
>>
>> Test is out of date with the spec, since the spec no longer has 
>> normative requirments that states a 'NOT_SUPPORTED_ERR' should be raised.
> 
> It doesn't look for a NOT_SUPPORTED_ERR, it looks for a TypeError. The 
> test was updated in September. [4]

Indeed.

>> http://test.w3.org/html/tests/submission/PhilipTaylor/canvas/2d.pattern.image.string.html 
>>
>> Test should check for TYPE_MISMATCH_ERR not TypeError per the canvas 
>> spec for createPattern
> 
> Exactly the same case as the first two tests mentioned in this email. 

Indeed.

>> http://test.w3.org/html/tests/submission/PhilipTaylor/canvas/2d.pattern.image.broken.html 
>>
>> This is seems incorrect....
>> All browsers fail because the image's "complete" attribute actually 
>> evaluates to true, but the code expects it to be false.
>> The test should find an interoperable way to pass in an image who's 
>> "complete" attribute is false
> 
> The specification states:
> 
>> If the image argument is an HTMLImageElement object that is not fully 
>> decodable, ..., then the implementation must return null.
> 
> So the test is correct, but the check for img.complete isn't necessary.
> 
> I approve this test with that check removed.

Agreed. The spec changed - I'll update the annotated spec copy and the test.

>> http://test.w3.org/html/tests/submission/PhilipTaylor/canvas/2d.drawImage.outsidesource.html 
>>
>> Many of the test here are invalid per the latest version of the spec.  
>> The only normative requirements are: "If the numeric arguments don't 
>> make sense (e.g. the destination is a 0×0 rectangle), throws an 
>> INDEX_SIZE_ERR exception."
> 
> That's not a normative requirement. The normative requirement is
> 
>> If one of the sw or sh arguments is zero, the implementation must 
>> raise an INDEX_SIZE_ERR exception.
> 
> However, this test does seem invalid.

Yep. The spec changed, then changed again - I believe the current 
behaviour is that pixels sampled from outside the image get clamped to 
the nearest edge pixel. I'll rewrite the test and hope implementors 
don't disagree with http://www.w3.org/Bugs/Public/show_bug.cgi?id=10799 
and change the spec again.

-- 
Philip Taylor
pjt47@cam.ac.uk

Received on Tuesday, 4 January 2011 15:50:56 UTC