Bug 11015 – SVG handles em units incorrectly

• Component: SVG
• Status: RESOLVED FIXED

[Eric Seidel (no email)]

SVG handles em units incorrectly

http://hixie.ch/tests/adhoc/svg/data-types/002.xhtml

[Rob Buis]

Created attachment 10794 [details]
First attempt

This should fix the problem but also produce nicer code in the end. Thanks to MacDome
for hints.
Cheers,

Rob.

[Eric Seidel (no email)]

Created attachment 10795 [details]
dynamic update testcase

I'm not sure this test case is 100% valid.  Both FF and Opera fail (the same way).

[Eric Seidel (no email)]

Comment on attachment 10794 [details]
First attempt

Looks great!

You'll need to add a test case to make sure that dynamic updates are still working properly after your changes.

Also, now that I think about it more, calcViewport should not take an argument, but rather should use element().

You probably don't need to call calcViewport unless selfNeedsLayout() is true.

Fix those, then I'll r+ it.

[Rob Buis]

Created attachment 10796 [details]
Now with testcases

Incorporated Hixie's test (plus preserveAspectRatio fix) and Eric's test.
Cheers,

Rob.

[Eric Seidel (no email)]

Comment on attachment 10796 [details]
Now with testcases

Hum...  Did you test to see if each of these was needed?

     if (attr->name() == SVGNames::xAttr) {
         xBaseValue()->setValueAsString(value);
+        if (renderer())
+            renderer()->setNeedsLayout(true);

Also, that's the wrong place to put that.  Those invalidations should go in attributeChange(attributeName) instead of parseMappedAttribute

Otherwise the patch looks great!

[Rob Buis]

Created attachment 10797 [details]
Improved patch

This one uses attributeChanged.
Cheers,

Rob.

[Eric Seidel (no email)]

Comment on attachment 10797 [details]
Improved patch

No need for these extra { }:
+    {
+        if (renderer())
+            renderer()->setNeedsLayout(true);
+    }

+    SVGElement::attributeChanged(attr, preserveDecls);

should call the direct parent, not skip parents in the chain.  That will cause weird bugs later (when we start to depend on SVGTransformableElement (or similar) doing the proper attributeChanged actions.

Resolve those two issues and land away!

r=me

[Eric Seidel (no email)]

Comment on attachment 10795 [details]
dynamic update testcase

this test wasn't quite right, marking obsolete.

[Rob Buis]

Landed in 16601.