Bug 6103 - <canvas> rectangle fills don't work with gradients
Summary: <canvas> rectangle fills don't work with gradients
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 420+
Hardware: Macintosh OS X 10.4
: P2 Normal
Assignee: Nobody
URL: http://developer.mozilla.org/samples/...
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2005-12-15 14:39 PST by David Carson
Modified: 2006-04-25 09:32 PDT (History)
0 users

See Also:


Attachments
Patch fillrect to support gradients (2.06 KB, patch)
2006-03-05 10:01 PST, David Carson
darin: review-
Details | Formatted Diff | Diff
patch including test and changelogs (5.65 KB, patch)
2006-03-05 17:51 PST, David Carson
no flags Details | Formatted Diff | Diff
patch (5.97 KB, patch)
2006-03-05 20:06 PST, David Carson
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Carson 2005-12-15 14:39:12 PST
Mozilla test case fails as the call to fillRect() does not use the specified
fillstyle (in this case a gradient).

Possible solution is implementing FillRect in kjs_html.cpp by building a path,
and using the same Fill code as filling a normal path:

        case Context2D::FillRect: {
            if (args.size() != 4) {
                Object err = Error::create(exec,SyntaxError);
                exec->setException(err);
                return err;
            }
            float x = (float)args[0].toNumber(exec);
            float y = (float)args[1].toNumber(exec);
            float w = (float)args[2].toNumber(exec);
            float h = (float)args[3].toNumber(exec);
            CGContextBeginPath(drawingContext);
            CGContextMoveToPoint (drawingContext, x, y);
            CGContextAddLineToPoint (drawingContext,x+w, y);
            CGContextAddLineToPoint (drawingContext,x+w, y+h);
            CGContextAddLineToPoint (drawingContext,x, y+h);
            CGContextClosePath(drawingContext);

            if (isGradient(contextObject->_fillStyle)) {
                CGContextSaveGState(drawingContext);
                
                // Set the clip from the current path because shading only
                // operates on clippin regions!  Odd, but true.
                CGContextClip(drawingContext);

                ObjectImp *o =
static_cast<ObjectImp*>(contextObject->_fillStyle.imp());
                Gradient *gradient = static_cast<Gradient*>(o);
                CGShadingRef shading = gradient->getShading();
                CGContextDrawShading(drawingContext, shading);
                
                CGContextRestoreGState(drawingContext);
            }
            else
                CGContextFillPath (drawingContext);

            renderer->setNeedsImageUpdate();
            break;
        }

Path creation should be safe as the existing call to CGContextFillRect() clears
the current path.
Comment 1 David Carson 2005-12-15 16:05:29 PST
Although, with the change specified below applied, it does not produce the right
gradient. It seems that when the color stops are sorted using the qsort
algorithm, the order of the colorstops become:
  lingrad.addColorStop(0, '#00ABEB');
  lingrad.addColorStop(0.5, '#66CC00');
  lingrad.addColorStop(0.5, '#fff');
  lingrad.addColorStop(1, '#fff'); 
(This might be because the way qsort works when the comparison values are
equal.) This results in a gradient from blue to green for the first 50% of the
rectangle and a white area (ie white to white gradient) for the second 50% of
the rectangle.
Comment 2 David Carson 2006-03-05 10:01:59 PST
Created attachment 6871 [details]
Patch fillrect to support gradients

As CGContextFillRect function does not support filling with a defined gradient, this patch creates a path for the rectangle when a gradient is specified for the fill type.
Comment 3 Darin Adler 2006-03-05 12:07:29 PST
Comment on attachment 6871 [details]
Patch fillrect to support gradients

Looks great!

r=me.

Marking review- because this needs a change log entry and a test case (presumably differing only in the pixel test).

It will be nice when we change canvas to use GraphicsContext, so we can move all this code out of kjs_html.cpp!
Comment 4 David Carson 2006-03-05 17:51:03 PST
Created attachment 6879 [details]
patch including test and changelogs

Should also move the canvas test cases:
LayoutTests/fast/dom/image-object-in-canvas*
LayoutTests/fast/dom/quadraticCurveTo*
to
LayoutTests/fast/canvas/
Comment 5 David Carson 2006-03-05 20:06:13 PST
Created attachment 6887 [details]
patch

Updated test case to describe what the canvas image should look like, as requested by maciej
Comment 6 Darin Adler 2006-03-09 08:26:40 PST
Comment on attachment 6887 [details]
patch

I found a bug that I'll fix while landing.

CGContextClip needs to be called after setting up the path. It was being called before, so we were actually filling the entire canvas, not just the rectangle! The test case shows the problem, because the filled rectangle is too big.
Comment 7 Darin Adler 2006-04-25 09:32:48 PDT
<rdar://problem/4139672> <canvas> element fillRect ignores gradients (6103)