Bug 80637 - fillText() does not ignore null or undefined as the maxWidth parameter
Summary: fillText() does not ignore null or undefined as the maxWidth parameter
Status: RESOLVED CONFIGURATION CHANGED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Canvas (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-03-08 13:54 PST by Tom Hudson
Modified: 2022-07-18 14:19 PDT (History)
9 users (show)

See Also:


Attachments
Patch (6.28 KB, patch)
2012-03-12 14:15 PDT, Tom Hudson
webkit-ews: commit-queue-
Details | Formatted Diff | Diff
Test Case (464 bytes, text/html)
2022-07-18 14:18 PDT, Brent Fulgham
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Tom Hudson 2012-03-08 13:54:38 PST
(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.
Comment 1 Adam Barth 2012-03-08 14:03:58 PST
Mark, this looks related to your work on optional parameters.
Comment 2 Tom Hudson 2012-03-12 08:33:20 PDT
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?
Comment 3 Tom Hudson 2012-03-12 14:15:23 PDT
Created attachment 131405 [details]
Patch
Comment 4 Tom Hudson 2012-03-12 14:22:46 PDT
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?
Comment 5 Early Warning System Bot 2012-03-12 14:36:42 PDT
Comment on attachment 131405 [details]
Patch

Attachment 131405 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/11947080
Comment 6 Build Bot 2012-03-12 14:40:06 PDT
Comment on attachment 131405 [details]
Patch

Attachment 131405 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/11943119
Comment 7 Early Warning System Bot 2012-03-12 14:48:22 PDT
Comment on attachment 131405 [details]
Patch

Attachment 131405 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11947089
Comment 8 Gustavo Noronha (kov) 2012-03-12 16:08:13 PDT
Comment on attachment 131405 [details]
Patch

Attachment 131405 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11942176
Comment 9 Gyuyoung Kim 2012-03-12 16:43:35 PDT
Comment on attachment 131405 [details]
Patch

Attachment 131405 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11948137
Comment 10 Build Bot 2012-03-12 22:38:38 PDT
Comment on attachment 131405 [details]
Patch

Attachment 131405 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/11943350
Comment 11 Tom Hudson 2012-03-13 06:27:13 PDT
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...
Comment 12 Rob Buis 2012-04-05 13:17:16 PDT
Comment on attachment 131405 [details]
Patch

This breaks a lot of platforms, clearing review flag.
Comment 13 Tom Hudson 2012-04-05 13:29:17 PDT
Needs somebody like pilgrim@ with IDL experience. Taking myself off.
Comment 14 Brent Fulgham 2022-07-18 14:18:56 PDT
Created attachment 460985 [details]
Test Case
Comment 15 Brent Fulgham 2022-07-18 14:19:13 PDT
Safari, Chrome, and Firefox all agree on rendering for this test case. I don't believe there is any remaining compatibility issue.