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
Created attachment 200655 [details] [diff] [review] 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 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 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.)
not going to lose a day with this but could become a ride-along if we're forced to respin by some other bug.
Vlad, can we get this patch onto the trunk for baking in case we decide to take this later on the branch?
Whoops, yes; I'll check it in tonight.
moving out to the 1.8 rc2 ride-along candidate list. We'll consider taking this if we do an RC2.
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 on attachment 200655 [details] [diff] [review] canvas-save-restore-fix.patch Identified prior to rc1 as a candiate for rc2.