Last Comment Bug 660216 - getBBox and getCTM on foreignObject should/should not include 'x'/'y' offsets respectively
: getBBox and getCTM on foreignObject should/should not include 'x'/'y' offsets...
Status: RESOLVED FIXED
: testcase
Product: Core
Classification: Components
Component: SVG (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla13
Assigned To: Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13)
:
Mentors:
Depends on: 762411 780169
Blocks: 614732
  Show dependency treegraph
 
Reported: 2011-05-27 05:39 PDT by David Rivron
Modified: 2012-08-04 17:05 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
foreignObject getBBox getCTM testcase (441 bytes, image/svg+xml)
2011-05-27 06:51 PDT, David Rivron
no flags Details
rect getBBox getCTM testcase (387 bytes, image/svg+xml)
2011-05-30 02:21 PDT, David Rivron
no flags Details
patch (23.87 KB, patch)
2012-02-12 13:15 PST, Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13)
no flags Details | Diff | Review
part 1 - rename PrependLocalTransformTo to PrependLocalTransformsTo (19.93 KB, patch)
2012-02-13 05:22 PST, Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13)
cam: review+
Details | Diff | Review
part 2 - Allow PrependLocalTransformsTo callers to specify which transforms they want (14.14 KB, patch)
2012-02-13 05:33 PST, Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13)
cam: review+
Details | Diff | Review
part 3 - fix getBBox and getCTM on foreignObject (10.10 KB, patch)
2012-02-13 05:34 PST, Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13)
cam: review+
Details | Diff | Review

Description David Rivron 2011-05-27 05:39:35 PDT
User-Agent:       Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0.1) Gecko/20100101 Firefox/4.0.1
Build Identifier: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:7.0a1) Gecko/20110526 Firefoxnightly/7.0a1

getBBox do not return the position defined by x and y while getCTM include this position in the transformation matrix.

Reproducible: Always

Steps to Reproduce:
<?xml version="1.0" encoding="UTF-8"?>
<svg version="1.0" width="120" height="120" xmlns="http://www.w3.org/2000/svg">
	<script type="text/ecmascript">
		window.addEventListener('load', function() {
			var foreignObject = document.getElementById('foreignObject');
			alert(foreignObject.getBBox().x + " " + foreignObject.getCTM().e);
		}, false);
	</script>
	<foreignObject id="foreignObject" x="10" y="10" width="100" height="100" />
</svg>

Actual Results:  
0 10

Expected Results:  
10 0
Comment 1 Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13) 2011-05-27 05:53:27 PDT
Thanks for the bug report David. It's helpful if you can use the "Add an attachment" link to attach your testcase so that it can be viewed live.
Comment 2 David Rivron 2011-05-27 06:51:53 PDT
Created attachment 535623 [details]
foreignObject getBBox getCTM testcase
Comment 3 XtC4UaLL [:xtc4uall] 2011-05-28 02:26:42 PDT
Confirmed against Mozilla/5.0 (Windows NT 5.1; rv:7.0a1) Gecko/20110527 Firefox/7.0a1 ID:20110527030642. Opera 11.5/Google Chrome 13 get it as expected.
Comment 4 Robert Longson 2011-05-29 11:42:06 PDT
We've asked the SVG WG for clarification (at least on the getCTM issue) and not received any. http://lists.w3.org/Archives/Public/www-svg/2005Jan/0015.html
Comment 5 David Rivron 2011-05-30 02:20:56 PDT
I have mention the getCTM issue because he has a different behavior than other SVG objects (rect, for example) for which position attributes are not included in the CTM.
Comment 6 David Rivron 2011-05-30 02:21:51 PDT
Created attachment 536038 [details]
rect getBBox getCTM testcase
Comment 7 George Carstoiu 2011-06-01 23:39:39 PDT
Mozilla/5.0 (X11; Linux i686; rv:7.0a1) Gecko/20110601 Firefox/7.0a1


Confirming issue and setting resolution to New.

Issue present ever since Firefox 3.6.17
Comment 8 Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13) 2012-02-10 04:01:42 PST
(In reply to David Rivron from comment #5)
> I have mention the getCTM issue because he has a different behavior than
> other SVG objects (rect, for example) for which position attributes are not
> included in the CTM.

Yeah, that's wrong.

Taking this, since I need the infrastructure required to fix this bug as part of bug 614732.
Comment 9 Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13) 2012-02-12 13:15:05 PST
Created attachment 596515 [details] [diff] [review]
patch
Comment 10 Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13) 2012-02-13 05:21:40 PST
Actually, let's split this into three logically self contained patches.
Comment 11 Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13) 2012-02-13 05:22:47 PST
Created attachment 596657 [details] [diff] [review]
part 1 - rename PrependLocalTransformTo to PrependLocalTransformsTo
Comment 12 Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13) 2012-02-13 05:33:45 PST
Created attachment 596658 [details] [diff] [review]
part 2 - Allow PrependLocalTransformsTo callers to specify which transforms they want
Comment 13 Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13) 2012-02-13 05:34:41 PST
Created attachment 596659 [details] [diff] [review]
part 3 - fix getBBox and getCTM on foreignObject
Comment 14 Cameron McCormack (:heycam) (away Jun 25 – Jul 10) 2012-02-16 17:58:45 PST
Comment on attachment 596658 [details] [diff] [review]
part 2 - Allow PrependLocalTransformsTo callers to specify which transforms they want

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

::: content/svg/content/src/nsSVGElement.h
@@ +154,5 @@
> +  enum WhichTransforms {
> +     eAll
> +    ,eFromUserSpace
> +    ,eToUserSpace
> +  };

I think commas at the end of the lines would be more consistent with other multi-line enum declarations we have.

I'd prefer a name for the enum that is more clearly a noun, like TransformTypes.  eAll also seems a bit too generic, given that it will be within the nsSVGElement namespace.  Can it be given a name like eAllTransforms?

Also, I think the names eFromUserSpace and eToUserSpace make them sound like they are inverses of each other.  How about something like eUserSpaceToParent and eChildToUserSpace?

@@ +175,5 @@
> +   * If aWhich is eToUserSpace, then only the transforms from the coordinate
> +   * space established by this element for its childre to this elements
> +   * userspace are included. This includes any offsets due to e.g. 'x'/'y'
> +   * attributes, and any transform due to a 'viewBox' attribute, but does not
> +   * include any transforms due to the 'transform' attribute.

Can you note in this comment where motion animation transforms come in?

::: content/svg/content/src/nsSVGForeignObjectElement.cpp
@@ +117,4 @@
>  {
> +  NS_ABORT_IF_FALSE(aWhich != eToUserSpace || aMatrix.IsIdentity(),
> +                    "Skipping eFromUserSpace transforms makes no sense");
> +

Why do we need this abort here (and in the other implementations of this method)?  Might there not be conceivable uses of PrependLocalTransformsTo that want to get out the eFromUserSpace transform and prepend it to a non-identity matrix?  It is just that all the current uses of this API won't need to do this?  Is there any downside to just supporting this?
Comment 15 Cameron McCormack (:heycam) (away Jun 25 – Jul 10) 2012-02-16 18:26:11 PST
Comment on attachment 596659 [details] [diff] [review]
part 3 - fix getBBox and getCTM on foreignObject

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

::: content/svg/content/src/nsSVGSVGElement.cpp
@@ +754,5 @@
> +  if (IsRoot()) {
> +    // Consistancy with other elements would have us return only the
> +    // eFromUserSpace transforms, but this is what we've been doing for
> +    // a while, and it keeps us consistant with webkit and opera (if not
> +    // really with the ambiguous spec).

Consistency, consistent, WebKit, Opera.

I cannot see an issue in the WG tracker for the ambiguity.  Can you raise one?

::: layout/svg/base/src/nsSVGUtils.cpp
@@ +469,5 @@
> +  bool &mFlag;
> +  bool mOldVal;
> +};
> +}
> +

This seems like a neat class!  I wonder if it should go somewhere more general, like in mfbt/?  I think it could be nicely templatised, too.

(No need to do this now, just a thought.)

@@ +479,5 @@
>      // Flush all pending notifications so that our frames are up to date
>      currentDoc->FlushPendingNotifications(Flush_Layout);
>    }
>  
> +  static bool sHaveRecursed = false;

I don't understand the use of this flag.  I see nsSVGGraphicElement::GetCTM calls nsSVGUtils::GetCTM.  Why is this not a problem currently?  I think it needs a comment to explain why the flag is needed.
Comment 16 Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13) 2012-02-16 22:20:14 PST
(In reply to Cameron McCormack (:heycam) from comment #14)
> I think commas at the end of the lines would be more consistent with other
> multi-line enum declarations we have.

Actually it's quite common to put them at the beginning to help preserve 'blame' when new types are added.

> I'd prefer a name for the enum that is more clearly a noun, like
> TransformTypes.  eAll also seems a bit too generic, given that it will be
> within the nsSVGElement namespace.  Can it be given a name like
> eAllTransforms?
> 
> Also, I think the names eFromUserSpace and eToUserSpace make them sound like
> they are inverses of each other.  How about something like
> eUserSpaceToParent and eChildToUserSpace?

It's only inverse if you have your SVG matrix convention hat on - we're in gfx matrix land here, and that has the opposite convention. Anyway, yeah, I can change all that.

> Can you note in this comment where motion animation transforms come in?

Yeah.

> Why do we need this abort here (and in the other implementations of this
> method)?  Might there not be conceivable uses of PrependLocalTransformsTo
> that want to get out the eFromUserSpace transform and prepend it to a
> non-identity matrix?  It is just that all the current uses of this API won't
> need to do this?  Is there any downside to just supporting this?

The downside is that it's likely a bug, due to someone having there SVG matrix convention hat on when they should have their gfx matrix convention hat on. If we find a use for it, we can remove those asserts, but right now I think it's wise to keep them.
Comment 17 Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13) 2012-02-16 22:24:49 PST
(In reply to Cameron McCormack (:heycam) from comment #15)
> I cannot see an issue in the WG tracker for the ambiguity.  Can you raise
> one?

Sure.

> This seems like a neat class!  I wonder if it should go somewhere more
> general, like in mfbt/?  I think it could be nicely templatised, too.
> 
> (No need to do this now, just a thought.)

Yeah, I'll look into that as a follow-up.

> > +  static bool sHaveRecursed = false;
> 
> I don't understand the use of this flag.  I see nsSVGGraphicElement::GetCTM
> calls nsSVGUtils::GetCTM.  Why is this not a problem currently?  I think it
> needs a comment to explain why the flag is needed.

It's there because nsSVGUtils::GetCTM calls *itself*. When it's passed something other than eAll, we want to make sure that as it looks up the parent chain the non-eAll is converted to eAll for the recursing calls, otherwise we'd be skipping transforms on each element along the parent chain. I'll add a comment.
Comment 18 Cameron McCormack (:heycam) (away Jun 25 – Jul 10) 2012-02-16 22:41:00 PST
(In reply to Jonathan Watt [:jwatt] from comment #16)
> (In reply to Cameron McCormack (:heycam) from comment #14)
> > I think commas at the end of the lines would be more consistent with other
> > multi-line enum declarations we have.
> 
> Actually it's quite common to put them at the beginning to help preserve
> 'blame' when new types are added.

OK, if that is the convention we prefer.

> > I'd prefer a name for the enum that is more clearly a noun, like
> > TransformTypes.  eAll also seems a bit too generic, given that it will be
> > within the nsSVGElement namespace.  Can it be given a name like
> > eAllTransforms?
> > 
> > Also, I think the names eFromUserSpace and eToUserSpace make them sound like
> > they are inverses of each other.  How about something like
> > eUserSpaceToParent and eChildToUserSpace?
> 
> It's only inverse if you have your SVG matrix convention hat on - we're in
> gfx matrix land here, and that has the opposite convention. Anyway, yeah, I
> can change all that.

Cool.

> > Can you note in this comment where motion animation transforms come in?
> 
> Yeah.
> 
> > Why do we need this abort here (and in the other implementations of this
> > method)?  Might there not be conceivable uses of PrependLocalTransformsTo
> > that want to get out the eFromUserSpace transform and prepend it to a
> > non-identity matrix?  It is just that all the current uses of this API won't
> > need to do this?  Is there any downside to just supporting this?
> 
> The downside is that it's likely a bug, due to someone having there SVG
> matrix convention hat on when they should have their gfx matrix convention
> hat on. If we find a use for it, we can remove those asserts, but right now
> I think it's wise to keep them.

All right.  r+ with the above then.
Comment 19 Cameron McCormack (:heycam) (away Jun 25 – Jul 10) 2012-02-16 23:12:37 PST
Comment on attachment 596659 [details] [diff] [review]
part 3 - fix getBBox and getCTM on foreignObject

(In reply to Jonathan Watt [:jwatt] from comment #17)
> It's there because nsSVGUtils::GetCTM calls *itself*. When it's passed
> something other than eAll, we want to make sure that as it looks up the
> parent chain the non-eAll is converted to eAll for the recursing calls,
> otherwise we'd be skipping transforms on each element along the parent
> chain. I'll add a comment.

I guess I'd prefer either a boolean argument to GetCTM to handle this (where the initial caller passes in false and the recursive call passes in true), or even better splitting it up into a public/private function to prevent callers from passing in explicit values for the boolean.  r+ with either of those.

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