Bug 15473 – SVG fails all 3 of Hixie's CSS intrinsic sizing tests

• Component: SVG
• Status: RESOLVED FIXED

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

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%.

[David Kilzer (:ddkilzer)]

(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.

[Eric Seidel (no email)]

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.

[Robert Blaut]

It looks like duplicate of bug 5793

[Nikolas Zimmermann]

Created attachment 95361 [details]
Patch

50 lines of code changed, but a dozen of new testcases, hence a 300k patch

[Nikolas Zimmermann]

CC'ing Rob & Dirk.

[Rob Buis]

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.

[Nikolas Zimmermann]

(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.

[Nikolas Zimmermann]

Created attachment 95364 [details]
Patch v2

Adress Robs comments.

[Eric Seidel (no email)]

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?

[Rob Buis]

(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.

[Nikolas Zimmermann]

(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

[Eric Seidel (no email)]

(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. :)

[Rob Buis]

> 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.

[Eric Seidel (no email)]

Comment on attachment 95364 [details]
Patch v2

Wait.  computerIntrinsicRatioInformation does have side-effects on that bool!  So Rob's original code-sense is correct.

[Eric Seidel (no email)]

(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.

[Rob Buis]

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.

[Eric Seidel (no email)]

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.

[Nikolas Zimmermann]

(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.

[Nikolas Zimmermann]

(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.

[Nikolas Zimmermann]

Landed in r87779.

[Ryosuke Niwa]

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]