getScreenCTM scaling broken on inline embedded 'svg' elements

RESOLVED FIXED in Firefox 14

Status

()

Core
SVG
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jgs, Assigned: Robert Longson)

Tracking

({regression})

13 Branch
mozilla16
regression
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox13 affected, firefox14 fixed, firefox15 fixed, firefox16 fixed)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
Would you care to find a regression range?
(Assignee)

Comment 2

5 years ago
https://quality.mozilla.org/docs/bugzilla/guide-to-triaging-bugs-for-firefox/finding-a-regression-window/
(Reporter)

Comment 3

5 years ago
Will check out the regression window link and get back to you.
(Assignee)

Updated

5 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Reporter)

Comment 4

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

Updated

5 years ago
Keywords: regression
(Assignee)

Comment 5

5 years ago
Could you type about:buildconfig in the address bar in each of these and post the Built from link please?
(Reporter)

Comment 6

5 years ago
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
(Assignee)

Comment 7

5 years ago
Which means we're looking for something in here: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=2271cb92cc05&tochange=550779e6bab4
(Assignee)

Comment 8

5 years ago
Which means it's almost certainly bug 660216
Blocks: 660216
(Assignee)

Comment 9

5 years ago
Reverting part 3 of bug 660216 fixes this.
(Assignee)

Comment 10

5 years ago
Created attachment 631679 [details] [diff] [review]
patch with test
Assignee: nobody → longsonr
Attachment #631679 - Flags: review?(jwatt)
(Assignee)

Comment 11

5 years ago
Thanks for the regression range jgs it would have been much much harder to fix without that.
For convenience, part 3 of bug 660216 fixes as checked in is:

http://hg.mozilla.org/mozilla-central/rev/b3a3372fecae
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).
(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!
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/
(Reporter)

Comment 16

5 years ago
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.
Fantastic, thanks jgs!
(Assignee)

Comment 18

5 years ago
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.
Attachment #631679 - Attachment is obsolete: true
Attachment #631679 - Flags: review?(jwatt)
Attachment #631903 - Flags: review?(jwatt)
(Assignee)

Updated

5 years ago
Duplicate of this bug: 762895
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"?
Attachment #631903 - Flags: review?(jwatt) → review+
(Assignee)

Comment 21

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/88af8c6a5ab9
Flags: in-testsuite+
Target Milestone: --- → mozilla16
(Assignee)

Updated

5 years ago
status-firefox13: --- → affected
status-firefox14: --- → affected
status-firefox15: --- → affected
status-firefox16: --- → affected
https://hg.mozilla.org/mozilla-central/rev/88af8c6a5ab9
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Assignee)

Updated

5 years ago
status-firefox16: affected → fixed
(Assignee)

Updated

5 years ago
Duplicate of this bug: 764738
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
Attachment #631903 - Flags: approval-mozilla-beta?
Attachment #631903 - Flags: approval-mozilla-aurora?

Updated

5 years ago
OS: Windows 7 → All
Hardware: x86_64 → All
Comment on attachment 631903 [details] [diff] [review]
attempt to address review comments

[Triage Comment]
Looks good, low risk regression from 13 fixed.
Attachment #631903 - Flags: approval-mozilla-beta?
Attachment #631903 - Flags: approval-mozilla-beta+
Attachment #631903 - Flags: approval-mozilla-aurora?
Attachment #631903 - Flags: approval-mozilla-aurora+

Comment 26

5 years ago
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
(Assignee)

Comment 27

5 years ago
(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

5 years ago
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
(Assignee)

Comment 29

5 years ago
status-firefox14 and status-firefox15 (top right of this form) will be updated when that happens.
https://hg.mozilla.org/releases/mozilla-aurora/rev/71f4f34bd47d
https://hg.mozilla.org/releases/mozilla-beta/rev/431d9eb7aa0c
status-firefox14: affected → fixed
status-firefox15: affected → fixed
(Assignee)

Comment 31

5 years ago
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.
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.
(Assignee)

Comment 33

5 years ago
Indeed, despite my best efforts you didn't let me mess it up after all ;-)
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.
status-firefox14: fixed → affected
Whiteboard: [needs re-landing on beta after mochitest issues are sorted out]
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.
(Assignee)

Comment 36

5 years ago
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.
(Assignee)

Comment 37

5 years ago
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.
(Assignee)

Updated

5 years ago
Whiteboard: [needs re-landing on beta after mochitest issues are sorted out] → [needs re-landing on beta]
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
status-firefox14: affected → fixed
Whiteboard: [needs re-landing on beta]
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.
(Assignee)

Comment 40

5 years ago
This usually indicates a failure to call preventDefault on the event object in the event handlers
You need to log in before you can comment on or make changes to this bug.