Percentage width replaced element in zero percent/fixed width container block incorrectly rendered.

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Sam Weinig]

Due to special casing in the layout code, if a container block has a 0px width, children with percentage widths will still be rendered even though they should be 0px wide.

[Sam Weinig]

Created attachment 8900 [details]
test case

[Pravin D]

Created attachment 151163 [details]
Patch

[Andy Estes]

Comment on attachment 151163 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        The width of a replaced element with percent width is governed by the width of its container. When the width of the container is 
> +        zero percent/fixed value then the width of the replaced element must also be zero.

Is there a similar issue with 0-height containers? Do we already handle that correctly?

> Source/WebCore/rendering/RenderBox.cpp:2199
> +            Length ContainerLogicalWidth = containingBlock()->style()->logicalWidth();

This variable should be named containerLogicalWidth (note the lowercase c).

> Source/WebCore/rendering/RenderBox.cpp:2200
> +            // FIXME: Handle cases when containing block width is calculated or View port percent.

viewport should be one word, with no capital v.

> LayoutTests/fast/css/percent-width-img-inside-zero-percent-and-fixed-container-expected.txt:10
> +The test case checks if the calculated width of an Image with percent width inside a zero percent/fixed container is zero.
> +
> +On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
> +
> +PASS fixedContainerImage.getBoundingClientRect().width is 0
> +PASS percentContainerImage.getBoundingClientRect().width is 0
> +PASS successfullyParsed is true
> +
> +TEST COMPLETE
> +

It seems strange write a dumpAsText test that prints values from the render tree. Why not make this test just dump the render tree? Your test would be much simpler since it wouldn't need all the JavaScript.

[Pravin D]

(In reply to comment #3)
> (From update of attachment 151163 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=151163&action=review
> 
> > Source/WebCore/ChangeLog:9
> > +        The width of a replaced element with percent width is governed by the width of its container. When the width of the container is 
> > +        zero percent/fixed value then the width of the replaced element must also be zero.
> 
> Is there a similar issue with 0-height containers? Do we already handle that correctly?
> 

I'm not sure... need to check...
> > Source/WebCore/rendering/RenderBox.cpp:2199
> > +            Length ContainerLogicalWidth = containingBlock()->style()->logicalWidth();
> 
> This variable should be named containerLogicalWidth (note the lowercase c).
> 
> > Source/WebCore/rendering/RenderBox.cpp:2200
> > +            // FIXME: Handle cases when containing block width is calculated or View port percent.
> 
> viewport should be one word, with no capital v.
> 
Will do it...
> > LayoutTests/fast/css/percent-width-img-inside-zero-percent-and-fixed-container-expected.txt:10
> > +The test case checks if the calculated width of an Image with percent width inside a zero percent/fixed container is zero.
> > +
> > +On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
> > +
> > +PASS fixedContainerImage.getBoundingClientRect().width is 0
> > +PASS percentContainerImage.getBoundingClientRect().width is 0
> > +PASS successfullyParsed is true
> > +
> > +TEST COMPLETE
> > +
> 
> It seems strange write a dumpAsText test that prints values from the render tree. Why not make this test just dump the render tree? Your test would be much simpler since it wouldn't need all the JavaScript.
> 
Need to check... Seemed more informative to use dumpAsText()... Otherwise I need to remove all the text from the test case to make it platform independent...

[Sam Weinig]

(In reply to comment #3)
> > LayoutTests/fast/css/percent-width-img-inside-zero-percent-and-fixed-container-expected.txt:10
> > +The test case checks if the calculated width of an Image with percent width inside a zero percent/fixed container is zero.
> > +
> > +On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
> > +
> > +PASS fixedContainerImage.getBoundingClientRect().width is 0
> > +PASS percentContainerImage.getBoundingClientRect().width is 0
> > +PASS successfullyParsed is true
> > +
> > +TEST COMPLETE
> > +
> 
> It seems strange write a dumpAsText test that prints values from the render tree. Why not make this test just dump the render tree? Your test would be much simpler since it wouldn't need all the JavaScript.

Better yet, make it a ref test?

[Sam Weinig]

Comment on attachment 151163 [details]
Patch

r-ing based on Andy's review.  It will probably need another round.  Thanks for working on this.  It is fun to see bugs I filed 6 years ago getting some traction :).

[Pravin D]

(In reply to comment #3)
> (From update of attachment 151163 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=151163&action=review
> 

> It seems strange write a dumpAsText test that prints values from the render tree. Why not make this test just dump the render tree? Your test would be much simpler since it wouldn't need all the JavaScript.
> 

I tried to make dump the render tree rather than using JS to calculate the width. It makes the expected file platform specific(at least qt port does it) even though the width of the IMG must be zero.

(In reply to comment #5)

> Better yet, make it a ref test?
> 
Ref might be in my opinion a over kill (As this not being a paint level fix).

Anyways Im uploading a patch the suggest changes by Andy (including the dumping the render tree). Please let me know your opinions

[Pravin D]

Created attachment 151307 [details]
Patch

[Pravin D]

(In reply to comment #6)
> (From update of attachment 151163 [details])
> r-ing based on Andy's review.  It will probably need another round.  Thanks for working on this.  It is fun to see bugs I filed 6 years ago getting some traction :).

> 

Your welcome... but I wonder why no one had picked this bug earlier !!!

[Sam Weinig]

(In reply to comment #9)
> (In reply to comment #6)
> > (From update of attachment 151163 [details] [details])
> > r-ing based on Andy's review.  It will probably need another round.  Thanks for working on this.  It is fun to see bugs I filed 6 years ago getting some traction :).
> 
> > 
> 
> Your welcome... but I wonder why no one had picked this bug earlier !!!

Or why I didn't just fix it when I filed it...6 years is long time to remember such things.

[Pravin D]

(In reply to comment #10)
> (In reply to comment #9)
> > (In reply to comment #6)
> > > (From update of attachment 151163 [details] [details] [details])
> > > r-ing based on Andy's review.  It will probably need another round.  Thanks for working on this.  It is fun to see bugs I filed 6 years ago getting some traction :).
> > 
> > > 
> > 
> > Your welcome... but I wonder why no one had picked this bug earlier !!!
> 
> Or why I didn't just fix it when I filed it...6 years is long time to remember such things.

> 
 :)

What do think abt the patch... should we go with the current test case or the previous ??

[Pravin D]

Will EFL bot ever finish ?!!

[Robert Hogan]

Comment on attachment 151307 [details]
Patch

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

> Source/WebCore/rendering/RenderBox.cpp:2201
> +            if (cw > 0 || (!cw && (containerLogicalWidth.isFixed() || containerLogicalWidth.isPercent())))

could you not just say:

else if (!cw && (containerLogicalWidth.isFixed() || containerLogicalWidth.isPercent())
    return 0;

This would avoid the need to compute the width when we know the result we want.

[Andy Estes]

(In reply to comment #7)
> (In reply to comment #5)
> 
> > Better yet, make it a ref test?
> > 
> Ref might be in my opinion a over kill (As this not being a paint level fix).
>

I actually agree with Sam that a reftest would be the best option here. You avoid both platform-dependent expected results and having to add script to your test.

I'd like to see you go one more round and add a reftest. It should be pretty easy: the reference document could just be the zero-width containers without the replaced children.

[Andy Estes]

Comment on attachment 151307 [details]
Patch

I like this test better, although Sam correctly pointed out that a ref test would

[Pravin D]

(In reply to comment #14)
> (In reply to comment #7)
> > (In reply to comment #5)
> > 
> > > Better yet, make it a ref test?
> > > 
> > Ref might be in my opinion a over kill (As this not being a paint level fix).
> >
> 
> I actually agree with Sam that a reftest would be the best option here. You avoid both platform-dependent expected results and having to add script to your test.
> 
> I'd like to see you go one more round and add a reftest. It should be pretty easy: the reference document could just be the zero-width containers without the replaced children.
> 

Sure... I'll add a ref test n upload another patch...
Apart from the test cases, whats your opinion of the fix?

PS:
I'm a little confused with test case preference thou :(
I was of thought that a dumpAsText() is better than a refTest(coz of the pixel test overhead) than a dumpRenderTree test (coz its highly platform senstivity).

How wud one go about choosing the correct type of test case??

[Pravin D]

(In reply to comment #13)
> (From update of attachment 151307 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=151307&action=review
> 
> > Source/WebCore/rendering/RenderBox.cpp:2201
> > +            if (cw > 0 || (!cw && (containerLogicalWidth.isFixed() || containerLogicalWidth.isPercent())))
> 
> could you not just say:
> 
> else if (!cw && (containerLogicalWidth.isFixed() || containerLogicalWidth.isPercent())
>     return 0;
> 
> This would avoid the need to compute the width when we know the result we want.
> 

I don't think thts entirely right. Correct me if I'm wrong :)
If you see the function
minimumValueForLength(logicalWidth, cw) when logicalWidth is percent, the its return value is zero if cw is zero(taken). However, if logicalWidth is calculated, I was not sure if it will always eval to zero . Im not very familar with calculated value type but I'd expect calculated values can  also be some kind expression that can be evaluated based on the seed value that is passed to it. If its true (calc value can be expressions) then its not proper to always ret zero...

Let me know ur opinion...

[Andy Estes]

(In reply to comment #16)
> (In reply to comment #14)
> > (In reply to comment #7)
> > > (In reply to comment #5)
> > > 
> > > > Better yet, make it a ref test?
> > > > 
> > > Ref might be in my opinion a over kill (As this not being a paint level fix).
> > >
> > 
> > I actually agree with Sam that a reftest would be the best option here. You avoid both platform-dependent expected results and having to add script to your test.
> > 
> > I'd like to see you go one more round and add a reftest. It should be pretty easy: the reference document could just be the zero-width containers without the replaced children.
> > 
> 
> Sure... I'll add a ref test n upload another patch...
> Apart from the test cases, whats your opinion of the fix?

The fix ok to me, and it doesn't seem to break any tests according to the EWS bots. I'd probably want a layout expert to review it, though.

> 
> PS:
> I'm a little confused with test case preference thou :(
> I was of thought that a dumpAsText() is better than a refTest(coz of the pixel test overhead) than a dumpRenderTree test (coz its highly platform senstivity).

You aren't considering the complexity of the test itself. A dumpAsText test is an awkward choice for testing layout metrics, as you can see by comparing the complexity of your first test to that of your second. Reference tests and non-dumpAsText tests exist to regression test changes to layout and rendering, so it makes sense to use them here.

[Pravin D]

(In reply to comment #18)
> (In reply to comment #16)
> > (In reply to comment #14)
> > > (In reply to comment #7)
> > > > (In reply to comment #5)
> > > > 
> > > > > Better yet, make it a ref test?
> > > > > 
> > > > Ref might be in my opinion a over kill (As this not being a paint level fix).
> > > >
> > > 
> > > I actually agree with Sam that a reftest would be the best option here. You avoid both platform-dependent expected results and having to add script to your test.
> > > 
> > > I'd like to see you go one more round and add a reftest. It should be pretty easy: the reference document could just be the zero-width containers without the replaced children.
> > > 
> > 
> > Sure... I'll add a ref test n upload another patch...
> > Apart from the test cases, whats your opinion of the fix?
> 
> The fix ok to me, and it doesn't seem to break any tests according to the EWS bots. I'd probably want a layout expert to review it, though.
> 
ok...

> > 
> > PS:
> > I'm a little confused with test case preference thou :(
> > I was of thought that a dumpAsText() is better than a refTest(coz of the pixel test overhead) than a dumpRenderTree test (coz its highly platform senstivity).
> 
> You aren't considering the complexity of the test itself. A dumpAsText test is an awkward choice for testing layout metrics, as you can see by comparing the complexity of your first test to that of your second. Reference tests and non-dumpAsText tests exist to regression test changes to layout and rendering, so it makes sense to use them here.
> 
maybe...

[Pravin D]

Created attachment 151589 [details]
Patch

[Pravin D]

(In reply to comment #20)
> Created an attachment (id=151589) [details]
> Patch
> 
Patch with refTest cases as suggested by Sam and Andy.

[Pravin D]

Created attachment 151670 [details]
Patch

[Pravin D]

(In reply to comment #22)
> Created an attachment (id=151670) [details]
> Patch
> 
Sorry!!! I had missed a Img resource file in the previous patch.

[Andy Estes]

Comment on attachment 151670 [details]
Patch

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

I like the ref test. r=me. Two nits about the image resource: the '-jpeg' in the filename is redundant, and the resource would likely be smaller as a PNG.

> Source/WebCore/ChangeLog:15
> +         The containing block is float or positioned in which case the width of the replace child element must be its instrinsic width.

floated, not float.
replaced, not replace.

> Source/WebCore/ChangeLog:17
> +         replace elment must be zero. 

replaced, not replace.

> Source/WebCore/rendering/RenderBox.cpp:2200
> +            // FIXME: Handle cases when containing block width is calculated or viewport percent.

Please file a bug and mention it here.

[Pravin D]

Created attachment 151911 [details]
Patch

[WebKit Review Bot]

Comment on attachment 151911 [details]
Patch

Clearing flags on attachment: 151911

Committed r122501: <http://trac.webkit.org/changeset/122501>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Pravin D]

Thanks for ur time and review Andy :)

[Julien Chaffraix]

This change caused bug 95892.