[webkit-reviews] review denied: [Bug 78726] Text should overflow when list item height set to 0 : [Attachment 127254] Patch

[bugzilla-daemon at webkit.org]

Julien Chaffraix <jchaffraix at webkit.org> has denied Robert Hogan
<robert at webkit.org>'s request for review:
Bug 78726: Text should overflow when list item height set to 0
https://bugs.webkit.org/show_bug.cgi?id=78726

Attachment 127254: Patch
https://bugs.webkit.org/attachment.cgi?id=127254&action=review

------- Additional Comments from Julien Chaffraix <jchaffraix at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=127254&action=review


I really think the testing is inadequate here, Robert. First the only test has
wrong results but also you don't check other values of 'overflow'.

>> Source/WebCore/rendering/RenderListItem.cpp:381
>> +	if (!logicalHeight() && (!firstLineBox() || (style()->overflowX() ==
OHIDDEN && style()->overflowY() == OHIDDEN)))
> 
> It seems like only one of the dimentions needs to be overflow hidden for all
the content to be clipped, right?  Can that be an || instead of an &&?

Eric is right here.

I think the change is not totally right. You are only handling the case
overflow: hidden and not all the overflow cases (the spec is pretty explicit
here and we have to follow the 'overflow' property). For example, we also have
an overflow clip for overflow: scroll and auto but your code would just skip
the clipping!

I really think this should be a RenderObject::hasOverflowClip() check instead
of poking the style.

> LayoutTests/fast/css/heightless-list-item-expected.html:62
> +</html>

The -expected.html is a copy of the test case so your test doesn't check
anything :(

> LayoutTests/fast/css/heightless-list-item.html:1
> +

It looks like this test is / will be part of the official CSS 2.1 test suite so
maybe it should be in LayoutTests/css.