Bug 76447 - Differentiate between SVG/CSS width/height attributes/properties
Summary: Differentiate between SVG/CSS width/height attributes/properties
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nikolas Zimmermann
URL:
Keywords:
Depends on: 76446 76623 77099
Blocks:
  Show dependency treegraph
 
Reported: 2012-01-17 06:06 PST by Nikolas Zimmermann
Modified: 2012-04-06 08:09 PDT (History)
6 users (show)

See Also:


Attachments
Patch v1 (192.93 KB, patch)
2012-01-18 06:16 PST, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Patch v2 (193.30 KB, patch)
2012-01-18 07:41 PST, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Patch v3 (644.92 KB, patch)
2012-01-20 04:56 PST, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nikolas Zimmermann 2012-01-17 06:06:25 PST
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.
Comment 1 Nikolas Zimmermann 2012-01-18 06:16:42 PST
Created attachment 122913 [details]
Patch v1
Comment 2 Nikolas Zimmermann 2012-01-18 07:41:36 PST
Created attachment 122931 [details]
Patch v2
Comment 3 WebKit Review Bot 2012-01-18 14:55:24 PST
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
Comment 4 Nikolas Zimmermann 2012-01-19 03:51:40 PST
(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 :(
Comment 5 Antti Koivisto 2012-01-19 04:03:22 PST
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.
Comment 6 Nikolas Zimmermann 2012-01-19 04:41:45 PST
Committed r105402: <http://trac.webkit.org/changeset/105402>
Comment 7 Nikolas Zimmermann 2012-01-19 04:43:20 PST
(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..
Comment 8 Nikolas Zimmermann 2012-01-19 04:47:40 PST
(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.
Comment 9 Nikolas Zimmermann 2012-01-20 04:56:49 PST
Created attachment 123292 [details]
Patch v3
Comment 10 Nikolas Zimmermann 2012-01-20 04:57:31 PST
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.
Comment 11 Nikolas Zimmermann 2012-01-20 07:27:55 PST
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.
Comment 12 Nikolas Zimmermann 2012-01-20 07:31:00 PST
Committed r105513: <http://trac.webkit.org/changeset/105513>
Comment 13 WebKit Review Bot 2012-01-20 07:33:52 PST
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
Comment 14 Nikolas Zimmermann 2012-01-20 07:56:50 PST
Landed Gtk rebaselines in r105516.
Comment 15 Simon Fraser (smfr) 2012-01-20 11:49:37 PST
svg/wicd/rightsizing-grid.xhtml is also broken because of this.
Comment 16 Nikolas Zimmermann 2012-01-21 00:17:21 PST
(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.
Comment 18 Nikolas Zimmermann 2012-01-23 13:34:27 PST
(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.
Comment 19 Nikolas Zimmermann 2012-01-26 07:06:36 PST
(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.
Comment 20 Stephen Chenney 2012-04-06 08:09:03 PDT
Committed r113436: <http://trac.webkit.org/changeset/113436>