Bug 19954 - Element with -webkit-mask: -webkit-canvas() is not redrawn when the canvas updates
Summary: Element with -webkit-mask: -webkit-canvas() is not redrawn when the canvas up...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh OS X 10.5
: P2 Normal
Assignee: Simon Fraser (smfr)
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2008-07-08 16:33 PDT by Simon Fraser (smfr)
Modified: 2008-07-11 10:58 PDT (History)
2 users (show)

See Also:


Attachments
Testcase (2.12 KB, text/html)
2008-07-08 16:33 PDT, Simon Fraser (smfr)
no flags Details
Patch to repaint() when the mask image changes (2.05 KB, patch)
2008-07-08 16:36 PDT, Simon Fraser (smfr)
hyatt: review-
Details | Formatted Diff | Diff
Patch addressing review comments (4.70 KB, patch)
2008-07-09 15:40 PDT, Simon Fraser (smfr)
hyatt: review-
Details | Formatted Diff | Diff
Final patch (6.70 KB, patch)
2008-07-09 17:27 PDT, Simon Fraser (smfr)
hyatt: review+
Details | Formatted Diff | Diff
Patch with testcase and changelog. (10.34 KB, patch)
2008-07-10 10:48 PDT, Simon Fraser (smfr)
simon.fraser: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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