433397 - Canvas putImageData is affected by clipping

Status: RESOLVED FIXED

(Core :: Graphics: Canvas2D, defect)

Description

[Philip Taylor]

HTML5 says "The [...] clipping region [...] must not affect the getImageData() and putImageData() methods", but FF3 only draws pixels inside the clipping region.

The test case passes in FF2.0.0.14 on Linux. It fails in almost-current nightly on Windows (2008051011) and Linux (2008051104).

Comment 1

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Blah.  Easy fix (Save(), clear clip, draw, Restore()), no idea when/where this regressed.

Comment 2

[Saint Wesonga]

Attachment: nsCanvasRenderingContext2D.cpp with missing call to ResetClip()

Here's the diff:

diff -r 0bc50734e20e content/canvas/src/nsCanvasRenderingContext2D.cpp
--- a/content/canvas/src/nsCanvasRenderingContext2D.cpp	Fri May 22 15:57:16 2009 -0400
+++ b/content/canvas/src/nsCanvasRenderingContext2D.cpp	Fri May 22 23:33:18 2009 -0400
@@ -3715,6 +3715,9 @@
     gfxContextPathAutoSaveRestore pathSR(mThebes);
     gfxContextAutoSaveRestore autoSR(mThebes);
 
+    // ignore clipping region, as per spec
+    mThebes->ResetClip();
+
     mThebes->IdentityMatrix();
     mThebes->Translate(gfxPoint(x, y));
     mThebes->NewPath();

Comment 3

[Saint Wesonga]

Attachment: Updated putImageData/clipping region mochitest

The corresponding test needs to switch from "todo_isPixel" to "isPixel" as well

diff -r 0bc50734e20e content/canvas/test/test_2d.imageData.put.clip.html
--- a/content/canvas/test/test_2d.imageData.put.clip.html	Fri May 22 15:57:16 2009 -0400
+++ b/content/canvas/test/test_2d.imageData.put.clip.html	Fri May 22 23:40:06 2009 -0400
@@ -48,7 +48,7 @@
 ctx.clip();
 ctx.putImageData(imgdata, 0, 0);
 isPixel(ctx, 25,25, 0,255,0,255, "25,25", "0,255,0,255", 2);
-todo_isPixel(ctx, 75,25, 0,255,0,255, "75,25", "0,255,0,255", 2);
+isPixel(ctx, 75,25, 0,255,0,255, "75,25", "0,255,0,255", 2);
 
 SimpleTest.finish();

Comment 4

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Comment on attachment 379305 [details] [diff] [review]
nsCanvasRenderingContext2D.cpp with missing call to ResetClip()

Not much to add for superreview for a one-liner (assuming the diff in the comment is what needs review here)

However, in the future, please attach just the diff; attaching the whole file isn't particularly useful since it can mean different things against different versions (i.e., overwrite somebody else's changes).

Comment 5

[Saint Wesonga]

Attachment: Correct patch for missing call to ResetClip()

Attached correct patch with the diff information to the bug

Comment 6

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Attachment: patch with code and test change

Here's a single patch with the code change and the test change (which obviously need to be committed together, so they should be in one patch).  It also has a commit message (with reviewers and bug number) and the author.

(This diff is actually a patch in a mercurial patch queue.)

I've sent this to the try server ( https://wiki.mozilla.org/Build:TryServer ) to make sure tests pass (as changeset 0cda626dabd1).

Comment 7

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

It passed try server build and unit test (modulo some usual random orange).

Comment 8

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Landed on mozilla-central:
http://hg.mozilla.org/mozilla-central/rev/7c62ba676b2c