Bug 15473 - SVG fails all 3 of Hixie's CSS intrinsic sizing tests
Summary: SVG fails all 3 of Hixie's CSS intrinsic sizing tests
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 523.x (Safari 3)
Hardware: Macintosh OS X 10.4
: P2 Normal
Assignee: Nikolas Zimmermann
URL: http://hixie.ch/tests/adhoc/css/box/r...
Keywords: NeedsReduction
Depends on: 15849
Blocks:
  Show dependency treegraph
 
Reported: 2007-10-12 01:42 PDT by Eric Seidel
Modified: 2011-06-10 22:37 PDT (History)
5 users (show)

See Also:


Attachments
Patch (304.10 KB, patch)
2011-05-30 12:39 PDT, Nikolas Zimmermann
rwlbuis: review-
Details | Formatted Diff | Diff
Patch v2 (305.64 KB, patch)
2011-05-30 13:17 PDT, Nikolas Zimmermann
rwlbuis: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Seidel 2007-10-12 01:42:17 PDT
SVG fails all 3 of Hixie's CSS intrinsic sizing tests

http://hixie.ch/tests/adhoc/css/box/replaced/intrinsic/svg/001.html
http://hixie.ch/tests/adhoc/css/box/replaced/intrinsic/svg/002.html
http://hixie.ch/tests/adhoc/css/box/replaced/intrinsic/svg/003.html

If necessary this can be split into multiple bugs.
Comment 1 Eric Seidel 2007-10-15 01:16:19 PDT
In order for this to work, RenderReplaced will need to understand having a non-integer intrinsic size.  Currently the default intrinsic size is 300x150.  These tests depend on <object> being able to grab the size off the <svg> and setting its own intrinsic size to 100%x100%.
Comment 2 David Kilzer (:ddkilzer) 2007-10-15 20:15:22 PDT
(In reply to comment #1)
> In order for this to work, RenderReplaced will need to understand having a
> non-integer intrinsic size.  Currently the default intrinsic size is 300x150. 
> These tests depend on <object> being able to grab the size off the <svg> and
> setting its own intrinsic size to 100%x100%.

The fix for Bug 15359 may cause a change in behavior for these tests.

Comment 3 Eric Seidel 2007-11-05 18:37:08 PST
Wow.  So CSS 2.1 definitely spec's this, and we just simply don't implement it:
http://www.w3.org/TR/CSS21/visudet.html#inline-replaced-width

This will require changes to RenderBox to support non-fixed intrinsic sizes.
Comment 4 Robert Blaut 2008-03-19 06:02:00 PDT
It looks like duplicate of bug 5793
Comment 5 Nikolas Zimmermann 2011-05-30 12:39:26 PDT
Created attachment 95361 [details]
Patch

50 lines of code changed, but a dozen of new testcases, hence a 300k patch
Comment 6 Nikolas Zimmermann 2011-05-30 12:40:41 PDT
CC'ing Rob & Dirk.
Comment 7 Rob Buis 2011-05-30 12:59:54 PDT
Comment on attachment 95361 [details]
Patch

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

The code and tests look fine to me. I had some minor gripes, but I think it is close to being ready :)

> Source/WebCore/ChangeLog:13
> +        The *on-target-svg-absolute.xhtml tests look equal to WebKit/FF but Opera fails them, likely a relict of the differnt

s/differnt/different

> Source/WebCore/rendering/svg/RenderSVGRoot.h:42
> +    FloatSize computeIntrinsicRatio(bool& isPercentageIntrinsicSize) const;

I had to look at the header to see that the bool was an in-out param. Can you do anything to make that more clear? Now it seems like a sort of a getter that also happens to set a passed in bool as side-effect. Maybe use Info in the name and pass in both data members that need to be calculated?

> LayoutTests/ChangeLog:6
> +        https://bugs.webkit.org/show_bug.cgi?id=15473

I think this needs more wording. Maybe you can repeat the paragraph about the test in the WebCore/ChangeLog and explain a bit detailed what the tests do.
Comment 8 Nikolas Zimmermann 2011-05-30 13:15:43 PDT
(In reply to comment #7)
> (From update of attachment 95361 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=95361&action=review
> 
> The code and tests look fine to me. I had some minor gripes, but I think it is close to being ready :)
Great!

> 
> > Source/WebCore/ChangeLog:13
> > +        The *on-target-svg-absolute.xhtml tests look equal to WebKit/FF but Opera fails them, likely a relict of the differnt
> 
> s/differnt/different
Fixed.

> 
> > Source/WebCore/rendering/svg/RenderSVGRoot.h:42
> > +    FloatSize computeIntrinsicRatio(bool& isPercentageIntrinsicSize) const;
> 
> I had to look at the header to see that the bool was an in-out param. Can you do anything to make that more clear? Now it seems like a sort of a getter that also happens to set a passed in bool as side-effect. Maybe use Info in the name and pass in both data members that need to be calculated?
Yeah, I named it "void computeIntrinsicRatioInformation(FloatSize& intrinsicRatio, bool& isPercentageIntrinsicSize) const".

> 
> > LayoutTests/ChangeLog:6
> > +        https://bugs.webkit.org/show_bug.cgi?id=15473
> 
> I think this needs more wording. Maybe you can repeat the paragraph about the test in the WebCore/ChangeLog and explain a bit detailed what the tests do.
Ok. Too tired too explain the test, about to go to bed. I'll promise to explain them tomorrow, maybe you can still r+ w/o the comments now, I'd just repeat in short what these new tests contain.
Comment 9 Nikolas Zimmermann 2011-05-30 13:17:59 PDT
Created attachment 95364 [details]
Patch v2

Adress Robs comments.
Comment 10 Eric Seidel 2011-05-30 13:20:14 PDT
Comment on attachment 95364 [details]
Patch v2

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

> Source/WebCore/rendering/RenderPart.cpp:142
> +    bool isPercentageIntrinsicSize = false;

Enums are always beter than bools, but I am glad that you named to bool nicely. :)

> Source/WebCore/rendering/RenderPart.cpp:171
> +                    return static_cast<int>(ceilf(containingBlock->width() * intrinsicRatio.width() / 100));

We seem to do this repeatedly.  I wonder if we shouldn't have a roundUp() function.

> Source/WebCore/rendering/RenderPart.cpp:197
> +    contentRenderer->computeIntrinsicRatioInformation(intrinsicRatio, isPercentageIntrinsicSize);

Wouldn't we want to just return the FloatSize?
Comment 11 Rob Buis 2011-05-30 14:35:16 PDT
(In reply to comment #10)
> > Source/WebCore/rendering/RenderPart.cpp:197
> > +    contentRenderer->computeIntrinsicRatioInformation(intrinsicRatio, isPercentageIntrinsicSize);
> 
> Wouldn't we want to just return the FloatSize?

Niko did that on my suggestion, its in the thread. From the original version it was not clear to me that the flag was writeable as well, making it look like a side-effect. So this was my stab at fixing that, maybe it also works if the & can be introduced in the call (like for the bOk params we have/had). I don't think it should hold up the landing much. Let me know if you have an idea to get rid of the side-effect feeling...
Cheers,

Rob.
Comment 12 Nikolas Zimmermann 2011-05-30 23:23:37 PDT
(In reply to comment #10)
> (From update of attachment 95364 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=95364&action=review
> 
> > Source/WebCore/rendering/RenderPart.cpp:142
> > +    bool isPercentageIntrinsicSize = false;
> 
> Enums are always beter than bools, but I am glad that you named to bool nicely. :)
Phew :-)

> 
> > Source/WebCore/rendering/RenderPart.cpp:171
> > +                    return static_cast<int>(ceilf(containingBlock->width() * intrinsicRatio.width() / 100));
> 
> We seem to do this repeatedly.  I wonder if we shouldn't have a roundUp() function.
This seems certainly out of scope for this patch, we're doing the ceilf dance in tons of places.

> 
> > Source/WebCore/rendering/RenderPart.cpp:197
> > +    contentRenderer->computeIntrinsicRatioInformation(intrinsicRatio, isPercentageIntrinsicSize);
> 
> Wouldn't we want to just return the FloatSize?
Heh, I had that in the beginnning, Rob suggested to change that..

Cheers,
Niko
Comment 13 Eric Seidel 2011-05-31 08:16:39 PDT
(In reply to comment #11)
> (In reply to comment #10)
> > > Source/WebCore/rendering/RenderPart.cpp:197
> > > +    contentRenderer->computeIntrinsicRatioInformation(intrinsicRatio, isPercentageIntrinsicSize);
> > 
> > Wouldn't we want to just return the FloatSize?
> 
> Niko did that on my suggestion, its in the thread. From the original version it was not clear to me that the flag was writeable as well, making it look like a side-effect. So this was my stab at fixing that, maybe it also works if the & can be introduced in the call (like for the bOk params we have/had). I don't think it should hold up the landing much. Let me know if you have an idea to get rid of the side-effect feeling...

I would argue the side-effect feeling is caused by not using an enum for hte bool (thus needing a nicely named variable to describe what the bool is doing!)

intrinsicRatio = contentRenderer->computeIntrinsicRatio(PercentageIntrinsicSize);

Would not cause the side-effect feeling I assume. :)
Comment 14 Rob Buis 2011-05-31 08:32:07 PDT
> I would argue the side-effect feeling is caused by not using an enum for hte bool (thus needing a nicely named variable to describe what the bool is doing!)
> 
> intrinsicRatio = contentRenderer->computeIntrinsicRatio(PercentageIntrinsicSize);
> 
> Would not cause the side-effect feeling I assume. :)

Yes, that would address my concern, thanks.
Cheers,

Rob.
Comment 15 Eric Seidel 2011-05-31 08:34:45 PDT
Comment on attachment 95364 [details]
Patch v2

Wait.  computerIntrinsicRatioInformation does have side-effects on that bool!  So Rob's original code-sense is correct.
Comment 16 Eric Seidel 2011-05-31 08:35:15 PDT
(In reply to comment #15)
> (From update of attachment 95364 [details])
> Wait.  computerIntrinsicRatioInformation does have side-effects on that bool!  So Rob's original code-sense is correct.

Aka, the enum stuff I suggested is not helpful since you're trying to return that one bool as well.
Comment 17 Rob Buis 2011-05-31 08:35:29 PDT
Comment on attachment 95364 [details]
Patch v2

I give the r+ assuming you update the comments. You can forget my computeIntrinsicRatio suggestion, that is not really needed, but you could have a look if Eric's enum suggestion makes things more clearer before landing.
Comment 18 Eric Seidel 2011-05-31 08:38:06 PDT
Comment on attachment 95364 [details]
Patch v2

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

Seems fine.  There may be other cleaner ways to write that function as discussed above.  Please consider before landing.  Thansk!

> Source/WebCore/rendering/svg/RenderSVGRoot.cpp:65
> +void RenderSVGRoot::computeIntrinsicRatioInformation(FloatSize& intrinsicRatio, bool& isPercentageIntrinsicSize) const

One could also have this return a new struct.  We'll see how many places this pairing is needed.  It almost seems like you want a Length pair.  I wonder if you want this to return two length's by refernece.  Donno.
Comment 19 Nikolas Zimmermann 2011-05-31 22:41:25 PDT
(In reply to comment #13)
> (In reply to comment #11)
> > (In reply to comment #10)
> > > > Source/WebCore/rendering/RenderPart.cpp:197
> > > > +    contentRenderer->computeIntrinsicRatioInformation(intrinsicRatio, isPercentageIntrinsicSize);
> > > 
> > > Wouldn't we want to just return the FloatSize?
> > 
> > Niko did that on my suggestion, its in the thread. From the original version it was not clear to me that the flag was writeable as well, making it look like a side-effect. So this was my stab at fixing that, maybe it also works if the & can be introduced in the call (like for the bOk params we have/had). I don't think it should hold up the landing much. Let me know if you have an idea to get rid of the side-effect feeling...
> 
> I would argue the side-effect feeling is caused by not using an enum for hte bool (thus needing a nicely named variable to describe what the bool is doing!)
> 
> intrinsicRatio = contentRenderer->computeIntrinsicRatio(PercentageIntrinsicSize);
Ermm, now I'm confused.
The bool I'm using is a _reference_. Did you overlook that? How would using an enum help at all?
Do you want me to pass in an enum& reference???

The caller of computeIntriniscRatio just wants to know wheter percentage intrinsic sizes are used or not.
Comment 20 Nikolas Zimmermann 2011-05-31 22:42:35 PDT
(In reply to comment #16)
> (In reply to comment #15)
> > (From update of attachment 95364 [details] [details])
> > Wait.  computerIntrinsicRatioInformation does have side-effects on that bool!  So Rob's original code-sense is correct.
> 
> Aka, the enum stuff I suggested is not helpful since you're trying to return that one bool as well.

Phew sorry for my post, just seeing that you saw it :-)

(In reply to comment #18)
> (From update of attachment 95364 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=95364&action=review
> 
> Seems fine.  There may be other cleaner ways to write that function as discussed above.  Please consider before landing.  Thansk!
> 
> > Source/WebCore/rendering/svg/RenderSVGRoot.cpp:65
> > +void RenderSVGRoot::computeIntrinsicRatioInformation(FloatSize& intrinsicRatio, bool& isPercentageIntrinsicSize) const
> 
> One could also have this return a new struct.  We'll see how many places this pairing is needed.  It almost seems like you want a Length pair.  I wonder if you want this to return two length's by refernece.  Donno.

Thank.
Comment 21 Nikolas Zimmermann 2011-06-01 00:31:43 PDT
Landed in r87779.
Comment 22 Ryosuke Niwa 2011-06-10 22:37:46 PDT
So a bunch of tests added by this patch are failing on Windows:
http://build.webkit.org/results/Windows%207%20Release%20(Tests)/r88594%20(13663)/results.html

They're mostly 1px difference but I'm not sure if that's okay for these tests:
-              RenderSVGPath {rect} at (6,5) size 113x102 [fill={[type=SOLID] [color=#008000]}] [x=6.25] [y=5.62] [width=112.50] [height=101.25]
+              RenderSVGPath {rect} at (6,5) size 113x102 [fill={[type=SOLID] [color=#008000]}] [x=6.25] [y=5.63] [width=112.50] [height=101.25]