Bug 22580 - canvas.setShadow should use platform-independent GraphicsContext API
Summary: canvas.setShadow should use platform-independent GraphicsContext API
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows XP
: P2 Normal
Assignee: Tony Chang
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-12-01 17:03 PST by Tony Chang
Modified: 2008-12-08 15:43 PST (History)
2 users (show)

See Also:


Attachments
take 1 (6.37 KB, patch)
2008-12-01 17:04 PST, Tony Chang
no flags Details | Formatted Diff | Diff
take 2 (6.22 KB, patch)
2008-12-01 17:06 PST, Tony Chang
mitz: review-
Details | Formatted Diff | Diff
test for transform and shadow (223 bytes, text/html)
2008-12-02 15:13 PST, Tony Chang
no flags Details
[1/1] Convert CanvasRenderingContext2D::setShadow to use (12.58 KB, patch)
2008-12-02 18:22 PST, Tony Chang
mitz: review-
Details | Formatted Diff | Diff
[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+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tony Chang 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.
Comment 1 Tony Chang 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?
Comment 2 Tony Chang 2008-12-01 17:06:59 PST
Created attachment 25646 [details]
take 2

Oops, remove an unnecessary call to applyShadow.
Comment 3 mitz 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.
Comment 4 Tony Chang 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.
Comment 5 Tony Chang 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.
Comment 6 mitz 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>.
Comment 7 Tony Chang 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(-)
Comment 8 Tony Chang 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.
Comment 9 mitz 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?
Comment 10 Tony Chang 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.
Comment 11 mitz 2008-12-03 12:38:31 PST
Hyatt, what do you think?
Comment 12 Dave Hyatt 2008-12-04 17:34:59 PST
I agree with Mitz.

Comment 13 Tony Chang 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(-)
Comment 14 Eric Seidel (no email) 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+
Comment 15 Eric Seidel (no email) 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