The default bug view has changed. See this FAQ.

getBBox and getCTM on foreignObject should/should not include 'x'/'y' offsets respectively

RESOLVED FIXED in mozilla13

Status

()

Core
SVG
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: David Rivron, Assigned: jwatt)

Tracking

({testcase})

Trunk
mozilla13
testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 1 obsolete attachment)

(Reporter)

Description

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

Comment 1

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

Comment 2

6 years ago
Created attachment 535623 [details]
foreignObject getBBox getCTM testcase
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.
Keywords: testcase
OS: Mac OS X → All
Version: unspecified → Trunk

Comment 4

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

Comment 5

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

Comment 6

6 years ago
Created attachment 536038 [details]
rect getBBox getCTM testcase

Comment 7

6 years ago
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
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Comment 8

5 years ago
(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.
Assignee: nobody → jwatt
Blocks: 614732
Status: NEW → ASSIGNED
Hardware: x86 → All
(Assignee)

Updated

5 years ago
Summary: Incorrect BBox and CTM on SVG foreignObject → getBBox and getCTM on foreignObject should not include 'x'/'y' offsets
(Assignee)

Comment 9

5 years ago
Created attachment 596515 [details] [diff] [review]
patch
Attachment #596515 - Flags: review?(cam)
(Assignee)

Updated

5 years ago
Summary: getBBox and getCTM on foreignObject should not include 'x'/'y' offsets → getBBox and getCTM on foreignObject should/should not include 'x'/'y' offsets respectively
(Assignee)

Comment 10

5 years ago
Actually, let's split this into three logically self contained patches.
(Assignee)

Comment 11

5 years ago
Created attachment 596657 [details] [diff] [review]
part 1 - rename PrependLocalTransformTo to PrependLocalTransformsTo
Attachment #596657 - Flags: review?(cam)
(Assignee)

Comment 12

5 years ago
Created attachment 596658 [details] [diff] [review]
part 2 - Allow PrependLocalTransformsTo callers to specify which transforms they want
Attachment #596658 - Flags: review?(cam)
(Assignee)

Comment 13

5 years ago
Created attachment 596659 [details] [diff] [review]
part 3 - fix getBBox and getCTM on foreignObject
Attachment #596659 - Flags: review?(cam)
(Assignee)

Updated

5 years ago
Attachment #596657 - Attachment description: patch - rename PrependLocalTransformTo to PrependLocalTransformsTo → part 1 - rename PrependLocalTransformTo to PrependLocalTransformsTo
(Assignee)

Updated

5 years ago
Attachment #596515 - Attachment is obsolete: true
Attachment #596515 - Flags: review?(cam)
Attachment #596657 - Flags: review?(cam) → review+
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 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.
(Assignee)

Comment 16

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

Comment 17

5 years ago
(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.
(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.
Attachment #596658 - Flags: review?(cam) → review+
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.
Attachment #596659 - Flags: review?(cam) → review+
https://hg.mozilla.org/mozilla-central/rev/e49393a599fa
https://hg.mozilla.org/mozilla-central/rev/46f96343c85d
https://hg.mozilla.org/mozilla-central/rev/b3a3372fecae
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13

Updated

5 years ago
Depends on: 762411

Updated

5 years ago
Depends on: 780169
You need to log in before you can comment on or make changes to this bug.