Bug 102654 - Zero size gradient should paint nothing on canvas
Summary: Zero size gradient should paint nothing on canvas
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Canvas (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL: http://w3c-test.org/html/tests/appro...
Keywords:
Depends on:
Blocks: 102656
  Show dependency treegraph
 
Reported: 2012-11-19 00:26 PST by Rashmi Shyamasundar
Modified: 2013-02-01 11:06 PST (History)
28 users (show)

See Also:


Attachments
Patch (15.89 KB, patch)
2012-12-06 02:29 PST, Rashmi Shyamasundar
no flags Details | Formatted Diff | Diff
Patch (15.83 KB, patch)
2013-01-07 19:55 PST, Rashmi Shyamasundar
no flags Details | Formatted Diff | Diff
Patch (19.42 KB, patch)
2013-01-28 21:08 PST, Rashmi Shyamasundar
no flags Details | Formatted Diff | Diff
Patch (23.90 KB, patch)
2013-01-28 23:17 PST, Rashmi Shyamasundar
no flags Details | Formatted Diff | Diff
Patch (19.39 KB, patch)
2013-01-31 04:09 PST, Rashmi Shyamasundar
no flags Details | Formatted Diff | Diff
Patch (19.39 KB, patch)
2013-01-31 20:54 PST, Rashmi Shyamasundar
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Rashmi Shyamasundar 2012-11-19 00:26:26 PST
Function prototype :- CanvasRenderingContext2D::createLinearGradient(float x0, float y0, float x1, float y1, ExceptionCode& ec)

The HTML5 spec (http://www.whatwg.org/specs/web-apps/current-work/#dom-context-2d-createlineargradient)says - "If x0 = x1 and y0 = y1, then the linear gradient must paint nothing."

Currently the functions fill(), fillText(), stroke(), strokeRect() and strokeText() are not checking this condition.

Failing W3C tests :-

http://w3c-test.org/html/tests/approved/canvas/2d.gradient.interpolate.zerosize.fill.html
http://w3c-test.org/html/tests/approved/canvas/2d.gradient.interpolate.zerosize.fillText.html
http://w3c-test.org/html/tests/approved/canvas/2d.gradient.interpolate.zerosize.stroke.html        
http://w3c-test.org/html/tests/approved/canvas/2d.gradient.interpolate.zerosize.strokeRect.html        
http://w3c-test.org/html/tests/approved/canvas/2d.gradient.interpolate.zerosize.strokeText.html
Comment 1 Rashmi Shyamasundar 2012-12-06 02:29:31 PST
Created attachment 177980 [details]
Patch
Comment 2 Rashmi Shyamasundar 2012-12-06 02:33:13 PST
The attached patch introduces a check for zero-gradient in the functions fill(), stroke(), strokeRect() and drawTextInternal(), in the class CanvasRenderingContext2D.
Comment 3 Chris Dumez 2012-12-06 02:52:30 PST
For the record, Firefox does not seem to follow the spec either here.
Comment 4 Gyuyoung Kim 2012-12-26 18:46:22 PST
CC'ing Darin and Dirk, could you take a look this patch ?
Comment 5 Dirk Schulze 2012-12-26 19:12:43 PST
(In reply to comment #4)
> CC'ing Darin and Dirk, could you take a look this patch ?

If FF does not follow the spec, why should we? Are we compatible with FF? What are Opera and IE doing?
Comment 6 Rashmi Shyamasundar 2012-12-26 20:21:44 PST
These W3C tests are
1. Passing on Opera
2. Failing on FireFox and IE.
Comment 7 Dirk Schulze 2012-12-26 21:11:33 PST
(In reply to comment #6)
> These W3C tests are
> 1. Passing on Opera
> 2. Failing on FireFox and IE.

Again, are WebKit, IE and FF interoperable? If yes, the spec should change IMO.
Comment 8 Darin Adler 2012-12-27 10:05:24 PST
(In reply to comment #7)
> (In reply to comment #6)
> > These W3C tests are
> > 1. Passing on Opera
> > 2. Failing on FireFox and IE.
> 
> Again, are WebKit, IE and FF interoperable? If yes, the spec should change IMO.

I agree with Dirk. This feature has been in browsers for years, and we should probably not change browser behavior if only Opera matches the spec. Lets investigate getting the specification fixed before we break with Firefox, IE, and WebKit’s own historical behavior in the name of matching the specification wording.
Comment 9 Rik Cabanier 2012-12-28 13:00:22 PST
(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #6)
> > > These W3C tests are
> > > 1. Passing on Opera
> > > 2. Failing on FireFox and IE.
> > 
> > Again, are WebKit, IE and FF interoperable? If yes, the spec should change IMO.
> 
> I agree with Dirk. This feature has been in browsers for years, and we should probably not change browser behavior if only Opera matches the spec. Lets investigate getting the specification fixed before we break with Firefox, IE, and WebKit’s own historical behavior in the name of matching the specification wording.

I tried the testcases on ie 10 and it is following the spec. That means that we have 2 shipping implementations.
It really makes sense to disallow this edge case. If the start and end point of a gradient coincide, there is no 'direction' so it's impossible to tell how the gradient is supposed to draw. 
My vote is to fix this in the webkit and mozilla implementations.
Comment 10 Dirk Schulze 2012-12-30 15:28:51 PST
(In reply to comment #6)
> These W3C tests are
> 1. Passing on Opera
> 2. Failing on FireFox and IE.

Rik said it is passing IE. I can't verify it on my own ATM. Which IE version did you try? Was it desktop or mobile IE?
Comment 11 Rashmi Shyamasundar 2012-12-31 01:32:50 PST
I see that the tests are failing on desktop-IE 8 and 9.
Comment 12 Rik Cabanier 2012-12-31 17:05:09 PST
(In reply to comment #10)
> (In reply to comment #6)
> > These W3C tests are
> > 1. Passing on Opera
> > 2. Failing on FireFox and IE.
> 
> Rik said it is passing IE. I can't verify it on my own ATM. Which IE version did you try? Was it desktop or mobile IE?

IE 10 on Win7 and Windows Phone 8
Comment 13 Dirk Schulze 2012-12-31 18:05:05 PST
This is what SVG is saying:

""
If ‘x1’ = ‘x2’ and ‘y1’ = ‘y2’, then the area to be painted will be painted as a single color using the color and opacity of the last gradient stop.
""

Wouldn't it make sense to align the specs in this point? Isn't it that what WebKit and Firefox are doing? It is not important if something makes sense from a mathematical point of view, if it is well defined. In this case Canvas should follow SVG IMO.
Comment 14 Rik Cabanier 2012-12-31 21:19:12 PST
(In reply to comment #13)
> This is what SVG is saying:
> 
> ""
> If ‘x1’ = ‘x2’ and ‘y1’ = ‘y2’, then the area to be painted will be painted as a single color using the color and opacity of the last gradient stop.
> ""
> 
> Wouldn't it make sense to align the specs in this point? Isn't it that what WebKit and Firefox are doing? It is not important if something makes sense from a mathematical point of view, if it is well defined. In this case Canvas should follow SVG IMO.

Who makes that call? There are 2 implementations that follow the spec. Do we know if WebKit and Firefox are following the SVG spec or do they each have different behavior?

FYI the canvas spec is in CR so if you want to make this change, it will have to move to LC again.
Comment 15 Dirk Schulze 2012-12-31 22:40:11 PST
(In reply to comment #14)
> (In reply to comment #13)
> > This is what SVG is saying:
> > 
> > ""
> > If ‘x1’ = ‘x2’ and ‘y1’ = ‘y2’, then the area to be painted will be painted as a single color using the color and opacity of the last gradient stop.
> > ""
> > 
> > Wouldn't it make sense to align the specs in this point? Isn't it that what WebKit and Firefox are doing? It is not important if something makes sense from a mathematical point of view, if it is well defined. In this case Canvas should follow SVG IMO.
> 
> Who makes that call? There are 2 implementations that follow the spec. Do we know if WebKit and Firefox are following the SVG spec or do they each have different behavior?

I try to verify this on Wednesday when I am back in the office.

> 
> FYI the canvas spec is in CR so if you want to make this change, it will have to move to LC again.

I don't care about going back to LC for Canvas. I care about interoperable and consistent specs and implementations. And we also seem to have two consistent implementations for another behavior, this argument doesn't really count. If it turns out that webkit and firefox are consistent with SVG, I will request the change for Canvas.
Comment 16 Rashmi Shyamasundar 2013-01-01 22:07:48 PST
Webkit(checked on windows port) does not conform to SVG spec.
FireFox conforms to SVG spec. I used the below test to check the conformance to SVG spec - 

In the test - http://w3c-test.org/html/tests/approved/canvas/2d.gradient.interpolate.zerosize.fill.html , replace the _addTest function-definition with the below definition, to check conformance to SVG spec :
 
_addTest(function(canvas, ctx) {

ctx.fillStyle = 'blue';
ctx.fillRect(0, 0, 100, 50);

var g = ctx.createLinearGradient(50, 25, 50, 25); // zero-length line (undefined direction)
g.addColorStop(0, 'yellow');
g.addColorStop(1, 'rgb(0,255,0)');
ctx.fillStyle = g;
ctx.rect(0, 0, 100, 50);
ctx.fill();
_assertPixel(canvas, 40,20, 0,255,0,255, "40,20", "0,255,0,255");
Comment 17 Dirk Schulze 2013-01-02 10:10:11 PST
> Webkit(checked on windows port) does not conform to SVG spec.
> FireFox conforms to SVG spec. I used the below test to check the conformance to SVG spec - 
> 
> In the test - http://w3c-test.org/html/tests/approved/canvas/2d.gradient.interpolate.zerosize.fill.html , replace the _addTest function-definition with the below definition, to check conformance to SVG spec :
> 
> _addTest(function(canvas, ctx) {
> 
> ctx.fillStyle = 'blue';
> ctx.fillRect(0, 0, 100, 50);
> 
> var g = ctx.createLinearGradient(50, 25, 50, 25); // zero-length line (undefined direction)
> g.addColorStop(0, 'yellow');
> g.addColorStop(1, 'rgb(0,255,0)');
> ctx.fillStyle = g;
> ctx.rect(0, 0, 100, 50);
> ctx.fill();
> _assertPixel(canvas, 40,20, 0,255,0,255, "40,20", "0,255,0,255");

Hm, I actually can not confirm the behavior:

<html>
<canvas id="c" width="100" height="100"></canvas>
<script>
    var ctx = document.getElementById('c').getContext('2d');
    var lingrad = ctx.createLinearGradient(50,50,50,50);
    lingrad.addColorStop(0, 'yellow');
    lingrad.addColorStop(0.5, 'blue');
    lingrad.addColorStop(1, 'green');
    ctx.strokeRect(0,0,100,100);
    ctx.fillStyle = lingrad;
    ctx.fillRect(0,0,100,100);
</script>
</html>​

See http://fiddle.jshell.net/kHnnJ/1/

Firefox draws a vertical green stripe next to a vertical yellow stripe. This is not correct according to SVG or Canvas spec.

Safari and Chrome don't paint anything.
Comment 18 Dirk Schulze 2013-01-02 10:12:48 PST
(In reply to comment #17)

> Firefox draws a vertical green stripe next to a vertical yellow stripe. This is not correct according to SVG or Canvas spec.
> 
> Safari and Chrome don't paint anything.

IE and Opera don't paint anything either.
Comment 19 Rik Cabanier 2013-01-02 10:33:16 PST
(In reply to comment #18)
> (In reply to comment #17)
> 
> > Firefox draws a vertical green stripe next to a vertical yellow stripe. This is not correct according to SVG or Canvas spec.
> > 
> > Safari and Chrome don't paint anything.
> 
> IE and Opera don't paint anything either.

So we have 3 independent implementations...
Comment 20 Dirk Schulze 2013-01-02 10:48:57 PST
(In reply to comment #19)
> (In reply to comment #18)
> > (In reply to comment #17)
> > 
> > > Firefox draws a vertical green stripe next to a vertical yellow stripe. This is not correct according to SVG or Canvas spec.
> > > 
> > > Safari and Chrome don't paint anything.
> > 
> > IE and Opera don't paint anything either.
> 
> So we have 3 independent implementations...

I see the problem now. There is a difference between using

    fillRect(0,0,100,100)

and 

    rect(0,0,100,100)
    fill();

Means we use a different logic for Path and Rect, which is not good.
Comment 21 Ian 'Hixie' Hickson 2013-01-02 12:14:26 PST
(In reply to comment #14)
> Who makes that call?

This is basically the browsers' call. The WHATWG spec will side with whatever is most widely implemented when someone mentions that it doesn't match everyone.

 
> FYI the canvas spec is in CR so if you want to make this change, it will have to move to LC again.

That's the advantage of living standards. They don't have to worry about such things. :-)


I recommend basically coming up with a decision, implementing it (consistently for fillRect() and rec();fill(), ideally!) and then letting the whatwg list know that this is an area with interop issues and summarising what browsers do so that I can update the spec accordingly.
Comment 22 Dirk Schulze 2013-01-02 15:50:42 PST
Comment on attachment 177980 [details]
Patch

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

Given that we already have this check for fillRect and Opera and IE are compatible with the spec, I think we should go ahead with this.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:2249
> +    // If gradient size is zero, then paint nothing.
> +    Gradient* gradient = c->strokeGradient();
> +    if (gradient && gradient->isZeroSize())
> +        return;
> +
> +    // If gradient size is zero, then paint nothing.
> +    gradient = c->fillGradient();
> +    if (gradient && gradient->isZeroSize())
> +        return;

I am not sure about this part. Why shouldn't we fill, if just the stroke gradient is bogus? (Same the other way around.)

> LayoutTests/canvas/philip/tests.js:195
> +
> +

remove these extra lines.
Comment 23 Rashmi Shyamasundar 2013-01-07 19:55:44 PST
Created attachment 181627 [details]
Patch
Comment 24 Rashmi Shyamasundar 2013-01-07 20:07:13 PST
The attachment has changes as described in  Comment #22.

In the function CanvasRenderingContext2D::drawTextInternal(), if value of the boolean variable "fill" is true, then the fill-gradient is checked for zero-size. Otherwise the stroke-gradient is checked for zero-size.
Comment 25 Eric Seidel (no email) 2013-01-17 23:51:11 PST
I don't know who is working on our canvas code these days, but it's certainly not me.  Oliver did long ago too, but I don't think he is these days either.
Comment 26 Dirk Schulze 2013-01-18 07:26:24 PST
Comment on attachment 181627 [details]
Patch

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

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:2251
> +    // If gradient size is zero, then paint nothing.
> +    Gradient* gradient = c->strokeGradient();
> +    if (!fill && gradient && gradient->isZeroSize())
> +        return;
> +
> +    gradient = c->fillGradient();
> +    if (fill && gradient && gradient->isZeroSize())
> +        return;

Now I got it. You can just do one operation per call. Either fill or stroke. LGTM.

> LayoutTests/ChangeLog:23
> +        * canvas/philip/tests/2d.gradient.interpolate.zerosize.fill-expected.txt: Added.
> +        * canvas/philip/tests/2d.gradient.interpolate.zerosize.fill.html: Added.
> +        * canvas/philip/tests/2d.gradient.interpolate.zerosize.fillText-expected.txt: Added.
> +        * canvas/philip/tests/2d.gradient.interpolate.zerosize.fillText.html: Added.
> +        * canvas/philip/tests/2d.gradient.interpolate.zerosize.stroke-expected.txt: Added.
> +        * canvas/philip/tests/2d.gradient.interpolate.zerosize.stroke.html: Added.
> +        * canvas/philip/tests/2d.gradient.interpolate.zerosize.strokeRect-expected.txt: Added.
> +        * canvas/philip/tests/2d.gradient.interpolate.zerosize.strokeRect.html: Added.
> +        * canvas/philip/tests/2d.gradient.interpolate.zerosize.strokeText-expected.txt: Added.
> +        * canvas/philip/tests/2d.gradient.interpolate.zerosize.strokeText.html: Added.

Are you extending the test suite from philip here, or fixing it? I would rather not touch this folder if there is nothing obviously wrong in it. Can you write tests and put them to fast/canvas instead? Please avoid the test structure from this test suite and follow the other tests in fast/canvas.
Comment 27 Rashmi Shyamasundar 2013-01-28 21:08:59 PST
Created attachment 185144 [details]
Patch
Comment 28 Early Warning System Bot 2013-01-28 21:13:17 PST
Comment on attachment 185144 [details]
Patch

Attachment 185144 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/16155936
Comment 29 Peter Beverloo (cr-android ews) 2013-01-28 21:20:57 PST
Comment on attachment 185144 [details]
Patch

Attachment 185144 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/16161953
Comment 30 kov's GTK+ EWS bot 2013-01-28 21:35:20 PST
Comment on attachment 185144 [details]
Patch

Attachment 185144 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/16155948
Comment 31 Build Bot 2013-01-28 21:36:09 PST
Comment on attachment 185144 [details]
Patch

Attachment 185144 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/16152973
Comment 32 EFL EWS Bot 2013-01-28 23:07:57 PST
Comment on attachment 185144 [details]
Patch

Attachment 185144 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/16192019
Comment 33 Build Bot 2013-01-28 23:16:04 PST
Comment on attachment 185144 [details]
Patch

Attachment 185144 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16202007
Comment 34 Rashmi Shyamasundar 2013-01-28 23:17:11 PST
Created attachment 185172 [details]
Patch
Comment 35 Rashmi Shyamasundar 2013-01-29 00:51:07 PST
The new patch has test cases under the directory - LayoutTests/fast/canvas.
Comment 36 Build Bot 2013-01-29 01:35:27 PST
Comment on attachment 185172 [details]
Patch

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

New failing tests:
fast/workers/worker-document-leak.html
Comment 37 Rashmi Shyamasundar 2013-01-29 02:21:45 PST
The patch is passing on all the ports except mac-wk2.

On mac-wk2, the test "fast/workers/worker-document-leak.html" is failing. This test is not related to my patch.
Comment 38 Dirk Schulze 2013-01-29 05:57:27 PST
Comment on attachment 185172 [details]
Patch

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

Nearly done. Did you investigate why mac-wk2 is failing? Shouldn't canvas tests run on WK2 nowadays?

> LayoutTests/fast/canvas/2d.gradient.interpolate.zerosize.fill.html:33
> +var canvas = document.getElementById('c');
> +var ctx = canvas.getContext("2d");
> +drawCanvas(ctx);
> +if (_assertPixel(canvas, 40,20, 0,255,0,255) == true)
> +    document.getElementById("console").innerHTML = "TEST PASSED";
> +else

I would really appreciate if you could transform these tests into more WebKit Canvas style tests like fast/canvas/canvas-clearRect.html for example. The test itself looks fine. This is more of a style change request.
Comment 39 Dirk Schulze 2013-01-29 08:15:45 PST
(In reply to comment #37)
> The patch is passing on all the ports except mac-wk2.
> 
> On mac-wk2, the test "fast/workers/worker-document-leak.html" is failing. This test is not related to my patch.

That seems not to be true. Here the log from the WK2 bot:

Unexpected flakiness: text-only failures (7)
  canvas/philip/tests/2d.text.draw.fontface.notinpage.html [ Failure Pass ]
  fast/workers/worker-lifecycle.html [ Failure Pass ]
  fast/writing-mode/vertical-rl-replaced-selection.html [ Failure Pass ]
  http/tests/notifications/window-show-on-click.html [ Failure Pass ]
  platform/mac/fast/forms/input-number-click.html [ Failure Pass ]
  svg/animations/smil-leak-dynamically-added-element-instances.svg [ Failure Pass ]
  svg/animations/smil-leak-elements.svg [ Failure Pass ]


Regressions: Unexpected text-only failures (9)
  canvas/philip/tests/2d.path.arc.scale.2.html [ Failure ]
  canvas/philip/tests/2d.path.arcTo.scale.html [ Failure ]
  canvas/philip/tests/2d.path.arcTo.transformation.html [ Failure ]
  canvas/philip/tests/2d.path.stroke.scale1.html [ Failure ]
  canvas/philip/tests/2d.path.stroke.skew.html [ Failure ]
  fast/canvas/webgl/read-pixels-test.html [ Failure ]
  fast/js/regress/integer-modulo.html [ Failure ]
  fast/workers/worker-close-more.html [ Failure ]
  fast/workers/worker-document-leak.html [ Failure ]

I am not sure how this is related to your code, but they might use gradients.
Comment 40 Rashmi Shyamasundar 2013-01-31 04:09:36 PST
Created attachment 185741 [details]
Patch
Comment 41 WebKit Review Bot 2013-01-31 07:06:04 PST
Comment on attachment 185741 [details]
Patch

Attachment 185741 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16264267

New failing tests:
fast/canvas/canvas-fill-zeroSizeGradient.html
platform/chromium/virtual/gpu/fast/canvas/canvas-fill-zeroSizeGradient.html
Comment 42 Build Bot 2013-01-31 08:08:48 PST
Comment on attachment 185741 [details]
Patch

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

New failing tests:
fast/canvas/canvas-fill-zeroSizeGradient.html
Comment 43 Build Bot 2013-01-31 13:19:31 PST
Comment on attachment 185741 [details]
Patch

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

New failing tests:
fast/canvas/canvas-fill-zeroSizeGradient.html
Comment 44 Rashmi Shyamasundar 2013-01-31 20:54:05 PST
Created attachment 185924 [details]
Patch
Comment 45 Dirk Schulze 2013-02-01 02:45:02 PST
Comment on attachment 185924 [details]
Patch

LGTM. r=me.
Comment 46 Rashmi Shyamasundar 2013-02-01 03:59:42 PST
Comment on attachment 185924 [details]
Patch

Please approve the patch for commit.
Comment 47 WebKit Review Bot 2013-02-01 11:06:17 PST
Comment on attachment 185924 [details]
Patch

Clearing flags on attachment: 185924

Committed r141612: <http://trac.webkit.org/changeset/141612>
Comment 48 WebKit Review Bot 2013-02-01 11:06:26 PST
All reviewed patches have been landed.  Closing bug.