Open
Bug 589552
Opened 14 years ago
Updated 2 years ago
(ietestcenter) HTML5 Canvas 4/15: canvas linear gradient must paint nothing
Categories
(Core :: Graphics: Canvas2D, defect)
Core
Graphics: Canvas2D
Tracking
()
NEW
People
(Reporter: darxus-mozillabug, Unassigned)
References
()
Details
(Keywords: html5)
Attachments
(1 file, 2 obsolete files)
(deleted),
patch
|
jrmuizel
:
review-
|
Details | Diff | Splinter Review |
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:2.0b4pre) Gecko/20100817 Minefield/4.0b4pre
Build Identifier: Trunk
Fails test.
Reproducible: Always
Blocks: ietestcenter
Summary: (ietestcenter) Does not create gradient → (ietestcenter) HTML5 Canvas
Summary: (ietestcenter) HTML5 Canvas → (ietestcenter) HTML5 Canvas 4/15: canvas linear gradient must paint nothing
Updated•14 years ago
|
Status: UNCONFIRMED → NEW
Component: General → Canvas: 2D
Ever confirmed: true
Keywords: html5
OS: Linux → All
QA Contact: general → canvas.2d
Hardware: x86_64 → All
Comment 1•14 years ago
|
||
Also tested by
http://www.w3c-test.org/html/tests/approved/canvas/2d.gradient.interpolate.zerosize.html
Blocks: 622842
Updated•14 years ago
|
Assignee: nobody → Ms2ger
Status: NEW → ASSIGNED
Comment 2•14 years ago
|
||
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
Attachment #538679 -
Flags: review?(Olli.Pettay)
Comment 3•14 years ago
|
||
Perhaps some gfx dev knows whether something could be added to gfxPattern,
so that the hacky mIsEmpty wouldn't be needed.
Updated•14 years ago
|
Attachment #538679 -
Flags: review?(Olli.Pettay) → review?(bjacob)
Comment 4•14 years ago
|
||
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
Attachment #538679 -
Flags: review?(bjacob) → review-
Comment 5•14 years ago
|
||
Ask Jeff if you have Cairo questions.
Comment 6•14 years ago
|
||
How about this, then?
Attachment #538679 -
Attachment is obsolete: true
Attachment #538929 -
Flags: review?(bjacob)
Comment 7•14 years ago
|
||
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•13 years ago
|
||
What the status of this? Ms2ger, do you need help to make this change to Thebes?
Updated•13 years ago
|
Attachment #538929 -
Flags: review?(bjacob)
Comment 9•13 years ago
|
||
Attachment #538929 -
Attachment is obsolete: true
Attachment #552726 -
Flags: review?(bjacob)
Comment 10•13 years ago
|
||
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•13 years ago
|
||
It has a test. According to my data, Chrome and Opera pass the test. Anything else?
Comment 12•13 years ago
|
||
Comment on attachment 552726 [details] [diff] [review]
Patch v3
Jeff will be more competent than me to review this.
Attachment #552726 -
Flags: review?(bjacob) → review?(jmuizelaar)
Comment 13•13 years ago
|
||
Jeff, ping.
Comment 14•13 years ago
|
||
Comment on attachment 552726 [details] [diff] [review]
Patch v3
Do we need an version of this for the Azure canvas implementation?
Comment 15•13 years ago
|
||
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()
Attachment #552726 -
Flags: review?(jmuizelaar) → review-
Comment 16•13 years ago
|
||
And sorry for taking so long to review.
Comment 17•13 years ago
|
||
(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.
Updated•12 years ago
|
Assignee: Ms2ger → nobody
Comment 19•6 years ago
|
||
No assignee, updating the status.
Updated•2 years ago
|
Severity: normal → S3
You need to log in
before you can comment on or make changes to this bug.
Description
•