296904 - canvas shapes incorrectly affect current path

Status: RESOLVED FIXED

(Core :: Graphics: Canvas2D, defect)

Description

[ArronM (:paper)]

From the specs (as written currently):

"Shapes are painted without affecting the current path"

However, clearRect, fillRect, and strokeRect all destroy the current path in the
current implementation.  

Safari canvas specs say clearRect is destroys the current path, while fillRect
and strokeRect have no such feature listed.  At the very least, I suggest making
all three functions behave the same, either by altering Gecko code to the spec,
or the spec to the Gecko code.

Comment 1

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

Fixed either by 333177 or before; we use stroke_preserve and friends now.

Comment 2

[Hixie (not reading bugmail)]

http://www.hixie.ch/tests/adhoc/html/canvas/016.html

Comment 3

[Philip Taylor]

Attachment: test case

More comprehensive test case - fillRect, strokeRect, clearRect, drawImage, putImageData.

Comment 4

[Philip Taylor]

Attachment: patch

This fixes compatibility with the current spec, and with Opera and Safari 3.

It also breaks compatibility with some sites that depend on FF2's behaviour - see http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2007-September/012561.html

Comment 5

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

Comment on attachment 282173 [details] [diff] [review]
patch

looks good to me; I wish we hadn't put in fillRect()/drawRect() in the first place, but I guess we would've had the same problems with images anyway.

Comment 6

[Philip Taylor]

Attachment: patch with reftest

Same code, but converted the test case into a reftest. (I'm hoping it's alright to carry through the r+.)

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Would be great to get all browsers do the same before too many sites start depending on the old behavior. And since there is a patch getting it fixed shouldn't be too much work.

Comment 8

[Brian Crowder]

blocking1.9 already, doesn't need approval to land, right?

Comment 9

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

Comment on attachment 282192 [details] [diff] [review]
patch with reftest

Correct; Philip, do you have cvs commit privs?  Or do you need me to land this?

Comment 10

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

Checked in, thanks!

Comment 11

[Jeff Walden [:Waldo]]

Thanks for writing a reftest and including it in the patch!  People who include automated tests in their patches are my heroes.