Bug 102656 – The 2D Canvas functions fillText()/strokeText() should display nothing when maxWidth is less then or equal to zero

• Status: RESOLVED
• Resolution: FIXED
• Component: Canvas

[Rashmi Shyamasundar]

The spec http://www.whatwg.org/specs/web-apps/current-work/multipage/the-canvas-element.html#text-preparation-algorithm) says -
"If maxWidth was provided but is less than or equal to zero, return an empty array"

[Rashmi Shyamasundar]

Created attachment 177465 [details]
Patch

[Rashmi Shyamasundar]

The attached patch introduces a check for negative-maxWidth in the function CanvasRenderingContext2D::drawTextInternal().

[Chris Dumez]

Comment on attachment 177465 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=177465&action=review

> Source/WebCore/ChangeLog:3
> +        If maxWidth for displaying a text, is zero or negative then, return empty array

I guess we should mention Canvas2D in the bug title.

> LayoutTests/canvas/philip/tests/2d.text.draw.fill.maxWidth.negative-expected.txt:1
> +Cannot automatically verify result

It needs to be automatically verified to be a useful LayoutTest, right? See comment below.

> LayoutTests/canvas/philip/tests/2d.text.draw.fill.maxWidth.negative.html:20
> +

This test looks different from http://dvcs.w3.org/hg/html/raw-file/tip/tests/submission/PhilipTaylor/canvas/2d.text.draw.fill.maxWidth.negative.html

For example, it is missing _assertGreen(ctx, 100, 50); in the end.

[Rashmi Shyamasundar]

I have uploaded a patch to fix the bug - https://bugs.webkit.org/show_bug.cgi?id=102654 . As part of this patch, I have defined the function _assertGreen() in LayoutTests/canvas/philip/tests.js .

After that patch is reviewed and accepted, the same function can be used in the layout-test for this bug.

[Chris Dumez]

(In reply to comment #4)
> I have uploaded a patch to fix the bug - https://bugs.webkit.org/show_bug.cgi?id=102654 . As part of this patch, I have defined the function _assertGreen() in LayoutTests/canvas/philip/tests.js .
> 
> After that patch is reviewed and accepted, the same function can be used in the layout-test for this bug.

You should probably mark the other bug as a dependency then.

[Rashmi Shyamasundar]

This w3c test is 
1. Passing on Opera
2. Failing on IE, FireFox, Safari.

[Rashmi Shyamasundar]

Created attachment 186533 [details]
Patch

[Benjamin Poulain]

Comment on attachment 186533 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=186533&action=review

Nice catch, it is amazing this was not tested before... :)

Some comments:

> Source/WebCore/ChangeLog:9
> +        The functions fillText()/strokeText() should not display anything when 
> +        maxWidth is less than or equal to zero.

I suggest referencing this in the ChangeLog: http://www.w3.org/TR/2dcontext/#text-preparation-algorithm

> LayoutTests/ChangeLog:13
> +        * fast/canvas/canvas-fillText-maxWidth-zero-expected.txt: Added.
> +        * fast/canvas/canvas-fillText-maxWidth-zero.html: Added.
> +        * fast/canvas/script-tests/canvas-fillText-maxWidth-zero.js: Added.

The name of this test is unfortunate:
1) You test more than fillText, you also check strokeText.
2) The value 0 is not particularly interesting. It is <= 0.

I suggest splitting the test in two (fillText and strokeText), and change maxWidth-zero to invalid-maxWidth.

> LayoutTests/fast/canvas/script-tests/canvas-fillText-maxWidth-zero.js:17
> +debug("Test canvas.fillText() with maxWidth zero");
> +ctx.fillStyle = '#f00';
> +ctx.fillText("AA", 0, 1, 0);
> +
> +var imageData = ctx.getImageData(0, 0, 1, 1);
> +var imgdata = imageData.data;
> +shouldBe("imgdata[0]", "0");
> +shouldBe("imgdata[1]", "255");
> +shouldBe("imgdata[2]", "0");

This looks a little fragile. Does the test pass if you set maxWidth to 1?

Is it possible to make a less fragile test?

[Rashmi Shyamasundar]

Created attachment 186820 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 186820 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=186820&action=review

> Source/WebCore/ChangeLog:3
> +        If maxWidth for displaying a text, is zero or negative then, return empty array

maxWidth or what? The bug titel could be seriously improved. You could add [2D Canvas] or so

> Source/WebCore/ChangeLog:9
> +        maxWidth is less than or equal to zero.

, according to spec:

[Rashmi Shyamasundar]

Created attachment 186827 [details]
Patch

[Dirk Schulze]

Comment on attachment 186827 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=186827&action=review

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:2176
> +    if (useMaxWidth && ( !isfinite(maxWidth) || (maxWidth <= 0) ))

Remove the spaces between ( and ! as well as the last space. The braces around maxWidth <= 0  seem to be unnecessary too.

Did you test that on Firefox, Opera and IE as well? Do they not draw the text? If they do, we should keep it the same way.

[Rashmi Shyamasundar]

This issue is not being reproducible on latest WebKit-efl port.

In windows port, with the latest nightly build - WebKit r131444, the issue is still reproducible when maxWidth is zero. 

The W3 compliance test - http://w3c-test.org/html/tests/approved/canvas/2d.text.draw.fill.maxWidth.zero.html is failing on WebKit r131444.

[Dirk Schulze]

(In reply to comment #13)
> This issue is not being reproducible on latest WebKit-efl port.
> 
> In windows port, with the latest nightly build - WebKit r131444, the issue is still reproducible when maxWidth is zero. 
> 
> The W3 compliance test - http://w3c-test.org/html/tests/approved/canvas/2d.text.draw.fill.maxWidth.zero.html is failing on WebKit r131444.

Again, I have less concerns about compatibility to a spec that is still in flux then interoperability to other browsers. Can you check the other browsers as well please? Not just WebKit browsers?

[Rashmi Shyamasundar]

On Opera(Version 12.12):
maxWidth = 0 : Text is not displayed
maxWidth = -1 : Text is displayed 

On FireFox(Version 18.0.2):
maxWidth = 0 : Text is displayed
maxWidth = -1 : Text is not displayed 

Behavior of FireFox is opposite to the behavior of Opera.

IE9 does not support canvas.

Currently WebKit-Windows port is behaving like FireFox.

[Benjamin Poulain]

(In reply to comment #15)
> On Opera(Version 12.12):
> maxWidth = 0 : Text is not displayed
> maxWidth = -1 : Text is displayed 
> 
> On FireFox(Version 18.0.2):
> maxWidth = 0 : Text is displayed
> maxWidth = -1 : Text is not displayed 

Okay, those make no sense. Their implementations probably do not check anything.

I am in favor of following the spec, reviewing this, and filing bugs for Firefox/Opera.
Dirk, what is your opinion?

[Dirk Schulze]

(In reply to comment #16)
> (In reply to comment #15)
> > On Opera(Version 12.12):
> > maxWidth = 0 : Text is not displayed
> > maxWidth = -1 : Text is displayed 
> > 
> > On FireFox(Version 18.0.2):
> > maxWidth = 0 : Text is displayed
> > maxWidth = -1 : Text is not displayed 
> 
> Okay, those make no sense. Their implementations probably do not check anything.
> 
> I am in favor of following the spec, reviewing this, and filing bugs for Firefox/Opera.
> Dirk, what is your opinion?

I agree.

[Dirk Schulze]

Comment on attachment 186827 [details]
Patch

Thanks for testing this on other browsers. The patch looks good to me.Please fix the style issues before landing.

[Rashmi Shyamasundar]

Created attachment 187797 [details]
Patch

[Rashmi Shyamasundar]

In the latest patch -
 1. I have made the style changes as per review-comments by Mr. Dirk Schulze. 
 2. In the layout tests, I have increased the canvas-size and font-size, just to  make sure that the tests are anytime, not fragile. Canvas-size=100*50. font-size=35px.

[Dirk Schulze]

Comment on attachment 187797 [details]
Patch

LGTM. Please set the cq? flag as well in the future if you want the patch landed by the bots. r=me.

[WebKit Review Bot]

Comment on attachment 187797 [details]
Patch

Clearing flags on attachment: 187797

Committed r142754: <http://trac.webkit.org/changeset/142754>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.