313372 - Calling canvas restore() resets the strokeStyle property

Status: RESOLVED FIXED

(Core :: Graphics: Canvas2D, defect)

Description

[Kamiel Martinet]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b5) Gecko/20051021 Firefox/1.5
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b5) Gecko/20051021 Firefox/1.5

When calling a canvas restore method to retrieve a previous saved state from the
stack doesn't return the saved strokeStyle property but instead resets it to the
original black color.

Reproducible: Always

Steps to Reproduce:
1. see the test case
2.
3.

Actual Results:  
one red and one black shape

Expected Results:  
two red shapes

Comment 1

[Kamiel Martinet]

Attachment: Test case

Comment 2

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

Attachment: canvas-save-restore-fix.patch

Fix for save/restore not saving styles.  This was a pretty bad ommission that I'm not sure how anyone didn't notice yet :)  This patch saves the current styles up to a maximum depth (set to 50).  Most of the code change is just changing style access to point to the right entry in the stack.

Comment 3

[Stuart Parmenter]

Comment on attachment 200655 [details] [diff] [review]
canvas-save-restore-fix.patch

I'm a little worried that 50 is too small, but this is certainly better than what we have now.

Might want to think about a bigger patch to lazily allocate the stack of these so we can go a lot deeper.

Comment 4

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

Comment on attachment 200655 [details] [diff] [review]
canvas-save-restore-fix.patch

?'ing for RC approval to get it on the radar after RC1 goes out (I know that the code is locked down for RC1 at the moment).  This is a low risk patch that fixes a pretty glaring bug in canvas that someone just came across on the weekend.

It should be very low risk; it only touches the internals of canvas state maintenance.  Without this patch, people are likely to run into this bug fairly often as they start working with canvas.  (Or, worse yet, they'll just have "odd" behaviour that they can't explain.  The same thing will happen if they ever nest save/restore 50 levels deep with this patch, but I don't see that depth being too limiting.  We can do a real fix for a future version with nsCOMArray and friends.)

Comment 5

[Asa Dotzler [:asa]]

not going to lose a day with this but could become a ride-along if we're forced to respin by some other bug.

Comment 6

[Scott MacGregor]

Vlad, can we get this patch onto the trunk for baking in case we decide to take this later on the branch?

Comment 7

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

Whoops, yes; I'll check it in tonight.

Comment 8

[Asa Dotzler [:asa]]

moving out to the 1.8 rc2 ride-along candidate list. We'll consider taking this if we do an RC2.

Comment 9

[Brendan Eich [:brendan]]

Comment on attachment 200655 [details] [diff] [review]
canvas-save-restore-fix.patch

Only thought is to assert mSaveCount doesn't underflow.  If you control the calls to Restore, it should be possible to do without the mSaveCount > 0 tests.  At the least, you could fuse them into one test (outer if-then).  sr=me in any case.

/be

Comment 10

[Mike Schroepfer]

Comment on attachment 200655 [details] [diff] [review]
canvas-save-restore-fix.patch

Identified prior to rc1 as a candiate for rc2.