Last Comment Bug 313372 - Calling canvas restore() resets the strokeStyle property
: Calling canvas restore() resets the strokeStyle property
: fixed1.8
Product: Core
Classification: Components
Component: Canvas: 2D (show other bugs)
: 1.8 Branch
: x86 Windows XP
: -- normal (vote)
: ---
Assigned To: Vladimir Vukicevic [:vlad] [:vladv]
: Milan Sreckovic [:milan]
Depends on:
  Show dependency treegraph
Reported: 2005-10-22 05:10 PDT by Kamiel Martinet
Modified: 2005-11-02 12:27 PST (History)
3 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Test case (1.88 KB, text/html)
2005-10-22 05:12 PDT, Kamiel Martinet
no flags Details
canvas-save-restore-fix.patch (10.74 KB, patch)
2005-10-24 14:08 PDT, Vladimir Vukicevic [:vlad] [:vladv]
pavlov: review+
brendan: superreview+
mtschrep: approval1.8rc2+
Details | Diff | Splinter Review

Description Kamiel Martinet 2005-10-22 05:10:55 PDT
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

Actual Results:  
one red and one black shape

Expected Results:  
two red shapes
Comment 1 Kamiel Martinet 2005-10-22 05:12:06 PDT
Created attachment 200434 [details]
Test case
Comment 2 Vladimir Vukicevic [:vlad] [:vladv] 2005-10-24 14:08:25 PDT
Created attachment 200655 [details] [diff] [review]

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 2005-10-24 14:13:43 PDT
Comment on attachment 200655 [details] [diff] [review]

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] 2005-10-24 14:55:03 PDT
Comment on attachment 200655 [details] [diff] [review]

?'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] 2005-10-26 11:35:16 PDT
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 2005-10-28 16:13:08 PDT
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] 2005-10-28 16:38:09 PDT
Whoops, yes; I'll check it in tonight.
Comment 8 Asa Dotzler [:asa] 2005-10-31 14:38:33 PST
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] 2005-11-01 23:58:29 PST
Comment on attachment 200655 [details] [diff] [review]

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.

Comment 10 Mike Schroepfer 2005-11-02 11:13:50 PST
Comment on attachment 200655 [details] [diff] [review]

Identified prior to rc1 as a candiate for rc2.

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