863994 - transform attribute on <svg> elements does not work

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Robert Longson [:longsonr]]

Attachment: testcase

Comment 1

[Holger Jeromin]

Attachment: Empty or useless [rotate(0,100,200)] transform renders wrong

another problem with transform on SVG Elements

Comment 2

[Robert Longson [:longsonr]]

Attachment: patch

Now that <svg> elements inherit from SVGTransformable we can implement the fragment identifier functionality on top of that instead of rolling our own.

Comment 3

[Daniel Holbert [:dholbert]]

Comment on attachment 743512 [details] [diff] [review]
patch

>+++ b/content/svg/content/src/SVGSVGElement.cpp
> /* virtual */ gfxMatrix
> SVGSVGElement::PrependLocalTransformsTo(const gfxMatrix &aMatrix,
>                                         TransformTypes aWhich) const
> {
[...]
>+  // 'transform' attribute:
>+  gfxMatrix fromUserSpace =
>+    SVGGraphicsElement::PrependLocalTransformsTo(aMatrix, aWhich);
>+  if (aWhich == eUserSpaceToParent) {
>+    return fromUserSpace;
>+  }
>+
>   if (IsInner()) {


s/SVGGraphicsElement/SVGSVGElementBase/

Also: The PrependLocalTransformsTo call ends applying the <animateMotion> transform:
 https://mxr.mozilla.org/mozilla-central/source/content/svg/content/src/SVGTransformableElement.cpp#108
..which it looks like we currently skip for <svg> elements (until your patch here is applied).  So presumably <animateMotion> on an <svg> element didn't used to have any effect, but now will?  (I'm not sure whether it'll have a *visible* effect on outer-SVG, but it should have a visible effect on inner-SVG, at least).  Per spec, it looks like it *is* supposed to apply:  http://www.w3.org/TR/SVG11/animate.html#AnimationAttributesAndProperties  So assuming there's a behavior-change, the new behavior is probably correct.

So: if this does change that behavior, it'd be worth including a testcase for that (animateMotion on <svg> elements, inner and outer), testing the behavior-change (if any) to ensure that we don't regress the newly-correct behavior.

>+// Callback function, for freeing uint64_t values stored in property table
>+static void
>+ReleaseTransformPropertyValue(void*    aObject,       /* unused */
>+                              nsIAtom* aPropertyName, /* unused */
>+                              void*    aPropertyValue,
>+                              void*    aData          /* unused */)
>+{
>+  SVGTransformList* valPtr =
>+    static_cast<SVGTransformList*>(aPropertyValue);
>+  delete valPtr;

s/uint64_t/SVGTransformList/ in that comment

(Looks like we've got several copies of that incorrect comment in this file, actually -- maybe fix the other ones while you're at it?)

>+const SVGTransformList*
>+SVGSVGElement::GetTransformProperty() const
>+{
>+  void* valPtr = GetProperty(nsGkAtoms::transform);
>+  if (valPtr) {
>+    return reinterpret_cast<SVGTransformList*>(valPtr);
>+  }
>+  return nullptr;
>+}

Use static_cast (not reinterpret_cast)

[still reading, more comments coming]

Comment 4

[Robert Longson [:longsonr]]

animateMotion should be applied on <svg> elements even in SVG 1.1 per http://www.w3.org/TR/SVG/animate.html#AnimationAttributesAndProperties

Comment 5

[Robert Longson [:longsonr]]

I'll add some tests

Comment 6

[Daniel Holbert [:dholbert]]

Comment on attachment 743512 [details] [diff] [review]
patch

>diff --git a/content/svg/content/src/SVGFragmentIdentifier.cpp b/content/svg/content/src/SVGFragmentIdentifier.cpp
>+void 
>+SVGFragmentIdentifier::RestoreOldTransform(dom::SVGSVGElement *root)
>+{
>+  const SVGTransformList *oldTransformPtr = root->GetTransformProperty();
>+  if (oldTransformPtr) {
>+    root->GetAnimatedTransformList(nsSVGElement::DO_ALLOCATE)->SetBaseValue(*oldTransformPtr);
>+  } else {
>+    nsSVGAnimatedTransformList* transformList = root->GetAnimatedTransformList();
>+    }

nit: in "SVGTransformList *oldTransformPtr" on the first line of the function there, shift the * to the left, for consistency w/ surrounding code.

>diff --git a/layout/reftests/svg/fragmentIdentifier-01.xhtml b/layout/reftests/svg/fragmentIdentifier-01.xhtml
>--- a/layout/reftests/svg/fragmentIdentifier-01.xhtml
>+++ b/layout/reftests/svg/fragmentIdentifier-01.xhtml
>@@ -1,13 +1,18 @@
> <html xmlns="http://www.w3.org/1999/xhtml" class="reftest-wait">
>   <head>
>     <title>Testcases for SVG fragment identifiers</title>
>   </head>
>   <body style="background-color: lime;">
>+    <style>
>+       object {
>+         overflow: hidden;
>+       }
>+    </style>

So presumably this change is because this patch tweaked the positioning/sizing of some <svg> elements in such a way that they now overflow their <object> and trigger scrollbars, yes?  I assume you double-checked that this rendering-change was a sensible/correct one, and therefore worthy of suppressing with overflow:hidden?

Ideally, it might be nice to just add overflow:hidden to the (few?) affected object elements rather than to all of them, to keep this targeted (and so that if one of the not-yet-overflowing <object> elements unexpectedly starts overflowing in the future, we'll notice).  Not a huge deal, though.

r=me with the above comments addressed/responded to.

Thanks!

Comment 7

[Robert Longson [:longsonr]]

https://tbpl.mozilla.org/?tree=Try&rev=4d547402bf9a

Comment 8

[Robert Longson [:longsonr]]

The mochitest failure is another bug that this patch fixes. See bug 660216 which fixed foreignObject. I'll fix the mochitest when I land this.

Comment 9

[Robert Longson [:longsonr]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b8b048e24dcd
https://hg.mozilla.org/integration/mozilla-inbound/rev/745a6846cf43

Comment 10

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/b8b048e24dcd
https://hg.mozilla.org/mozilla-central/rev/745a6846cf43