589552 - (ietestcenter) HTML5 Canvas 4/15: canvas linear gradient must paint nothing

Status: RESOLVED FIXED

(Core :: Graphics: Canvas2D, defect)

Description

[Darxus]

User-Agent:       Mozilla/5.0 (X11; Linux x86_64; rv:2.0b4pre) Gecko/20100817 Minefield/4.0b4pre
Build Identifier: Trunk

Fails test.

Reproducible: Always

Comment 1

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Also tested by

http://www.w3c-test.org/html/tests/approved/canvas/2d.gradient.interpolate.zerosize.html

Comment 2

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Patch v1

It feels a little hacky, but it seems impossible to get the coordinates back out of the nsCanvasGradient.

http://www.whatwg.org/html/#dom-context-2d-createlineargradient

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Perhaps some gfx dev knows whether something could be added to gfxPattern,
so that the hacky mIsEmpty wouldn't be needed.

Comment 4

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 538679 [details] [diff] [review]
Patch v1

I would really prefer a solution that doesn't introduce new state that can be inconsistent with existing state. Indeed gfxPattern doesn't seem to expose the coords at the moment, but maybe that can be changed? Internally it seems to just have a cairo_pattern_t, and the function to get the points from it seems to be 

cairo_pattern_get_linear_points (cairo_pattern_t *pattern,
				 double *x0, double *y0,
 				 double *x1, double *y1)

http://mxr.mozilla.org/mozilla-central/source/gfx/cairo/cairo/src/cairo-pattern.c#3142

Comment 5

[Benoit Jacob [:bjacob] (mostly away)]

Ask Jeff if you have Cairo questions.

Comment 6

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Patch v2

How about this, then?

Comment 7

[Joe Drew (not getting mail)]

Drive-by comment: I really don't think we should be calling cairo functions bare like that. Add stuff to Thebes if it's necessary, IMO.

Comment 8

[Benoit Jacob [:bjacob] (mostly away)]

What the status of this? Ms2ger, do you need help to make this change to Thebes?

Comment 9

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Patch v3

Comment 10

[Jeff Muizelaar [:jrmuizel]]

This will need a test. It would also be good to know what the behaviour of other browsers on the test is. Is IE the only browser following the spec? etc.

Comment 11

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

It has a test. According to my data, Chrome and Opera pass the test. Anything else?

Comment 12

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 552726 [details] [diff] [review]
Patch v3

Jeff will be more competent than me to review this.

Comment 13

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Jeff, ping.

Comment 14

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 552726 [details] [diff] [review]
Patch v3

Do we need an version of this for the Azure canvas implementation?

Comment 15

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 552726 [details] [diff] [review]
Patch v3

Review of attachment 552726 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/canvas/src/nsCanvasRenderingContext2D.cpp
@@ +104,1 @@
>  

Why is this needed? We don't really want cairo stuff sneaking out into thebes callers.

@@ +1706,2 @@
>      nsRefPtr<nsIDOMCanvasGradient> grad = new nsCanvasGradient(gradpat);
> +    grad.forget(_retval);

Please separate cleanups out into a separate patch from web visible behavior changes.

@@ +2056,5 @@
>  {
> +    nsCanvasGradient* gradient = CurrentState().gradientStyles[STYLE_FILL];
> +    if (gradient && gradient->GetPattern()->IsCollapsedLinearGradient()) {
> +        return NS_OK;
> +    }

Why do we only need to check this in FillRect()? I also don't know what cairo's behaviour for zero length linear gradients is and whether there's a good reason for it. Perhaps cairo's just doing the wrong thing here.

::: gfx/thebes/gfxPattern.cpp
@@ +198,5 @@
>  }
> +
> +bool
> +gfxPattern::IsCollapsedLinearGradient()
> +{

It wasn't obvious to me from the name what a collapsed linear gradient is. I wonder if a better name for this would be ZeroLengthLinearGradient()

Comment 16

[Jeff Muizelaar [:jrmuizel]]

And sorry for taking so long to review.

Comment 17

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #15)
> Comment on attachment 552726 [details] [diff] [review] [diff] [details] [review]
> Patch v3
> 
> Review of attachment 552726 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> ::: content/canvas/src/nsCanvasRenderingContext2D.cpp
> @@ +104,1 @@
> >  
> 
> Why is this needed? We don't really want cairo stuff sneaking out into
> thebes callers.

If you mean the include, that's a leftover from a previous patch I'll remove.

> @@ +1706,2 @@
> >      nsRefPtr<nsIDOMCanvasGradient> grad = new nsCanvasGradient(gradpat);
> > +    grad.forget(_retval);
> 
> Please separate cleanups out into a separate patch from web visible behavior
> changes.

Sure.

> @@ +2056,5 @@
> >  {
> > +    nsCanvasGradient* gradient = CurrentState().gradientStyles[STYLE_FILL];
> > +    if (gradient && gradient->GetPattern()->IsCollapsedLinearGradient()) {
> > +        return NS_OK;
> > +    }
> 
> Why do we only need to check this in FillRect()? I also don't know what
> cairo's behaviour for zero length linear gradients is and whether there's a
> good reason for it. Perhaps cairo's just doing the wrong thing here.

I have no idea what cairo's contract says about this. I'll have a look if anything else needs changing.

> ::: gfx/thebes/gfxPattern.cpp
> @@ +198,5 @@
> >  }
> > +
> > +bool
> > +gfxPattern::IsCollapsedLinearGradient()
> > +{
> 
> It wasn't obvious to me from the name what a collapsed linear gradient is. I
> wonder if a better name for this would be ZeroLengthLinearGradient()

OK.

Comment 18

[Sylvestre Ledru [:Sylvestre]]

No assignee, updating the status.

Comment 19

[Sylvestre Ledru [:Sylvestre]]

No assignee, updating the status.

Comment 20

[Mayank Bansal]

Direct links do not work, and we diligently track w3c test-suite now.