Bug 80637 – fillText() does not ignore null or undefined as the maxWidth parameter

• Status: RESOLVED
• Resolution: CONFIGURATION CHANGED
• Component: Canvas

[Tom Hudson]

(Originally reported at http://code.google.com/p/chromium/issues/detail?id=110995, but appears to be a WebKit issue)

1. Use null or undefined as the fourth parameter for fillText
   e.g. ctx.fillText("Hello World!", 10, 10, null);
2. Note that it does not get displayed (probably translates null -> 0)

By my reading of the spec, that may be a defensible result for null, but not for undefined.

V8CanvasRenderingContext2D.cpp is auto-generated, and the IDL allows for optional parameters, but the autogenerated implementation isn't checking whether the 4th parameter is NULL or UNSPECIFIED - if anything's provided there from the JavaScript, it's passed as the 4th parameter, presumably coerced to 0. When the fourth parameter is undefined, we should instead be calling the 3-parameter version of the function.

[Adam Barth]

Mark, this looks related to your work on optional parameters.

[Tom Hudson]

The IDL spec seems to me to suggest that we could just add [TreatUndefinedAs=Missing]:

CanvasRenderingContext2D.idl:
        void fillText(in DOMString text, in float x, in float y, in [Optional, TreatUndefinedAs=Missing] float maxWidth);
        void strokeText(in DOMString text, in float x, in float y, in [Optional, TreatUndefinedAs=Missing] float maxWidth);

...but that fails to compile; it looks like we've used TreatUndefinedAs for NullString or EmptyString elsewhere in the IDL, but never Missing. WebCore/bindings/scripts/*.pm isn't very clear at first reading, but I'd guess we don't yet support it?

I think we might instead be able to provide a custom implementation, at least until Missing is supported?

[Tom Hudson]

Created attachment 131405 [details]
Patch

[Tom Hudson]

Adam, you've done most of the reviews of the .idl file in the last year - could you take a look at this patch, or recommend somebody else?

Open questions:
1. Do we want to treat NULL like Undefined, or like 0? I think the JavaScript spec expects the latter, although the developers who filed the Chrome bug want the former.
2. Do we go with this as good enough until our WebIDL implementation is extended to handle [TreatUnspecifiedAs=Missing]? Or is somebody familiar with that code available to do the general fix soon?

[Early Warning System Bot]

Comment on attachment 131405 [details]
Patch

Attachment 131405 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/11947080

[Build Bot]

Comment on attachment 131405 [details]
Patch

Attachment 131405 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/11943119

[Early Warning System Bot]

Comment on attachment 131405 [details]
Patch

Attachment 131405 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11947089

[Gustavo Noronha (kov)]

Comment on attachment 131405 [details]
Patch

Attachment 131405 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11942176

[Gyuyoung Kim]

Comment on attachment 131405 [details]
Patch

Attachment 131405 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11948137

[Build Bot]

Comment on attachment 131405 [details]
Patch

Attachment 131405 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/11943350

[Tom Hudson]

Hmm, looks like the right way to fix this is to find somebody to fix the IDL implementation? Otherwise we have to (1) commit the same hackery in all the ports, or (2) have port- (or backend-?) specific defines in the IDL files. There already are some #if (defined(V8_BINDING) && V8_BINDING) clauses, so that may be an expedient way out...

[Rob Buis]

Comment on attachment 131405 [details]
Patch

This breaks a lot of platforms, clearing review flag.

[Tom Hudson]

Needs somebody like pilgrim@ with IDL experience. Taking myself off.

[Brent Fulgham]

Created attachment 460985 [details]
Test Case

[Brent Fulgham]

Safari, Chrome, and Firefox all agree on rendering for this test case. I don't believe there is any remaining compatibility issue.