Bug 11015

Summary:

SVG handles em units incorrectly

Product:

WebKit

Reporter:

Eric Seidel (no email) <eric>

Component:

SVG

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

Keywords:

HasReduction

Priority:

Version:

420+

Hardware:

Mac

OS:

OS X 10.4

URL:

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

Bug Depends on:

Bug Blocks:

6018

Attachments:

Description	Flags
First attempt	eric: review-
dynamic update testcase	none
Now with testcases	eric: review-
Improved patch	eric: review+

Eric Seidel (no email)

Reported 2006-09-24 18:57:12 PDT

SVG handles em units incorrectly http://hixie.ch/tests/adhoc/svg/data-types/002.xhtml

Attachments
First attempt (6.41 KB, patch) 2006-09-27 00:49 PDT, Rob Buis	eric: review-	Details Formatted Diff Diff
dynamic update testcase (417 bytes, image/svg+xml) 2006-09-27 01:06 PDT, Eric Seidel (no email)	no flags	Details
Now with testcases (31.60 KB, patch) 2006-09-27 02:53 PDT, Rob Buis	eric: review-	Details Formatted Diff Diff
Improved patch (31.72 KB, patch) 2006-09-27 06:01 PDT, Rob Buis	eric: review+	Details Formatted Diff Diff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.

Rob Buis

Comment 1 2006-09-27 00:49:04 PDT

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)

Comment 2 2006-09-27 01:06:17 PDT

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 3 2006-09-27 01:09:11 PDT

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

Comment 4 2006-09-27 02:53:09 PDT

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 5 2006-09-27 03:40:47 PDT

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

Comment 6 2006-09-27 06:01:13 PDT

Created attachment 10797 [details] Improved patch This one uses attributeChanged. Cheers, Rob.

Eric Seidel (no email)

Comment 7 2006-09-27 12:21:22 PDT

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 8 2006-09-27 13:31:20 PDT

Comment on attachment 10795 [details] dynamic update testcase this test wasn't quite right, marking obsolete.

Rob Buis

Comment 9 2006-09-27 13:40:11 PDT

Landed in 16601.

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