Bug 22580 - canvas.setShadow should use platform-independent GraphicsContext API
: canvas.setShadow should use platform-independent GraphicsContext API
Status: RESOLVED FIXED
: WebKit
Platform
: 528+ (Nightly build)
: PC Windows XP
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2008-12-01 17:03 PST by
Modified: 2008-12-08 15:43 PST (History)


Attachments
take 1 (6.37 KB, patch)
2008-12-01 17:04 PST, Tony Chang
no flags Review Patch | Details | Formatted Diff | Diff
take 2 (6.22 KB, patch)
2008-12-01 17:06 PST, Tony Chang
mitz: review-
Review Patch | 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-
Review Patch | 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+
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 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 From 2008-12-01 17:04:47 PST -------
Created an attachment (id=25645) [details]
take 1

Is there a way for me to convert CMYK to RGB to convert the last method as well?
------- Comment #2 From 2008-12-01 17:06:59 PST -------
Created an attachment (id=25646) [details]
take 2

Oops, remove an unnecessary call to applyShadow.
------- Comment #3 From 2008-12-01 19:57:55 PST -------
(From update of attachment 25646 [details])
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 From 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 From 2008-12-02 15:13:29 PST -------
Created an attachment (id=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 From 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 From 2008-12-02 18:22:33 PST -------
Created an attachment (id=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 From 2008-12-02 18:24:00 PST -------
(From update of attachment 25698 [details])
Ok, no longer apply transforms to canvas shadows.
------- Comment #9 From 2008-12-02 21:23:24 PST -------
(From update of attachment 25698 [details])
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 From 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 From 2008-12-03 12:38:31 PST -------
Hyatt, what do you think?
------- Comment #12 From 2008-12-04 17:34:59 PST -------
I agree with Mitz.
------- Comment #13 From 2008-12-05 12:19:29 PST -------
Created an attachment (id=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 From 2008-12-08 12:52:49 PST -------
(From update of attachment 25782 [details])
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 From 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