Bug 22580 – canvas.setShadow should use platform-independent GraphicsContext API

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Tony Chang]

Currently there's only a CG path.  Switching to the platform independent GraphicsContext API should allow canvas shadows to work with cairo or skia.

[Tony Chang]

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]

Created attachment 25646 [details]
take 2

Oops, remove an unnecessary call to applyShadow.

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]

(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]

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.

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]

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 on attachment 25698 [details]
[1/1] Convert CanvasRenderingContext2D::setShadow to use

Ok, no longer apply transforms to canvas shadows.

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]

(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.

Hyatt, what do you think?

[Dave Hyatt]

I agree with Mitz.

[Tony Chang]

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 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)]

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