RESOLVED FIXED 102656
The 2D Canvas functions fillText()/strokeText() should display nothing when maxWidth is less then or equal to zero
https://bugs.webkit.org/show_bug.cgi?id=102656
Summary The 2D Canvas functions fillText()/strokeText() should display nothing when m...
Rashmi Shyamasundar
Reported 2012-11-19 01:00:44 PST
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"
Attachments
Patch (4.08 KB, patch)
2012-12-04 03:25 PST, Rashmi Shyamasundar
no flags
Patch (6.15 KB, patch)
2013-02-04 20:29 PST, Rashmi Shyamasundar
no flags
Patch (9.99 KB, patch)
2013-02-06 04:11 PST, Rashmi Shyamasundar
no flags
Patch (10.07 KB, patch)
2013-02-06 04:38 PST, Rashmi Shyamasundar
no flags
Patch (10.16 KB, patch)
2013-02-12 00:40 PST, Rashmi Shyamasundar
no flags
Rashmi Shyamasundar
Comment 1 2012-12-04 03:25:37 PST
Rashmi Shyamasundar
Comment 2 2012-12-04 03:27:50 PST
The attached patch introduces a check for negative-maxWidth in the function CanvasRenderingContext2D::drawTextInternal().
Chris Dumez
Comment 3 2012-12-04 03:34:46 PST
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
Comment 4 2012-12-06 02:43:36 PST
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
Comment 5 2012-12-06 02:46:51 PST
(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
Comment 6 2012-12-27 21:47:41 PST
This w3c test is 1. Passing on Opera 2. Failing on IE, FireFox, Safari.
Rashmi Shyamasundar
Comment 7 2013-02-04 20:29:34 PST
Benjamin Poulain
Comment 8 2013-02-04 23:22:31 PST
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
Comment 9 2013-02-06 04:11:26 PST
Kenneth Rohde Christiansen
Comment 10 2013-02-06 04:14:22 PST
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
Comment 11 2013-02-06 04:38:51 PST
Dirk Schulze
Comment 12 2013-02-06 15:38:26 PST
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
Comment 13 2013-02-07 02:33:50 PST
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
Comment 14 2013-02-07 15:37:54 PST
(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
Comment 15 2013-02-10 21:57:58 PST
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
Comment 16 2013-02-10 23:13:13 PST
(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
Comment 17 2013-02-11 08:10:45 PST
(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 18 2013-02-11 08:15:25 PST
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
Comment 19 2013-02-12 00:40:38 PST
Rashmi Shyamasundar
Comment 20 2013-02-12 00:47:03 PST
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 21 2013-02-13 08:50:39 PST
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 22 2013-02-13 09:03:11 PST
Comment on attachment 187797 [details] Patch Clearing flags on attachment: 187797 Committed r142754: <http://trac.webkit.org/changeset/142754>
WebKit Review Bot
Comment 23 2013-02-13 09:03:19 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.