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"
Created attachment 177465 [details] Patch
The attached patch introduces a check for negative-maxWidth in the function CanvasRenderingContext2D::drawTextInternal().
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.
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.
(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.
This w3c test is 1. Passing on Opera 2. Failing on IE, FireFox, Safari.
Created attachment 186533 [details] Patch
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?
Created attachment 186820 [details] Patch
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:
Created attachment 186827 [details] Patch
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.
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.
(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?
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.
(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?
(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.
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.
Created attachment 187797 [details] Patch
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.
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.
Comment on attachment 187797 [details] Patch Clearing flags on attachment: 187797 Committed r142754: <http://trac.webkit.org/changeset/142754>
All reviewed patches have been landed. Closing bug.