Bug 76447 – Differentiate between SVG/CSS width/height attributes/properties

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Nikolas Zimmermann]

Differentiate between SVG/CSS width/height attributes/properties.
We currently map SVG width/height attributes to CSS properties, which is a hack, and we have quite a lot of code in rendering/ now that works around this fact.
We're violating several pieces of both CSS 2.1 and SVG 1.1 2nd edition.

I have a complete fix for this, and will divide it into smaller patches.

[Nikolas Zimmermann]

Created attachment 122913 [details]
Patch v1

[Nikolas Zimmermann]

Created attachment 122931 [details]
Patch v2

[WebKit Review Bot]

Comment on attachment 122931 [details]
Patch v2

Attachment 122931 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11282369

New failing tests:
css1/text_properties/vertical_align.html
css2.1/20110323/background-intrinsic-006.htm
svg/custom/object-sizing-width-50p-height-75p-on-target-svg-absolute.xhtml
media/video-aspect-ratio.html
svg/zoom/page/zoom-svg-through-object-with-auto-size.html
css1/box_properties/height.html
tables/mozilla_expected_failures/bugs/97619.html
svg/custom/object-sizing-width-50p-height-75p-on-target-svg.xhtml
svg/custom/object-sizing-width-50p-on-target-svg-absolute.xhtml
fast/writing-mode/block-level-images.html
tables/mozilla/bugs/bug14929.html
tables/mozilla/bugs/bug86708.html
svg/custom/object-sizing-width-75p-height-50p-on-target-svg-absolute.xhtml
svg/zoom/page/zoom-svg-through-object-with-absolute-size-2.xhtml
tables/mozilla/bugs/bug101674.html
fast/replaced/table-percent-height.html
svg/custom/object-sizing-width-75p-height-50p-on-target-svg.xhtml
css1/box_properties/clear.html
css1/formatting_model/floating_elements.html
css1/box_properties/float.html
svg/custom/object-sizing-width-50p-height-50p-on-target-svg-absolute.xhtml
css2.1/t1004-c5524-width-00-b-g.html

[Nikolas Zimmermann]

(In reply to comment #3)
> (From update of attachment 122931 [details])
> Attachment 122931 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/11282369
> 
> New failing tests:
> css1/text_properties/vertical_align.html
> css2.1/20110323/background-intrinsic-006.htm
> svg/custom/object-sizing-width-50p-height-75p-on-target-svg-absolute.xhtml
> media/video-aspect-ratio.html
> svg/zoom/page/zoom-svg-through-object-with-auto-size.html
> css1/box_properties/height.html
> tables/mozilla_expected_failures/bugs/97619.html
> svg/custom/object-sizing-width-50p-height-75p-on-target-svg.xhtml
> svg/custom/object-sizing-width-50p-on-target-svg-absolute.xhtml
> fast/writing-mode/block-level-images.html
> tables/mozilla/bugs/bug14929.html
> tables/mozilla/bugs/bug86708.html
> svg/custom/object-sizing-width-75p-height-50p-on-target-svg-absolute.xhtml
> svg/zoom/page/zoom-svg-through-object-with-absolute-size-2.xhtml
> tables/mozilla/bugs/bug101674.html
> fast/replaced/table-percent-height.html
> svg/custom/object-sizing-width-75p-height-50p-on-target-svg.xhtml
> css1/box_properties/clear.html
> css1/formatting_model/floating_elements.html
> css1/box_properties/float.html
> svg/custom/object-sizing-width-50p-height-50p-on-target-svg-absolute.xhtml
> css2.1/t1004-c5524-width-00-b-g.html

Hrm, I wonder about changes in non-SVG context :( Not sure whats going on here. I wish there was a way to obtain these results prior to landing :(

[Antti Koivisto]

Comment on attachment 122931 [details]
Patch v2

View in context: https://bugs.webkit.org/attachment.cgi?id=122931&action=review

r=me, with a few nits.

> Source/WebCore/rendering/RenderBoxModelObject.cpp:882
> +                resolvedWidth = static_cast<int>(ceilf(intrinsicWidth.value() * style()->effectiveZoom()));

I would prefer the more compact constructor style cast int(ceilf(intrinsicWidth.value() * style()->effectiveZoom())) here and elsewhere. Not sure what our style policy is for this.

> Source/WebCore/svg/SVGSVGElement.h:79
> +    enum ConsiderCSSMode {
> +        RespectCSSProperties,
> +        IgnoreCSSProperties
>      };

I think it would be better to eliminate this enum...

> Source/WebCore/svg/SVGSVGElement.h:83
> +    Length intrinsicWidth(ConsiderCSSMode = RespectCSSProperties) const;
> +    Length intrinsicHeight(ConsiderCSSMode = RespectCSSProperties) const;

...and just have separate functions for the two cases. The code would read better I think.

[Nikolas Zimmermann]

Committed r105402: <http://trac.webkit.org/changeset/105402>

[Nikolas Zimmermann]

(In reply to comment #5)
> I would prefer the more compact constructor style cast int(ceilf(intrinsicWidth.value() * style()->effectiveZoom())) here and elsewhere. Not sure what our style policy is for this.
I left it as-is per IRC discussion.

> I think it would be better to eliminate this enum...
> ...and just have separate functions for the two cases. The code would read better I think.
Fixed.

Thanks a lot! I'll go watch the Chromium bots, if there's really as much broken as it says..

[Nikolas Zimmermann]

(In reply to comment #7)
Argl, I ran the full set of tests w/o my patch applied in a wrong git branch.
Several text results need rebaselines, I'm going to take care of that now.

[Nikolas Zimmermann]

Created attachment 123292 [details]
Patch v3

[Nikolas Zimmermann]

Uploading a new version, that should have no regressions on Mac left. Awaiting cr-ews results, to find out whether it only has the failing tests that I also had to rebaseline on Mac.

[Nikolas Zimmermann]

Ok I'm happy with the results obtained by the cr-ews bot. I updated platform/chromium/test_expectations.txt and trying to land it a second time.

[Nikolas Zimmermann]

Committed r105513: <http://trac.webkit.org/changeset/105513>

[WebKit Review Bot]

Comment on attachment 123292 [details]
Patch v3

Attachment 123292 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11297113

New failing tests:
svg/zoom/page/zoom-background-images.html
svg/custom/object-sizing-width-50p-height-75p-on-target-svg-absolute.xhtml
svg/custom/object-sizing-width-50p-height-50p-on-target-svg-absolute.xhtml
tables/mozilla/bugs/bug101674.html
tables/mozilla/bugs/bug86708.html
svg/custom/object-sizing-width-75p-height-50p-on-target-svg.xhtml
svg/custom/object-sizing-width-75p-height-50p-on-target-svg-absolute.xhtml
tables/mozilla_expected_failures/bugs/97619.html
svg/custom/object-sizing-width-50p-height-75p-on-target-svg.xhtml
svg/custom/object-sizing-width-50p-on-target-svg-absolute.xhtml

[Nikolas Zimmermann]

Landed Gtk rebaselines in r105516.

[Simon Fraser (smfr)]

svg/wicd/rightsizing-grid.xhtml is also broken because of this.

[Nikolas Zimmermann]

(In reply to comment #15)
> svg/wicd/rightsizing-grid.xhtml is also broken because of this.
Broken, as in crash? Looks wrong? Just tried again with ToT, and it passes fine on Lion using tolerance 0.

[Simon Fraser (smfr)]

SL bot shows it as flakey:
http://build.webkit.org/results/SnowLeopard%20Intel%20Release%20(Tests)/r105576%20(36564)/results.html

[Nikolas Zimmermann]

(In reply to comment #17)
> SL bot shows it as flakey:
> http://build.webkit.org/results/SnowLeopard%20Intel%20Release%20(Tests)/r105576%20(36564)/results.html

I can reproduce it as well locally, I'll investigate in the morning, it's on top of my list.

[Nikolas Zimmermann]

(In reply to comment #18)
> (In reply to comment #17)
> > SL bot shows it as flakey:
Closing bug, now that 77099 landed, which fixes the flakiness. Thanks for your patience.

[Stephen Chenney]

Committed r113436: <http://trac.webkit.org/changeset/113436>