Bug 15849 – CSS 2.1: Support replaced elements with relative intrinsic sizes

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Eric Seidel (no email)]

CSS 2.1: Support replaced elements with relative intrinsic sizes

We need this support for SVGImage and <svg>'s referenced from <object> tags.  It looks like it's specified quite clearly in CSS 2.1, but we just don't support it:

http://www.w3.org/TR/CSS21/visudet.html#inline-replaced-width

[Eric Seidel (no email)]

This causes several bad bugs in our SVG implementation.  Both in SVGImage implementations as well as just normal <object>/<embed> sizing when dealing with SVG content.

[Nikolas Zimmermann]

The patch on bug 10526 fixes this bug, for embedded SVG documents.

[Nikolas Zimmermann]

It's now fixed for SVG documents embedded through object/embed/iframe in r87526.
I'll leave this open as master bug for SVGImage, which is still TODO.

[Nikolas Zimmermann]

Created attachment 96622 [details]
Patch

Code changes including ChangeLogs is about ~25k, the rest are layout tests.
I've added about 203 new tests from css2.1 that will be added in a seperated patch.

Fixes several CSS 2.1 test suite failures, including important ones for embedding SVGs!

[Nikolas Zimmermann]

CC'ing Dave & Simon, here's the patch I promised to Dave on IRC, implementing an important part of "CSS 2.1 Visual formatting model details" related to replaced elements with intrinsic sizes/ratios.

I'd be glad if you could review it. I have a follow-up patch that will replaced RenderImages own size-negotiation code, as described in the ChangeLogs.

[Nikolas Zimmermann]

Created attachment 96624 [details]
Adding 203 CSS 2.1 tests *replaced*/*intrinsic*

Adding all CSS 2.1 tests *replaced*/*intrinsic*, except background-intrinsic-* which still fails, as <img>/background-image (both using SVGImage) has not been unified yet with the new size-negotiation logic.

[WebKit Review Bot]

Comment on attachment 96622 [details]
Patch

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

New failing tests:
tables/mozilla/bugs/bug50695-2.html
http/tests/local/drag-over-remote-content.html
tables/mozilla/bugs/bug131020.html
http/tests/misc/object-embedding-svg-delayed-size-negotiation-2.htm
tables/mozilla/bugs/bug137388-2.html
tables/mozilla/bugs/bug137388-3.html
svg/zoom/page/zoom-replaced-intrinsic-ratio-001.htm
fast/replaced/percent-height-in-anonymous-block.html
fast/replaced/percent-height-in-anonymous-block-in-table.html
tables/mozilla/bugs/bug137388-1.html

[WebKit Review Bot]

Created attachment 96638 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Dave Hyatt]

Comment on attachment 96622 [details]
Patch

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

Just a few comments.

> Source/WebCore/rendering/RenderBox.cpp:668
> +    if (!style())
> +        return false;

Just remove this. style can never be null.

> Source/WebCore/rendering/RenderBox.cpp:672
> +    if (isRenderPart())
> +        return toRenderPart(const_cast<RenderBox*>(this))->embeddedContentBox();

isRenderPart() is virtual, so I think it would be cleaner to just make needsPreferredWidthsRecalculation virtual and subclass it for RenderPart.  Given that padding is almost never a percentage, the isRenderPart check is always being hit anyway, so you're already paying the virtual function invocation cost once. Might as well make the code cleaner then.

> Source/WebCore/rendering/RenderBox.h:123
> +    virtual void computeIntrinsicRatioInformation(FloatSize& /* intrinsicRatio */, bool& /* isPercentageIntrinsicSize */) const { }

Is it right for this to be public?

> Source/WebCore/rendering/RenderReplaced.cpp:192
> +int RenderReplaced::computeIntrinsicWidth(RenderBox* contentRenderer, bool includeMaxWidth) const

You've made a function called "computeIntrinsicWidth" that then invokes "computeReplacedLogicalWidth...".  In a vertical writing mode, "logical width" is actually height. There's an inconsistency here that needs to be dealt with. Either you just misnamed this function, or you intended for it to be truly physical, in which case what it's doing isn't correct for vertical writing modes.

> Source/WebCore/rendering/RenderReplaced.cpp:204
> +int RenderReplaced::computeIntrinsicHeight(RenderBox* contentRenderer) const
> +{

Same comment here as for computeIntrinsicWidth.

[Nikolas Zimmermann]

(In reply to comment #9)
> (From update of attachment 96622 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=96622&action=review
> 
> Just a few comments.
Thanks for the quick review Dave!

> 
> > Source/WebCore/rendering/RenderBox.cpp:668
> > +    if (!style())
> > +        return false;
> 
> Just remove this. style can never be null.
Fixed.

> 
> > Source/WebCore/rendering/RenderBox.cpp:672
> > +    if (isRenderPart())
> > +        return toRenderPart(const_cast<RenderBox*>(this))->embeddedContentBox();
> 
> isRenderPart() is virtual, so I think it would be cleaner to just make needsPreferredWidthsRecalculation virtual and subclass it for RenderPart.  Given that padding is almost never a percentage, the isRenderPart check is always being hit anyway, so you're already paying the virtual function invocation cost once. Might as well make the code cleaner then.
Excellent suggestion.

> 
> > Source/WebCore/rendering/RenderBox.h:123
> > +    virtual void computeIntrinsicRatioInformation(FloatSize& /* intrinsicRatio */, bool& /* isPercentageIntrinsicSize */) const { }
> 
> Is it right for this to be public?
Ah right, a debugging leftover - I made it protected, only RenderReplaced calls it.

> 
> > Source/WebCore/rendering/RenderReplaced.cpp:192
> > +int RenderReplaced::computeIntrinsicWidth(RenderBox* contentRenderer, bool includeMaxWidth) const
> 
> You've made a function called "computeIntrinsicWidth" that then invokes "computeReplacedLogicalWidth...".  In a vertical writing mode, "logical width" is actually height. There's an inconsistency here that needs to be dealt with. Either you just misnamed this function, or you intended for it to be truly physical, in which case what it's doing isn't correct for vertical writing modes.
> 
> > Source/WebCore/rendering/RenderReplaced.cpp:204
> > +int RenderReplaced::computeIntrinsicHeight(RenderBox* contentRenderer) const
> > +{
> 
> Same comment here as for computeIntrinsicWidth.

Both are misnamed, I'll name them computeIntrinsicLogicalWidth/Height.
Going to upload a new patch soon.

[Nikolas Zimmermann]

Ok I can't make the compute* functions protected:

/Users/nzimmermann/Coding/WebKit/Source/WebCore/rendering/RenderBox.h: In member function 'virtual int WebCore::RenderReplaced::computeReplacedLogicalWidth(bool) const':
/Users/nzimmermann/Coding/WebKit/Source/WebCore/rendering/RenderBox.h:414: error: 'virtual void WebCore::RenderBox::computeIntrinsicRatioInformation(WebCore::FloatSize&, bool&) const' is protected
/Users/nzimmermann/Coding/WebKit/Source/WebCore/rendering/RenderReplaced.cpp:226: error: within this context
...

[Nikolas Zimmermann]

Created attachment 96848 [details]
Patch v2

Fix Daves comments.

[WebKit Review Bot]

Comment on attachment 96848 [details]
Patch v2

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

New failing tests:
tables/mozilla/bugs/bug50695-2.html
http/tests/local/drag-over-remote-content.html
tables/mozilla/bugs/bug131020.html
http/tests/misc/object-embedding-svg-delayed-size-negotiation-2.htm
tables/mozilla/bugs/bug137388-2.html
tables/mozilla/bugs/bug137388-3.html
svg/zoom/page/zoom-replaced-intrinsic-ratio-001.htm
fast/replaced/percent-height-in-anonymous-block.html
fast/replaced/percent-height-in-anonymous-block-in-table.html
tables/mozilla/bugs/bug137388-1.html

[WebKit Review Bot]

Created attachment 96850 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Nikolas Zimmermann]

(In reply to comment #14)
> Created an attachment (id=96850) [details]
> Archive of layout-test-results from ec2-cr-linux-02
> 
> The attached test failures were seen while running run-webkit-tests on the chromium-ews.
> Bot: ec2-cr-linux-02  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

(In reply to comment #13)
> (From update of attachment 96848 [details])
> Attachment 96848 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/8830264
> 
> New failing tests:
> tables/mozilla/bugs/bug50695-2.html
> http/tests/local/drag-over-remote-content.html
> tables/mozilla/bugs/bug131020.html
> http/tests/misc/object-embedding-svg-delayed-size-negotiation-2.htm
> tables/mozilla/bugs/bug137388-2.html
> tables/mozilla/bugs/bug137388-3.html
> svg/zoom/page/zoom-replaced-intrinsic-ratio-001.htm
> fast/replaced/percent-height-in-anonymous-block.html
> fast/replaced/percent-height-in-anonymous-block-in-table.html
> tables/mozilla/bugs/bug137388-1.html

Wow, this is super useful! It turns out I didn't rerun all tests - we definately have changes in some areas. I'll report back..
I'll investigate...

[Nikolas Zimmermann]

Created attachment 96983 [details]
Patch v3

Fixed remaining regressions. The height: auto correction shouldn't be applied to anonymous containing blocks.

[WebKit Review Bot]

Comment on attachment 96983 [details]
Patch v3

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

New failing tests:
tables/mozilla/bugs/bug50695-2.html
http/tests/local/drag-over-remote-content.html
tables/mozilla/bugs/bug131020.html
http/tests/misc/object-embedding-svg-delayed-size-negotiation-2.htm
tables/mozilla/bugs/bug137388-2.html
tables/mozilla/bugs/bug137388-3.html
svg/zoom/page/zoom-replaced-intrinsic-ratio-001.htm
fast/replaced/percent-height-in-anonymous-block-in-table.html
tables/mozilla/bugs/bug137388-1.html

[WebKit Review Bot]

Created attachment 96988 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Dave Hyatt]

Comment on attachment 96983 [details]
Patch v3

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

r=me

> Source/WebCore/rendering/RenderReplaced.cpp:261
> +                // The aforementioned 'contraint equation' used for block-level, non-replaced elements in normal flow:

Typo. contraint should be constraint.

[Dave Hyatt]

This can be done in a followup bug, but you should write a few vertical writing-mode tests, just so we can make sure that the math is right. Remember that SVG as image should have the content locked to a horizontal orientation (like bitmap images do), so I suspect you will find that you need to make some changes.

[Nikolas Zimmermann]

Oops, forgot to close this bug. This was already landed in r88913.

[Eric Seidel (no email)]

Comment on attachment 96624 [details]
Adding 203 CSS 2.1 tests *replaced*/*intrinsic*

Cleared review? from attachment 96624 [details] so that this bug does not appear in http://webkit.org/pending-review.  If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again).

[Simon Fraser (smfr)]

This caused by 75691.