Element with -webkit-mask: -webkit-canvas() is not redrawn when the canvas updates

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Simon Fraser (smfr)]

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.

[Simon Fraser (smfr)]

Created attachment 22166 [details]
Testcase

[Simon Fraser (smfr)]

<rdar://problem/6061428>

[Simon Fraser (smfr)]

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.

[Dave Hyatt]

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.

[Simon Fraser (smfr)]

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.

[Dave Hyatt]

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.

[Simon Fraser (smfr)]

But I don't have a StyleImage to pass into containsImage(), just a WrappedImagePtr.

[Simon Fraser (smfr)]

Created attachment 22193 [details]
Final patch

[Dave Hyatt]

Comment on attachment 22193 [details]
Final patch

r=me

[Simon Fraser (smfr)]

Created attachment 22199 [details]
Patch with testcase and changelog.

[Simon Fraser (smfr)]

Comment on attachment 22199 [details]
Patch with testcase and changelog.

Transferring r= from hyatt

[Dean Jackson]

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

[Simon Fraser (smfr)]

Committed by dino in r35101