Bug 19954

Summary: Element with -webkit-mask: -webkit-canvas() is not redrawn when the canvas updates
Product: WebKit Reporter: Simon Fraser (smfr) <simon.fraser>
Component: Layout and RenderingAssignee: Simon Fraser (smfr) <simon.fraser>
Status: RESOLVED FIXED    
Severity: Normal CC: dino, hyatt
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Macintosh   
OS: OS X 10.5   
Attachments:
Description Flags
Testcase
none
Patch to repaint() when the mask image changes
hyatt: review-
Patch addressing review comments
hyatt: review-
Final patch
hyatt: review+
Patch with testcase and changelog. simon.fraser: review+

Description Simon Fraser (smfr) 2008-07-08 16:33:00 PDT
The attached testcase shows a problem where, when the mask, which is a canvas, changes, the element is not redrawn.

The testcase has an image with style:
  -webkit-mask: -webkit-canvas(canvas1);
and an onmousemove() handler that redraws the canvas using the mouse coordinate to determine the size of the gradient.

What I see:
When the page loads, the image is drawn with the gradient mask, but never changes thereafter.

What I expect:
The image + mask is redrawn whenever the canvas is udpated.
Comment 1 Simon Fraser (smfr) 2008-07-08 16:33:24 PDT
Created attachment 22166 [details]
Testcase
Comment 2 Simon Fraser (smfr) 2008-07-08 16:35:20 PDT
<rdar://problem/6061428>
Comment 3 Simon Fraser (smfr) 2008-07-08 16:36:47 PDT
Created attachment 22167 [details]
Patch to repaint() when the mask image changes

First cut at a patch. Not sure how to do a non-manual testcase for this.
Comment 4 Dave Hyatt 2008-07-09 13:12:04 PDT
Comment on attachment 22167 [details]
Patch to repaint() when the mask image changes

(1) Adding maskBoxImage to RenderBox's first check is good.

(2) Adding maskImage is bad.  We want precise invalidation like we have  with backgrounds.  See the layer walk at the end of the function.  You should make a little static helper function that walks layers to do the invalidation.  Then you can call it on the background layers and on the mask layers.

(2) The patch to RenderImage should just call the base class if you have a mask.  So changing if (hasBoxDecorations()) to if (hasBoxDecorations() || hasMask()) should be sufficient.
Comment 5 Simon Fraser (smfr) 2008-07-09 15:40:24 PDT
Created attachment 22191 [details]
Patch addressing review comments

> (1) Adding maskBoxImage to RenderBox's first check is good.

I didn't add that, I just unwrapped the line.

> (2) Adding maskImage is bad.  We want precise invalidation like we have  with
> backgrounds.  See the layer walk at the end of the function.  You should make a
> little static helper function that walks layers to do the invalidation.  Then
> you can call it on the background layers and on the mask layers.

Done. Note that I had to add FillLayer::containsWrappedImage().

I added a method, repaintLayerRectsForImage(), rather than a static method, because
it needs to get at the view() for invalidation.

> (2) The patch to RenderImage should just call the base class if you have a
> mask.  So changing if (hasBoxDecorations()) to if (hasBoxDecorations() ||
> hasMask()) should be sufficient.

Done.
Comment 6 Dave Hyatt 2008-07-09 15:46:43 PDT
Comment on attachment 22191 [details]
Patch addressing review comments

containsWrappedImage is unnecessary. Doesn't really buy you anything and worst case causes you to do two walks of the layers instead of one.  The existing walk already checks for the image.
Comment 7 Simon Fraser (smfr) 2008-07-09 15:54:03 PDT
But I don't have a StyleImage to pass into containsImage(), just a WrappedImagePtr.
Comment 8 Simon Fraser (smfr) 2008-07-09 17:27:48 PDT
Created attachment 22193 [details]
Final patch
Comment 9 Dave Hyatt 2008-07-09 17:29:44 PDT
Comment on attachment 22193 [details]
Final patch

r=me
Comment 10 Simon Fraser (smfr) 2008-07-10 10:48:16 PDT
Created attachment 22199 [details]
Patch with testcase and changelog.
Comment 11 Simon Fraser (smfr) 2008-07-10 11:07:46 PDT
Comment on attachment 22199 [details]
Patch with testcase and changelog.

Transferring r= from hyatt
Comment 12 Dean Jackson 2008-07-10 12:32:38 PDT
Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebCore/ChangeLog
	A	WebCore/manual-tests/canvas-mask-redraw.html
	M	WebCore/rendering/RenderBox.cpp
	M	WebCore/rendering/RenderBox.h
	M	WebCore/rendering/RenderImage.cpp
	M	WebCore/rendering/RenderObject.cpp
Committed r35101

Comment 13 Simon Fraser (smfr) 2008-07-10 13:48:09 PDT
Committed by dino in r35101