Last Comment Bug 762411 - getScreenCTM scaling broken on inline embedded 'svg' elements
: getScreenCTM scaling broken on inline embedded 'svg' elements
Status: RESOLVED FIXED
: regression
Product: Core
Classification: Components
Component: SVG (show other bugs)
: 13 Branch
: All All
: -- normal (vote)
: mozilla16
Assigned To: Robert Longson
:
Mentors:
: 762895 764738 (view as bug list)
Depends on:
Blocks: 660216
  Show dependency treegraph
 
Reported: 2012-06-07 02:12 PDT by jgs
Modified: 2012-07-03 08:20 PDT (History)
9 users (show)
longsonr: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
affected
fixed
fixed
fixed


Attachments
test-getScreenCTM-4.svg (963 bytes, image/svg+xml)
2012-06-07 02:12 PDT, jgs
no flags Details
patch with test (6.97 KB, patch)
2012-06-09 11:01 PDT, Robert Longson
no flags Details | Diff | Splinter Review
attempt to address review comments (9.59 KB, patch)
2012-06-11 08:36 PDT, Robert Longson
jwatt: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
lukasblakk+bugs: approval‑mozilla‑beta+
Details | Diff | Splinter Review
patch for beta (5.24 KB, patch)
2012-06-22 14:13 PDT, Robert Longson
no flags Details | Diff | Splinter Review

Description jgs 2012-06-07 02:12:57 PDT
Created attachment 630896 [details]
test-getScreenCTM-4.svg

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:13.0) Gecko/20100101 Firefox/13.0
Build ID: 20120601045813

Steps to reproduce:

Bug appeared in Firefox 13.0, was not present in 12.0. Have tested latest nightly build (16.0a1) and it is still present.

Using getScreenCTM() on an <svg> element inline-embedded within XHTML/HTML always returns a matrix with a 1:1 scaling factor (i.e. the "a" and "d" matrix values are always 1).

An example of this is test case 4 in bug 272630, https://bugzilla.mozilla.org/show_bug.cgi?id=272630

I have attached that test case to this bug report.

How to repeat:

1. Open the attached SVG in Firefox.

2. Move your mouse pointer backwards and forwards in the area of the page below the text paragraph.



Actual results:

The black circle does not correctly follow the mouse cursor. 

If your browser window is wide then the circle will move more than the mouse does in the horizontal direction, if it's narrow it will move less.

The vertical direction is also affected, but by a fixed ammount independent of your browser window height.


Expected results:

The centre of the circle should exactly track the mouse pointer.

The matrix returned by getScreenCTM() should contain scaling factors in the "a" and "d" values to correct for any scaling of the embedded SVG due to browser window size.
Comment 1 Robert Longson 2012-06-07 02:16:09 PDT
Would you care to find a regression range?
Comment 3 jgs 2012-06-07 02:27:38 PDT
Will check out the regression window link and get back to you.
Comment 4 jgs 2012-06-07 03:54:23 PDT
Okay, I've narrowed it down to the changes between two nightly builds (mozilla central versions, which I hope are the right ones to be using here):

This version works:
/pub/mozilla.org/firefox/nightly/2012/02/2012-02-17-03-12-27-mozilla-central

This version fails:
/pub/mozilla.org/firefox/nightly/2012/02/2012-02-18-03-11-56-mozilla-central

In both cases I used the firefox-13.0a1.en-US.win32.installer.exe

Please let me know if you need any more information.
Comment 5 Robert Longson 2012-06-07 03:58:43 PDT
Could you type about:buildconfig in the address bar in each of these and post the Built from link please?
Comment 6 jgs 2012-06-07 04:11:28 PDT
The one from 2012-02-17 is:

http://hg.mozilla.org/mozilla-central/rev/2271cb92cc05

The one from 2012-02-18 is:

http://hg.mozilla.org/mozilla-central/rev/550779e6bab4
Comment 7 Robert Longson 2012-06-07 04:15:42 PDT
Which means we're looking for something in here: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=2271cb92cc05&tochange=550779e6bab4
Comment 8 Robert Longson 2012-06-07 04:17:49 PDT
Which means it's almost certainly bug 660216
Comment 9 Robert Longson 2012-06-09 00:54:46 PDT
Reverting part 3 of bug 660216 fixes this.
Comment 10 Robert Longson 2012-06-09 11:01:21 PDT
Created attachment 631679 [details] [diff] [review]
patch with test
Comment 11 Robert Longson 2012-06-09 11:02:00 PDT
Thanks for the regression range jgs it would have been much much harder to fix without that.
Comment 12 Jonathan Watt [:jwatt] (back in October - email directly if necessary) 2012-06-10 15:03:37 PDT
For convenience, part 3 of bug 660216 fixes as checked in is:

http://hg.mozilla.org/mozilla-central/rev/b3a3372fecae
Comment 13 Jonathan Watt [:jwatt] (back in October - email directly if necessary) 2012-06-10 15:48:11 PDT
Comment on attachment 631679 [details] [diff] [review]
patch with test

Ah, I see. The "part 3" patch did have logic intended to prevent this problem - the IsRoot() check in the change to nsSVGSVGElement - but I should have used |!IsInner()|, not IsRoot(). In fact, that's not quite right either, since in inline SVG we need to take account of the transforms all the way to window space, so really we need to be calling nsSVGUtils::GetCTM without the IsRoot() check as we were before.

So what I think needs to happen is that nsSVGSVGElement::GetScreenCTM needs to be changed to just call nsSVGUtils::GetCTM. In nsSVGUtils::GetCTM I think we should then add your new |if| right before the |if (!ancestor || !ancestor->IsElement())|, since we would now need nsSVGUtils::GetCTM to handle IsRoot() nsSVGSVGElements too.

Please also add to your |if| an NS_ABORT_IF_FALSE(element->GetType() == nsGkAtoms::svgOuterSVGFrame, "Only outer-<svg> should get here"), plus a comment along the lines of:

  // For outer-<svg> we actually want nsSVGElement::eAllTransforms, not
  // nsSVGElement::eUserSpaceToParent. This isn't very consistent with other
  // elements, but it's what we've been doing for a while, and it keeps us
  // consistent with WebKit and Opera (if not really with the ambiguous spec).
Comment 14 Jonathan Watt [:jwatt] (back in October - email directly if necessary) 2012-06-10 15:48:54 PDT
(In reply to Robert Longson from comment #11)
> Thanks for the regression range jgs it would have been much much harder to
> fix without that.

Seconded! And thanks, Robert, for picking this up!
Comment 15 Jonathan Watt [:jwatt] (back in October - email directly if necessary) 2012-06-10 15:59:11 PDT
By the way, jgs, we could really do with some help catching problems like this _before_ they reach the release builds. The Beta builds are very stable (get virtually no changes before release), so if you'd be willing to use Beta or Aurora builds for your day-to-day browsing and report any regressions that you notice, that would be a great help. If you're interested, you can find Beta/Aurora builds here:

http://www.mozilla.org/en-US/firefox/channel/
Comment 16 jgs 2012-06-11 00:30:22 PDT
Thanks to everyone for picking this up so quickly!

Jonathan, yes we intend to use pre-release builds as a matter of routine now, and will be sure to report any issues we find.
Comment 17 Jonathan Watt [:jwatt] (back in October - email directly if necessary) 2012-06-11 02:46:37 PDT
Fantastic, thanks jgs!
Comment 18 Robert Longson 2012-06-11 08:36:05 PDT
Created attachment 631903 [details] [diff] [review]
attempt to address review comments

(In reply to Jonathan Watt [:jwatt] from comment #13)

> So what I think needs to happen is that nsSVGSVGElement::GetScreenCTM needs
> to be changed to just call nsSVGUtils::GetCTM. In nsSVGUtils::GetCTM I think
> we should then add your new |if| right before the |if (!ancestor ||
> !ancestor->IsElement())|, since we would now need nsSVGUtils::GetCTM to
> handle IsRoot() nsSVGSVGElements too.

OK, I think I've done this.
> 
> Please also add to your |if| an NS_ABORT_IF_FALSE(element->GetType() ==
> nsGkAtoms::svgOuterSVGFrame, "Only outer-<svg> should get here"), plus a
> comment along the lines of:

But I don't follow this completely. We're dealing with elements, not frames. I've pulled out the svg element check to make it return a null matrix but I didn't add an ABORT_IF_FALSE as I'm rather afraid it could go off if we had invalid svg markup.
Comment 19 Robert Longson 2012-06-12 03:38:11 PDT
*** Bug 762895 has been marked as a duplicate of this bug. ***
Comment 20 Jonathan Watt [:jwatt] (back in October - email directly if necessary) 2012-06-12 09:58:24 PDT
Comment on attachment 631903 [details] [diff] [review]
attempt to address review comments

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

Looks good, thanks Robert!

::: layout/svg/base/src/nsSVGUtils.cpp
@@ +509,5 @@
>      // didn't find a nearestViewportElement
>      return gfxMatrix(0.0, 0.0, 0.0, 0.0, 0.0, 0.0); // singular
>    }
> +  if (element->Tag() != nsGkAtoms::svg) {
> +    return gfxMatrix(0.0, 0.0, 0.0, 0.0, 0.0, 0.0); // singular

Can you add a comment like the following before this line:

  // Not a valid SVG fragment

@@ +511,5 @@
>    }
> +  if (element->Tag() != nsGkAtoms::svg) {
> +    return gfxMatrix(0.0, 0.0, 0.0, 0.0, 0.0, 0.0); // singular
> +  }
> +  if (element == aElement && !aHaveRecursed) {

Actually, to be clear, can you start this comment with "We get here when getScreenCTM() is called on an outer-<svg>."

@@ +512,5 @@
> +  if (element->Tag() != nsGkAtoms::svg) {
> +    return gfxMatrix(0.0, 0.0, 0.0, 0.0, 0.0, 0.0); // singular
> +  }
> +  if (element == aElement && !aHaveRecursed) {
> +    // Consistency with other elements would have us return only the

Also, can you s/return/include/, since we're not actually returning here.

@@ +513,5 @@
> +    return gfxMatrix(0.0, 0.0, 0.0, 0.0, 0.0, 0.0); // singular
> +  }
> +  if (element == aElement && !aHaveRecursed) {
> +    // Consistency with other elements would have us return only the
> +    // eFromUserSpace transforms, but this is what we've been doing for

And on reflection "this" is a bit unclear as a comment since the eAllTransforms is implicit. Can you change "this is" to "we include the eAllTransforms transforms in this case since that's"?
Comment 22 Matt Brubeck (:mbrubeck) 2012-06-13 13:29:05 PDT
https://hg.mozilla.org/mozilla-central/rev/88af8c6a5ab9
Comment 23 Robert Longson 2012-06-14 04:54:54 PDT
*** Bug 764738 has been marked as a duplicate of this bug. ***
Comment 24 Jonathan Watt [:jwatt] (back in October - email directly if necessary) 2012-06-14 05:57:16 PDT
Comment on attachment 631903 [details] [diff] [review]
attempt to address review comments

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 660216
User impact if declined: Broken coordinate transforms in SVG web apps, without a good workaround
Testing completed (on m-c, etc.): passed tests on m-c, new tests added
Risk to taking this patch (and alternatives if risky): very low
String or UUID changes made by this patch: none
Comment 25 Lukas Blakk [:lsblakk] use ?needinfo 2012-06-15 15:32:22 PDT
Comment on attachment 631903 [details] [diff] [review]
attempt to address review comments

[Triage Comment]
Looks good, low risk regression from 13 fixed.
Comment 26 paperpond 2012-06-19 11:46:02 PDT
Hello,

This does not appear to be fixed. This drag and drop example uses svg and javascript. It relies on getScreenCTM to translate the the cursor coordinates between the browser and the svg viewport when the browser window resizes.

It works in IOS, Android, IE, and Chrome and it used to work in Firefox:

http://www.toytheater.com/getScreenCTM.html

Thanks
Comment 27 Robert Longson 2012-06-19 15:15:29 PDT
(In reply to paperpond from comment #26)
> 
> http://www.toytheater.com/getScreenCTM.html

This testcase works fine for me on trunk and is broken in 13. You are testing with a trunk nightly aren't you?
Comment 28 paperpond 2012-06-20 05:47:38 PDT
Sorry,

I was testing with 13, Beta, and Aurora, the fix must be in a build I do not have. When does it get pushed to the standard version?

Thanks
Comment 29 Robert Longson 2012-06-20 06:12:42 PDT
status-firefox14 and status-firefox15 (top right of this form) will be updated when that happens.
Comment 31 Robert Longson 2012-06-21 12:17:53 PDT
Daniel, I've just realised looking at what's checked in that the mochitest is not what I landed in the end. I simplified the changes because it failed on some platforms and then forgot about it, totally my fault and I'm so sorry after you stepped in to land it for me. 

You could back out all the mochitest changes for beta/aurora since they are on trunk or fix them up to look like the trunk check in.
Comment 32 Daniel Holbert [:dholbert] 2012-06-21 12:23:26 PDT
Hi Robert! I actually backported the exact cset that was checked in on trunk, like so:
> $ hgc export 88af8c6a5ab9 | hg qimport --name 762411.patch -
so I believe that I captured your mochitest updates.  So I think all is OK, but let me know if that ends up not being the case.
Comment 33 Robert Longson 2012-06-21 12:30:10 PDT
Indeed, despite my best efforts you didn't let me mess it up after all ;-)
Comment 34 Daniel Holbert [:dholbert] 2012-06-21 13:37:55 PDT
The new mochitest checks actually did end up failing on beta, on Mac at least:
 https://tbpl.mozilla.org/php/getParsedLog.php?id=12874336&tree=Mozilla-Beta
so I tentatively backed this out:
 https://hg.mozilla.org/releases/mozilla-beta/rev/22aee8436b4c

I don't have time to sort it out right now; we can figure it out & reland with that fixed sometime soon.
Comment 35 Daniel Holbert [:dholbert] 2012-06-21 13:49:16 PDT
The mochitests seem to pass on Aurora, so the failures on Beta are presumably due to a dependency / interaction with another checkin that hasn't made it to Beta.
Comment 36 Robert Longson 2012-06-22 12:59:52 PDT
Most, but not all of the difference is down to bug 755056 not being on beta.

Without this, when we call

  nsPoint point = frame->GetOffsetTo(ancestorFrame);

at the end of nsSVGUtils::GetCTMInternal we get 0 for the point, with it, we get non-zero values driven by the non-zero foreignObject mRect x and y.

Possible solutions:

a) Take bug 755056 (but I think we'd need to take bug 734082 to get the numbers exactly right)
b) simplify the mochitest so that foreignObject associated with outer2 has x="0" and y="0" for beta

I'm going to do a patch for b) as I think a) is too risky for beta. Since this is just a mochitest change I think we ought to just be able to reland this then.
Comment 37 Robert Longson 2012-06-22 14:13:13 PDT
Created attachment 635908 [details] [diff] [review]
patch for beta

This is the same code change but with a simpler mochitest (the foreignObject surrounding element outer2 has no x or y attributes). This mochitest passes both on beta and trunk.
Comment 38 Daniel Holbert [:dholbert] 2012-06-26 10:34:04 PDT
Landed new patch for beta (same as beta patch in comment 30, modulo Robert's mochitest tweaks):
  https://hg.mozilla.org/releases/mozilla-beta/rev/ea166d54c12b
Comment 39 Paul Silaghi, QA [:pauly] 2012-07-03 05:12:11 PDT
I've noticed this on Mozilla/5.0 (Windows NT 6.1; WOW64; rv:14.0) Gecko/20100101 Firefox/14.0b10:

1. Reduce the FF window and place it on the center of the screen
2. Open the testcase
3. Quickly drag the mouse pointer from left to the right over the FF window

Actual result:
The black circle gets stuck somewhere in its white space, not closer to the edge of the browser.

Expected results:
The black circle should be stuck to the edge of the browser when the mouse pointer exceed the FF window.
Comment 40 Robert Longson 2012-07-03 08:20:26 PDT
This usually indicates a failure to call preventDefault on the event object in the event handlers

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