Calling canvas restore() resets the strokeStyle property

RESOLVED FIXED

Status

()

Core
Canvas: 2D
RESOLVED FIXED
12 years ago
12 years ago

People

(Reporter: Kamiel Martinet, Assigned: vlad)

Tracking

({fixed1.8})

1.8 Branch
x86
Windows XP
fixed1.8
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Reporter)

Description

12 years ago
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
(Reporter)

Updated

12 years ago
Version: Trunk → 1.8 Branch
(Reporter)

Comment 1

12 years ago
Created attachment 200434 [details]
Test case
(Assignee)

Comment 2

12 years ago
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.
Assignee: nobody → vladimir
Status: UNCONFIRMED → ASSIGNED
Attachment #200655 - Flags: review?(pavlov)

Comment 3

12 years ago
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.
Attachment #200655 - Flags: review?(pavlov) → review+
(Assignee)

Comment 4

12 years ago
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.)
Attachment #200655 - Flags: approval1.8rc1?

Updated

12 years ago
Attachment #200655 - Flags: superreview?(brendan)

Comment 5

12 years ago
not going to lose a day with this but could become a ride-along if we're forced to respin by some other bug.
Flags: blocking1.8rc1?

Comment 6

12 years ago
Vlad, can we get this patch onto the trunk for baking in case we decide to take this later on the branch?
(Assignee)

Comment 7

12 years ago
Whoops, yes; I'll check it in tonight.
(Assignee)

Updated

12 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED

Comment 8

12 years ago
moving out to the 1.8 rc2 ride-along candidate list. We'll consider taking this if we do an RC2.
Flags: blocking1.8rc1? → blocking1.8rc2?

Updated

12 years ago
Attachment #200655 - Flags: approval1.8rc1? → approval1.8rc2?
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
Attachment #200655 - Flags: superreview?(brendan) → superreview+

Comment 10

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

Identified prior to rc1 as a candiate for rc2.
Attachment #200655 - Flags: approval1.8rc2? → approval1.8rc2+

Updated

12 years ago
Flags: blocking1.8rc2?
(Assignee)

Updated

12 years ago
Keywords: fixed1.8
You need to log in before you can comment on or make changes to this bug.