Bug 22580

Summary: canvas.setShadow should use platform-independent GraphicsContext API
Product: WebKit Reporter: Tony Chang <tony>
Component: PlatformAssignee: Tony Chang <tony>
Status: RESOLVED FIXED    
Severity: Normal CC: hyatt, mitz
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Windows XP   
Attachments:
Description Flags
take 1
none
take 2
mitz: review-
test for transform and shadow
none
[1/1] Convert CanvasRenderingContext2D::setShadow to use
mitz: review-
[1/1] Add a bool to GraphicsContext so that shadows can ignore eric: review+

Tony Chang
Reported 2008-12-01 17:03:33 PST
Currently there's only a CG path. Switching to the platform independent GraphicsContext API should allow canvas shadows to work with cairo or skia.
Attachments
take 1 (6.37 KB, patch)
2008-12-01 17:04 PST, Tony Chang
no flags
take 2 (6.22 KB, patch)
2008-12-01 17:06 PST, Tony Chang
mitz: review-
test for transform and shadow (223 bytes, text/html)
2008-12-02 15:13 PST, Tony Chang
no flags
[1/1] Convert CanvasRenderingContext2D::setShadow to use (12.58 KB, patch)
2008-12-02 18:22 PST, Tony Chang
mitz: review-
[1/1] Add a bool to GraphicsContext so that shadows can ignore (11.16 KB, patch)
2008-12-05 12:19 PST, Tony Chang
eric: review+
Tony Chang
Comment 1 2008-12-01 17:04:47 PST
Created attachment 25645 [details] take 1 Is there a way for me to convert CMYK to RGB to convert the last method as well?
Tony Chang
Comment 2 2008-12-01 17:06:59 PST
Created attachment 25646 [details] take 2 Oops, remove an unnecessary call to applyShadow.
mitz
Comment 3 2008-12-01 19:57:55 PST
Comment on attachment 25646 [details] take 2 CGContextSetShadowWithColor takes an offset and radius in device space, but GraphicsContext::setShadow()'s parameters are in user space. I think this patch will lead to the wrong behavior in transformed contexts.
Tony Chang
Comment 4 2008-12-02 15:11:33 PST
(In reply to comment #3) > (From update of attachment 25646 [details] [review]) > CGContextSetShadowWithColor takes an offset and radius in device space, but > GraphicsContext::setShadow()'s parameters are in user space. I think this patch > will lead to the wrong behavior in transformed contexts. You're right, this patch causes transformations to be applied to the shadow as well. It's unclear to me what the expected behavior is. On Safari 3.2, I see the transformation not being applied to the shadow. In Safari nightlies, I see the transformation being applied. I'll upload a test case.
Tony Chang
Comment 5 2008-12-02 15:13:29 PST
Created attachment 25686 [details] test for transform and shadow On some versions of safari, the red box is to the right, in others, it is below and to the right.
mitz
Comment 6 2008-12-02 16:14:14 PST
TOT WebKit was changed to make CSS text-shadow and box-shadow respect transforms. However, my reading of the canvas specification is that when drawing in a canvas, the canvas's CTM should not affect shadow drawing: "The shadowOffsetX and shadowOffsetY attributes specify the distance that the shadow will be offset in the positive horizontal and positive vertical distance respectively. Their values are in coordinate space units. They are not affected by the current transformation matrix" <http://www.whatwg.org/specs/web-apps/current-work/multipage/the-canvas-element.html#shadows>.
Tony Chang
Comment 7 2008-12-02 18:22:33 PST
Created attachment 25698 [details] [1/1] Convert CanvasRenderingContext2D::setShadow to use platform-independent GraphicsContext API. --- LayoutTests/fast/css/shadow-scale.html | 8 ++ .../mac/fast/css/shadow-scale-expected.checksum | 1 + .../mac/fast/css/shadow-scale-expected.png | Bin 0 -> 14750 bytes .../mac/fast/css/shadow-scale-expected.txt | 7 ++ WebCore/html/CanvasRenderingContext2D.cpp | 76 +++++--------------- WebCore/platform/graphics/GraphicsContext.cpp | 4 +- WebCore/platform/graphics/GraphicsContext.h | 4 +- .../graphics/cairo/GraphicsContextCairo.cpp | 2 +- WebCore/platform/graphics/cg/GraphicsContextCG.cpp | 31 +++++--- WebCore/platform/graphics/qt/GraphicsContextQt.cpp | 2 +- 10 files changed, 60 insertions(+), 75 deletions(-)
Tony Chang
Comment 8 2008-12-02 18:24:00 PST
Comment on attachment 25698 [details] [1/1] Convert CanvasRenderingContext2D::setShadow to use Ok, no longer apply transforms to canvas shadows.
mitz
Comment 9 2008-12-02 21:23:24 PST
Comment on attachment 25698 [details] [1/1] Convert CanvasRenderingContext2D::setShadow to use The patch is missing a change log entry. I think the test is redundant, because fast/transforms/shadows.html covers that case as well as several other. Given the expected usage pattern, do you think it will make more sense to make the shadow behavior part of the graphics state, so that canvas can just set it once when it creates the context?
Tony Chang
Comment 10 2008-12-03 11:51:53 PST
(In reply to comment #9) > (From update of attachment 25698 [details] [review]) > The patch is missing a change log entry. I think the test is redundant, because > fast/transforms/shadows.html covers that case as well as several other. Given > the expected usage pattern, do you think it will make more sense to make the > shadow behavior part of the graphics state, so that canvas can just set it once > when it creates the context? It's possible, but a bit tricky. The GraphicsContext is owned by an ImageBuffer which is owned by the HTML canvas element. I guess this would involve adding a default bool param to ImageBuffer::create for the transform case. If you think this is the right approach, let me know and I'll make the change. Otherwise, I'll make a new patch without the layout test and with a ChangeLog entry.
mitz
Comment 11 2008-12-03 12:38:31 PST
Hyatt, what do you think?
Dave Hyatt
Comment 12 2008-12-04 17:34:59 PST
I agree with Mitz.
Tony Chang
Comment 13 2008-12-05 12:19:29 PST
Created attachment 25782 [details] [1/1] Add a bool to GraphicsContext so that shadows can ignore transformations. This is needed by HTML canvas element where the spec says shadows are applied w/o transformations. --- WebCore/ChangeLog | 23 ++++++ WebCore/html/CanvasRenderingContext2D.cpp | 76 +++++--------------- WebCore/html/HTMLCanvasElement.cpp | 2 + WebCore/platform/graphics/GraphicsContext.cpp | 5 ++ WebCore/platform/graphics/GraphicsContext.h | 1 + WebCore/platform/graphics/GraphicsContextPrivate.h | 3 + WebCore/platform/graphics/cg/GraphicsContextCG.cpp | 29 +++++--- 7 files changed, 71 insertions(+), 68 deletions(-)
Eric Seidel (no email)
Comment 14 2008-12-08 12:52:49 PST
Comment on attachment 25782 [details] [1/1] Add a bool to GraphicsContext so that shadows can ignore Looks good. I really need to add a float-enabled Color class to platform, since with this patch, we only support 256 grays for shadows. Which is fine... but still lame. r+
Eric Seidel (no email)
Comment 15 2008-12-08 15:43:37 PST
Committing to http://svn.webkit.org/repository/webkit/trunk ... M WebCore/ChangeLog M WebCore/html/CanvasRenderingContext2D.cpp M WebCore/html/HTMLCanvasElement.cpp M WebCore/platform/graphics/GraphicsContext.cpp M WebCore/platform/graphics/GraphicsContext.h M WebCore/platform/graphics/GraphicsContextPrivate.h M WebCore/platform/graphics/cg/GraphicsContextCG.cpp Committed r39105
Note You need to log in before you can comment on or make changes to this bug.