Closed Bug 366697 Opened 13 years ago Closed 11 years ago

.getCTM() shouldn't return the same matrix as .getScreenCTM() for nested SVG elements

Categories

(Core :: SVG, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: i.aint.online, Assigned: ryoqun)

References

()

Details

(Keywords: testcase)

Attachments

(3 files, 3 obsolete files)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8.1) Gecko/20061010 Firefox/2.0
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8.1) Gecko/20061010 Firefox/2.0

According to:
1. http://www.w3.org/TR/SVG/coords.html#ElementsThatEstablishViewports
2. http://www.w3.org/TR/SVG/types.html#InterfaceSVGLocatable
calling the .getCTM() method should return E and F relative to the upper (in the DOM hierarchy) SVG element and not relative to the root, as it seems to happen.

Reproducible: Always

Steps to Reproduce:
1.See URL

Actual Results:  
Returns E=11 and F=41

Expected Results:  
Should return E=1 and F=1

Works the same with the adobe's SVG plugin. Seems to work as in specs in Opera 9.1
Assignee: nobody → general
Component: General → SVG
Product: Firefox → Core
QA Contact: general → ian
Version: unspecified → 1.8 Branch
Keywords: testcase
Assignee: general → jwatt
OS: Windows 2000 → All
Hardware: PC → All
Version: 1.8 Branch → Trunk
Status: UNCONFIRMED → NEW
Ever confirmed: true
Status: NEW → ASSIGNED
Jonathan, if you are not going to fix this in near future, can you reassign it to me? I want to fix this.
Sure thing.
Assignee: jwatt → ryoqun
Attached patch patch v1(reftest included) (obsolete) — Splinter Review
I found that getCTM shouldn't calculate the CTM beyond the nearest viewport element, and fixed the problem. Also, I created a reftest.
Attachment #377994 - Flags: review?(longsonr)
The nearest viewport element can be obtained via nsSVGUtils::GetNearestViewport. This method should use that one.

It would be better to test with a mochitest since we are testing something that is non-visual. See http://mxr.mozilla.org/mozilla-central/source/content/svg/content/test/test_viewport.html?force=1

Finally, could you fix the style issue in the method so else is on the same line as }
Attachment #377994 - Flags: review?(longsonr) → review-
OK, not quite finally there...

There's another implementation of GetCTM in nsSVGGraphicElement. It would be better if the nsSVGSVGElement code was moved to nsSVGUtils and then both nsSVGGraphicElement and nsSVGSVGElement called that single implementation.

FYI There's an additional test here
http://www.w3.org/Graphics/SVG/Test/20061213/htmlObjectHarness/full-types-basicDOM-01-b.html
Attached patch patch v2 (obsolete) — Splinter Review
Moved to Mochitest, fixed the style issue and cleaned my modification a bit. Currently, my first Mochitest is really small. Should I make it more exhaustive? As for the remaining two requests, I have some concerns:

nsSVGSVGElement::GetCTM behaves in four ways depending on the location in the tree. Particularly, the forth |if| clause, which ancestorCount is used with, is problematic. If I switched to SVGUtils::GetNearestViewportElement, I couldn't use ancestorCount and enter there, because SVGUtils::GetNearestViewportElement merely returns nsnull. Also I'm not sure SVGUtils::GetNearestViewportElement is really identical to the current |while| loop.

nsSVGSVGElement::GetCTM is called from nsSVGGraphicElement::GetCTM. So, those two methods aren't exactly duplicated code albeit the same name. And nsSVGGraphicElement::GetCTM is recursively called until it reaches to nsSVGSVGElement::GetCTM. Although I haven't thought hard, I'm vaguely thinking the merge would unnecessaryly complicates the code. Nevertheless, should I try to merge the two into SVGUtils::GetCTM and why?

Because I don't fully understand the code, I want my modification to be minimal at the moment. But, If I should do the above two refactoring this time, I'll do it anyway.
Attachment #377994 - Attachment is obsolete: true
Attachment #378030 - Flags: review?(longsonr)
> nsSVGSVGElement::GetCTM behaves in four ways depending on the location in the
> tree. Particularly, the forth |if| clause, which ancestorCount is used with, is
> problematic. If I switched to SVGUtils::GetNearestViewportElement, I couldn't
> use ancestorCount and enter there, because SVGUtils::GetNearestViewportElement
> merely returns nsnull. Also I'm not sure SVGUtils::GetNearestViewportElement is
> really identical to the current |while| loop.

This element is the root element (equivalent to ancestorCount == 0) if nsSVGUtils::GetParentElement either returns null or an nsSVGForeignObjectElement.

The code we have now isn't equivalent to nsSVGUtils::GetNearestViewportElement and that's a problem because it should be. You might want to think about extending the mochitest to cover the sort of cases that the getnearestviewport mochitest tests.

> 
> nsSVGSVGElement::GetCTM is called from nsSVGGraphicElement::GetCTM. So, those
> two methods aren't exactly duplicated code albeit the same name. And
> nsSVGGraphicElement::GetCTM is recursively called until it reaches to
> nsSVGSVGElement::GetCTM. Although I haven't thought hard, I'm vaguely thinking
> the merge would unnecessaryly complicates the code. Nevertheless, should I try
> to merge the two into SVGUtils::GetCTM and why?
> 
> Because I don't fully understand the code, I want my modification to be minimal
> at the moment. But, If I should do the above two refactoring this time, I'll do
> it anyway.

A common iterative solution in nsSVGUtils rather than the recursive solution as we have now would be better I think. If you want to do that in a separate bug that's fine. There will be some extra code to deal with outer SVG elements but why should the code to get the CTM for an inner SVG element differ from the code for a g element.
Summary: SVG's .getCTM() method returns wrong E and F values for embedded SVG elements → .getCTM() shouldn't return the same matrix as .getScreenCTM() for nested SVG elements
Thank you for answering in detail! I wanted to know the reason behind your requests and now these make sense, I'll address them in several days.

Also, I want to get this patch into 1.9.1 because I think implementing an API in a wrong way is worse than not implementing it at all. Well, I was tripped with this bug when playing with SVG...
In that case, should I request approval1.9.1 with the current patch or a coming refactored one?
I don't think this is 1.9.1 material. bug 483389 would have to go in first. You're too late really.
Attachment #378030 - Flags: review?(longsonr) → review-
(In reply to comment #9)
>... I think implementing an API
> in a wrong way is worse than not implementing it at all.

This bit I agree with though :-)
Because GetCTM is defined loosely, I made a testcase for checking invariants governing its family API of transformations. When this file is clicked, it tests several invariants across GetCTM, GetScreenCTM and GetTransformToElement. I used this behavior as the definition of GetCTM. After the calculation, it draws overlapping lines over existing lines using the acquired coordinates. If one of existing lines aren't overlapped, something is wrong. To note, none of Batik, webkit, presto doesn't do all correctly, but patched gecko does. After all, firefox was having problems only with GetCTM... The new our GetCTM becomes to follow webkit and presto. Batik has a problem, maybe.
Attached patch patch v3 (obsolete) — Splinter Review
I created a half-backed patch. Because this is the first patch which contains substantial amount of rewrite, you need to review this carefully, particularly for the coding style and conventions for mozilla. I'm pretty sure that there is some minor errors.

Generally, I have some uncertainties of the exact behavior of GetCTM. This is much like to nearestViewportElement. The spec is too ambiguous or lacks the detailed specification about it. To be honest, I've got less interested to implement this completely. Maybe, I should stop where basic testcases posted here start to working correctly.. Of cource, this patch does. And the extended mochitest covers most of use cases, I hope.

I found one mysterious thing in the original code. It is |if (viewportElement)| at http://mxr.mozilla.org/mozilla-central/source/content/svg/content/src/nsSVGSVGElement.cpp#809. Is never evaluates to true albeit the intention of the author of the code. As far as I read, if it worked as intended, GetCTM would work in the different manner than the GetScreenCTM, which would differ from my new implementation of GetCTM in detail....
Attachment #378030 - Attachment is obsolete: true
Attachment #379326 - Flags: review?(longsonr)
Attachment #379326 - Flags: review?(longsonr) → review-
Comment on attachment 379326 [details] [diff] [review]
patch v3

>+nsresult
>+nsSVGForeignObjectElement::AppendTranslate(nsIDOMSVGMatrix *aCTM,
>+                                           nsIDOMSVGMatrix **_retval)
>+{
>+  nsresult rv;

Declare rv where you use it below in one line

>+  // foreignObject is one of establishing-viewport elements.
>+  // so we are translated by foreignObject's x and y attribs.
>+  // cast to nsSVGElement so we get our ancestor coord context.
>+  const float x = mLengthAttributes[X].GetAnimValue(static_cast<nsSVGElement*>(this));
>+  const float y = mLengthAttributes[Y].GetAnimValue(static_cast<nsSVGElement*>(this));

call GetAnimatedLengthValues(&x, &y, nsnull); instead of the above.

> /* Helper for GetCTM and GetScreenCTM */
> nsresult
>-nsSVGGraphicElement::AppendLocalTransform(nsIDOMSVGMatrix *aCTM,
>-                                          nsIDOMSVGMatrix **_retval)
>+nsSVGGraphicElement::AppendTransform(nsIDOMSVGMatrix *aCTM,
>+                                     nsIDOMSVGMatrix **_retval)
> {
>+  nsCOMPtr<nsIDOMSVGMatrix> ctm = aCTM;
>+  if (nsCOMPtr<nsIDOMSVGForeignObjectElement>(do_QueryInterface((nsIContent *)this))) {

Looks like AppendTransform should be virtual then the nsSVGForeignObjectElement version can just do the right thing rather than checking explicitly for it here.

>+nsSVGSVGElement::AppendTransform(nsIDOMSVGMatrix *aCTM,
>+                                 nsIDOMSVGMatrix **_retval)

...

>+    } else {
>+      // this is a nested <svg> element. our immediate parent is an SVG element.
>+      // cast to nsSVGElement so we get our ancestor coord context.
>+      x = mLengthAttributes[X].GetAnimValue(static_cast<nsSVGElement*>(this));
>+      y = mLengthAttributes[Y].GetAnimValue(static_cast<nsSVGElement*>(this));

GetAnimatedLengthValues(&x, &y, nsnull) instead.

>   if (viewbox.width <= 0.0f || viewbox.height <= 0.0f) {
>+    NS_NOTREACHED("ivalid - don't paint element.");

s/ivalid/invalid/ though if svg files can produce this then you shouldn't add this line. The principle is that svg files should not be able to generate warnings, instead if the svg file is wrong we should log the error to the Error Console. Unfortunately logging to the Error Console is difficult as we don't want to do that on every repaint, only when we first create frames which is why there isn't much Error Console logging code.

> 
>+nsresult
>+nsSVGUtils::AppendTransformUptoElement(nsIContent *aContent, nsIDOMSVGElement *aElement, nsIDOMSVGMatrix * *aCTM){
>+  nsresult rv;
>+  nsCOMPtr<nsIDOMSVGElement> element = do_QueryInterface(aContent);
>+  nsIContent *ancestor = GetParentElement(aContent);
>+  if (!aElement) {
>+    // calculating GetScreenCTM(): traverse upto the root or non-SVG content.
>+    if (ancestor) {
>+      if (ancestor->GetNameSpaceID() == kNameSpaceID_SVG) {

Combine the above two lines.

>diff -r 1141d12a50b5 layout/svg/base/src/nsSVGUtils.h
>--- a/layout/svg/base/src/nsSVGUtils.h    Fri May 15 20:56:11 2009 +0200
>+++ b/layout/svg/base/src/nsSVGUtils.h    Sat May 23 18:08:52 2009 +0900
>@@ -253,6 +253,10 @@
>   static float CoordToFloat(nsPresContext *aPresContext,
>                             nsSVGElement *aContent,
>                             const nsStyleCoord &aCoord);
>+  /* Return the CTM */
>+  static nsresult AppendTransformUptoElement(nsIContent *aContent, nsIDOMSVGElement *aElement, nsIDOMSVGMatrix * *aCTM);
>+  static nsresult GetCTM(nsIContent *aContent, nsIDOMSVGMatrix * *aCTM);
>+  static nsresult GetScreenCTM(nsIContent *aContent, nsIDOMSVGMatrix * *aCTM);

Could do with some individual comments about what these methods do (especially the first one). Also wrap the arguments so we don't have lines that are too long.
(In reply to comment #13)
> Created an attachment (id=379326) [details]
> patch v3
> 
> I created a half-backed patch. Because this is the first patch which contains
> substantial amount of rewrite, you need to review this carefully, particularly
> for the coding style and conventions for mozilla. I'm pretty sure that there is
> some minor errors.

Looks pretty good to me. Not sure why you think it is half-baked. Coding style is absolutely fine.

> 
> Generally, I have some uncertainties of the exact behavior of GetCTM. This is
> much like to nearestViewportElement. The spec is too ambiguous or lacks the
> detailed specification about it. To be honest, I've got less interested to
> implement this completely. Maybe, I should stop where basic testcases posted
> here start to working correctly.. Of cource, this patch does. And the extended
> mochitest covers most of use cases, I hope.

The only other testcase I know is http://www.w3.org/Graphics/SVG/Test/20061213/htmlObjectHarness/full-types-basicDOM-01-b.html can you confirm that still works?

Does Opera match the results that this patch produces?

I think you're pretty nearly there, I didn't make any really substantial comments. Please do keep going now that you're so close.

> 
> I found one mysterious thing in the original code. It is |if (viewportElement)|
> at
> http://mxr.mozilla.org/mozilla-central/source/content/svg/content/src/nsSVGSVGElement.cpp#809.
> Is never evaluates to true albeit the intention of the author of the code. As
> far as I read, if it worked as intended, GetCTM would work in the different
> manner than the GetScreenCTM, which would differ from my new implementation of
> GetCTM in detail....

We should just try to do it right as best we can now rather than trying to guess what the original code was supposed to do :-)
Attached patch patch v4Splinter Review
Here is the revised patch!

>>   if (viewbox.width <= 0.0f || viewbox.height <= 0.0f) {
>>+    NS_NOTREACHED("ivalid - don't paint element.");
>s/ivalid/invalid/ though if svg files can produce this then you shouldn't add
>this line. The principle is that svg files.....
I removed it. Adhering the principle, I removed other warnings except one as well. The one is converted to XXX. The accompanied mochitest reaches to it when it tests the <use> elements referencing <symbol>. Maybe, I should do something for <use>/<symbol>, but I can hardly find any justification for the effort. After all, child elements of <symbol> will naturally be instantiated by multiple times in the wild SVGs. In that case, |child elements|.get[Screen]CTM() gets really undefined. Moreover, <use> can even reference to other <svg> and <g>, making fixing this really hard while being meaningful and/or useful at the same time.. So, I just left it as is.

>The only other testcase I know is
>http://www.w3.org/Graphics/SVG/Test/20061213/htmlObjectHarness/full-types-basicDOM-01-b.html
>can you confirm that still works?
It does. Actually, this testcase doesn't test the difference of GetCTM and GetScreenCTM. It merely tests the two return the same matrix when they should do. So, it isn't very useful regarding this bug.

>Does Opera match the results that this patch produces?
Hmm. I'm not sure about the Mochitest, because currently I don't know how to run Mochitest on Opera. Should I do for the interop? But, I'm sure that my click-and-paint testcase works in the same way between Firefox, Opera, Safari and Chrome on Windows XP SP3(at least on getCTM part). Well, I noticed that opera has a bug when zooming is not 100%.
Attachment #379326 - Attachment is obsolete: true
Attachment #379553 - Flags: review?(longsonr)
Attachment #379553 - Flags: review?(longsonr) → review+
> 
> >Does Opera match the results that this patch produces?
> Hmm. I'm not sure about the Mochitest, because currently I don't know how to
> run Mochitest on Opera. Should I do for the interop? But, I'm sure that my
> click-and-paint testcase works in the same way between Firefox, Opera, Safari
> and Chrome on Windows XP SP3(at least on getCTM part). Well, I noticed that
> opera has a bug when zooming is not 100%.

You can't run mochitests directly using Opera as far as I know. You would have to change the mochitests into something more like your click-and-paint testcase. Your click-and paint testcase already does that though.

Great stuff. Well done and thanks for persevering.
Yay! Thanks you for reviewing, too.

Then, what should I do? Request a super review?
No, you're done here :-) The tree is semi-closed at the moment. When it reopens to this kind of fix I'll land it for you.
I heard that the trunk is now open. Robert, can you commit this?
Pushed http://hg.mozilla.org/mozilla-central/rev/eeb04deff8f3
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Depends on: 543965
If I do a getCTM on an element in an SVG that has been transformed by the viewBox attribute, the CTM it returns is transformed. Shouldn't that only happen through getScreenCTM?

(This behavior happens in both Firefox and Chrome - I'm reporting it here because it seems wrong for either browser, and Firefox has an existing bug for overly-transformed getCTM results.)
If you think there's an issue Stuart please raise a new bug with a testcase showing the problem.
You need to log in before you can comment on or make changes to this bug.