Bug 15849

Summary: CSS 2.1: Support replaced elements with relative intrinsic sizes
Product: WebKit Reporter: Eric Seidel (no email) <eric>
Component: CSSAssignee: Nikolas Zimmermann <zimmermann>
Status: RESOLVED FIXED    
Severity: Normal CC: aestes, ddkilzer, dglazkov, emacemac7, hyatt, jeffschiller, mjs, simon.fraser, timur.mehrvarz, webkit.review.bot, zimmermann
Priority: P2 Keywords: SVGHitList
Version: 528+ (Nightly build)   
Hardware: Macintosh   
OS: OS X 10.4   
Bug Depends on: 10526    
Bug Blocks: 15836, 11976, 12095, 15473, 53009, 53099    
Attachments:
Description Flags
Patch
hyatt: review-, webkit.review.bot: commit-queue-
Adding 203 CSS 2.1 tests *replaced*/*intrinsic*
none
Archive of layout-test-results from ec2-cr-linux-01
none
Patch v2
webkit.review.bot: commit-queue-
Archive of layout-test-results from ec2-cr-linux-02
none
Patch v3
hyatt: review+, webkit.review.bot: commit-queue-
Archive of layout-test-results from ec2-cr-linux-02 none

Description Eric Seidel (no email) 2007-11-05 21:26:07 PST
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
Comment 1 Eric Seidel (no email) 2007-12-03 03:52:28 PST
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.
Comment 2 Nikolas Zimmermann 2011-05-27 05:40:27 PDT
The patch on bug 10526 fixes this bug, for embedded SVG documents.
Comment 3 Nikolas Zimmermann 2011-05-27 11:37:33 PDT
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.
Comment 4 Nikolas Zimmermann 2011-06-09 13:09:56 PDT
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!
Comment 5 Nikolas Zimmermann 2011-06-09 13:12:20 PDT
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.
Comment 6 Nikolas Zimmermann 2011-06-09 13:16:24 PDT
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.
Comment 7 WebKit Review Bot 2011-06-09 14:09:01 PDT
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
Comment 8 WebKit Review Bot 2011-06-09 14:09:07 PDT
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
Comment 9 Dave Hyatt 2011-06-10 12:15:02 PDT
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.
Comment 10 Nikolas Zimmermann 2011-06-10 23:22:24 PDT
(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.
Comment 11 Nikolas Zimmermann 2011-06-11 00:03:57 PDT
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
...
Comment 12 Nikolas Zimmermann 2011-06-11 00:46:54 PDT
Created attachment 96848 [details]
Patch v2

Fix Daves comments.
Comment 13 WebKit Review Bot 2011-06-11 01:09:05 PDT
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
Comment 14 WebKit Review Bot 2011-06-11 01:09:11 PDT
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
Comment 15 Nikolas Zimmermann 2011-06-11 02:04:50 PDT
(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...
Comment 16 Nikolas Zimmermann 2011-06-13 11:48:21 PDT
Created attachment 96983 [details]
Patch v3

Fixed remaining regressions. The height: auto correction shouldn't be applied to anonymous containing blocks.
Comment 17 WebKit Review Bot 2011-06-13 12:14:55 PDT
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
Comment 18 WebKit Review Bot 2011-06-13 12:15:01 PDT
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
Comment 19 Dave Hyatt 2011-06-14 12:13:41 PDT
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.
Comment 20 Dave Hyatt 2011-06-14 12:16:28 PDT
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.
Comment 21 Nikolas Zimmermann 2011-06-22 01:10:37 PDT
Oops, forgot to close this bug. This was already landed in r88913.
Comment 22 Eric Seidel (no email) 2011-07-05 17:57:49 PDT
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).
Comment 23 Simon Fraser (smfr) 2012-01-06 11:13:54 PST
This caused by 75691.