using drawImage with SVG source resets the context's current transformation

RESOLVED FIXED in Firefox 34

Status

()

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

People

(Reporter: Simon Sarris, Assigned: jwatt)

Tracking

({regression})

34 Branch
mozilla35
x86_64
Windows 8.1
regression
Points:
---

Firefox Tracking Flags

(firefox33 unaffected, firefox34+ fixed, firefox35+ fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

4 years ago
User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/39.0.2159.4 Safari/537.36

Steps to reproduce:

Scale the canvas, draw an SVG image, then inspect the context's transformation matrix. It gets reset! That's way bad.

Example:

ctx.scale(5,5);
ctx.fillStyle = 'rgba(0,0,0,.5)'

var img = new Image();
img.onload = function() {
  // comment this out to see what happens in Firefox Nightly:
  console.log(ctx.mozCurrentTransform) // 5,0,0,5,0,0
  ctx.drawImage(img, 0, 0, 29, 20, 0, 0, 29, 20);
  console.log(ctx.mozCurrentTransform) // 1,0,0,1,0,0 <- THATS BAD.
  
  ctx.fillRect(10,10,10,10);
}
img.src = "http://gojs.net/temp/draft-vm.svg";


Live example: http://codepen.io/simonsarris/pen/mKqye


Actual results:

The context's transformation matrix gets reset.


Expected results:

ctx.drawImage() should not have side effects like this.
(Reporter)

Updated

4 years ago
Version: 32 Branch → 35 Branch
(Reporter)

Updated

4 years ago
Component: Untriaged → Canvas: 2D
Product: Firefox → Core

Comment 1

4 years ago
[Tracking Requested - why for this release]: Regression

Regression window(m-i)
Good:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3488976ecf0f
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:34.0) Gecko/20100101 Firefox/34.0 ID:20140822131153
Bad:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3d51132a0099
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:34.0) Gecko/20100101 Firefox/34.0 ID:20140822131352
Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=3488976ecf0f&tochange=3d51132a0099
Regressed by:
3d51132a0099	Seth Fowler — Bug 1043560 - Refactor the imgIContainer::Draw API. r=tn,dholbert,jwatt,mwu,mattwoodrow,roc sr=jrmuizel
Blocks: 1043560
Status: UNCONFIRMED → NEW
status-firefox33: --- → unaffected
status-firefox34: --- → affected
status-firefox35: --- → affected
tracking-firefox34: --- → ?
tracking-firefox35: --- → ?
Ever confirmed: true
Keywords: regression
Version: 35 Branch → 34 Branch
Created attachment 8495187 [details] [diff] [review]
patch
Assignee: nobody → jwatt
Attachment #8495187 - Flags: review?(seth)
Created attachment 8495200 [details] [diff] [review]
patch
Attachment #8495187 - Attachment is obsolete: true
Attachment #8495187 - Flags: review?(seth)
Attachment #8495200 - Flags: review?(seth)
Created attachment 8496417 [details] [diff] [review]
patch

Actually, I had to move the imgFrame.cpp changes further up the stack. It seems that we need the transform to still be changed when we call gfxUtils::DrawPixelSnapped. The following tests fail if it isn't:

image/test/reftest/gif/small-background-size.gif
image/test/reftest/gif/small-background-size-2.gif
layout/reftests/bugs/456330-1.gif
layout/reftests/bugs/488685-1.html

We should redesign this code as per the comment I left in the code, but I don't have time to think about that right now.
Attachment #8495200 - Attachment is obsolete: true
Attachment #8495200 - Flags: review?(seth)
Attachment #8496417 - Flags: review?(seth)
tracking-firefox34: ? → +
tracking-firefox35: ? → +
Comment on attachment 8496417 [details] [diff] [review]
patch

Review of attachment 8496417 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good!

I thought I'd summarize our discussion on IRC about this so it's recorded somewhere: The fact that changing the matrix of a temporary gfxContext wrapping an underlying DrawTarget causes a permanent change in the underlying DrawTarget's matrix is profoundly unintuitive to me. You mentioned that we aren't supposed to wrap an existing DrawTarget in a temporary gfxContext. We concluded that it would be very useful if we could figure out how to assert about that, since it's not obvious from the API.
Attachment #8496417 - Flags: review?(seth) → review+
(Reporter)

Comment 6

4 years ago
You guys are the best.
Comment on attachment 8496417 [details] [diff] [review]
patch

Approval Request Comment
[Feature/regressing bug #]: bug 1043560
[User impact if declined]: HTML <canvas> transform broken if image draw to it
[Describe test coverage new/current, TBPL]: passed full Try run, plus new reftest, and now on m-i
[Risks and why]: should be low risk, since it only saves and restores a transform
[String/UUID change made/needed]: none
Attachment #8496417 - Flags: approval-mozilla-beta?
Attachment #8496417 - Flags: approval-mozilla-aurora?
Jonathan, 33 is marked as unaffected and you request an uplift to beta. Which one is right?
Flags: needinfo?(jwatt)
Comment on attachment 8496417 [details] [diff] [review]
patch

Sorry, getting my version numbers confused.
Attachment #8496417 - Flags: approval-mozilla-beta?
Flags: needinfo?(jwatt)
https://hg.mozilla.org/mozilla-central/rev/abf96b7fa963
Status: NEW → RESOLVED
Last Resolved: 4 years ago
status-firefox35: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Comment on attachment 8496417 [details] [diff] [review]
patch

Aurora+
Attachment #8496417 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.