Bug 102654 – Zero size gradient should paint nothing on canvas

• Component: Canvas
• Status: RESOLVED FIXED

[Rashmi Shyamasundar]

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

[Rashmi Shyamasundar]

Created attachment 177980 [details]
Patch

[Rashmi Shyamasundar]

The attached patch introduces a check for zero-gradient in the functions fill(), stroke(), strokeRect() and drawTextInternal(), in the class CanvasRenderingContext2D.

[Chris Dumez]

For the record, Firefox does not seem to follow the spec either here.

[Gyuyoung Kim]

CC'ing Darin and Dirk, could you take a look this patch ?

[Dirk Schulze]

(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?

[Rashmi Shyamasundar]

These W3C tests are
1. Passing on Opera
2. Failing on FireFox and IE.

[Dirk Schulze]

(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.

[Darin Adler]

(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.

[Rik Cabanier]

(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.

[Dirk Schulze]

(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?

[Rashmi Shyamasundar]

I see that the tests are failing on desktop-IE 8 and 9.

[Rik Cabanier]

(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

[Dirk Schulze]

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.

[Rik Cabanier]

(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.

[Dirk Schulze]

(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.

[Rashmi Shyamasundar]

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");

[Dirk Schulze]

> 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.

[Dirk Schulze]

(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.

[Rik Cabanier]

(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...

[Dirk Schulze]

(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.

[Ian 'Hixie' Hickson]

(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.

[Dirk Schulze]

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.

[Rashmi Shyamasundar]

Created attachment 181627 [details]
Patch

[Rashmi Shyamasundar]

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.

[Eric Seidel (no email)]

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.

[Dirk Schulze]

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.

[Rashmi Shyamasundar]

Created attachment 185144 [details]
Patch

[Early Warning System Bot]

Comment on attachment 185144 [details]
Patch

Attachment 185144 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/16155936

[Peter Beverloo (cr-android ews)]

Comment on attachment 185144 [details]
Patch

Attachment 185144 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/16161953

[kov's GTK+ EWS bot]

Comment on attachment 185144 [details]
Patch

Attachment 185144 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/16155948

[Build Bot]

Comment on attachment 185144 [details]
Patch

Attachment 185144 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/16152973

[EFL EWS Bot]

Comment on attachment 185144 [details]
Patch

Attachment 185144 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/16192019

[Build Bot]

Comment on attachment 185144 [details]
Patch

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

[Rashmi Shyamasundar]

Created attachment 185172 [details]
Patch

[Rashmi Shyamasundar]

The new patch has test cases under the directory - LayoutTests/fast/canvas.

[Build Bot]

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

[Rashmi Shyamasundar]

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.

[Dirk Schulze]

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.

[Dirk Schulze]

(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.

[Rashmi Shyamasundar]

Created attachment 185741 [details]
Patch

[WebKit Review Bot]

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

[Build Bot]

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

[Build Bot]

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

[Rashmi Shyamasundar]

Created attachment 185924 [details]
Patch

[Dirk Schulze]

Comment on attachment 185924 [details]
Patch

LGTM. r=me.

[Rashmi Shyamasundar]

Comment on attachment 185924 [details]
Patch

Please approve the patch for commit.

[WebKit Review Bot]

Comment on attachment 185924 [details]
Patch

Clearing flags on attachment: 185924

Committed r141612: <http://trac.webkit.org/changeset/141612>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.