| Summary: | SVG fails all 3 of Hixie's CSS intrinsic sizing tests | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Eric Seidel (no email) <eric> | ||||||
| Component: | SVG | Assignee: | Nikolas Zimmermann <zimmermann> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | jeffschiller, krit, rniwa, rwlbuis, webkit | ||||||
| Priority: | P2 | Keywords: | NeedsReduction | ||||||
| Version: | 523.x (Safari 3) | ||||||||
| Hardware: | Mac | ||||||||
| OS: | OS X 10.4 | ||||||||
| URL: | http://hixie.ch/tests/adhoc/css/box/replaced/intrinsic/svg/ | ||||||||
| Bug Depends on: | 15849 | ||||||||
| Bug Blocks: | |||||||||
| Attachments: |
|
||||||||
|
Description
Eric Seidel (no email)
2007-10-12 01:42:17 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%. (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. 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. Created attachment 95361 [details]
Patch
50 lines of code changed, but a dozen of new testcases, hence a 300k patch
CC'ing Rob & Dirk. 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. (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. Created attachment 95364 [details]
Patch v2
Adress Robs comments.
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? (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. (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 (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. :) > 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 on attachment 95364 [details]
Patch v2
Wait. computerIntrinsicRatioInformation does have side-effects on that bool! So Rob's original code-sense is correct.
(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 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 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. (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. (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. Landed in r87779. 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] |