Bug 78726 – Text should overflow when list item height set to 0

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

[Robert Hogan]

"If height of content exceeds the set height of a block-level non-replaced element in normal flow (like a list-item such as in this test), then the content should overflow according to the 'overflow' property."

[Robert Hogan]

Created attachment 127254 [details]
Patch

[Robert Hogan]

Anyone care to take a look? It's just a one-liner.

[Boris Zbarsky]

Should probably forward-dup bug 33094 to this one....

[Robert Hogan]

*** Bug 33094 has been marked as a duplicate of this bug. ***

[Gérard Talbot (no longer involved)]

Test in the test suite is

http://test.csswg.org/suites/css2.1/nightly-unstable/html4/height-applies-to-010a.htm

Gérard

[Eric Seidel (no email)]

Comment on attachment 127254 [details]
Patch

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

> 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 &&?

[Julien Chaffraix]

Comment on attachment 127254 [details]
Patch

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.

[Julien Chaffraix]

Comment on attachment 127254 [details]
Patch

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

>>> 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.

Another (likely better) way would be to check if you have some visual overflow on your RenderListItem. I would have to think about the corner cases though.

>> 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 :(

Actually scratch this comment, it's me just not reading the expected output properly (including the comments). I still think we need to test the other values of 'overflow'.

[Robert Hogan]

(In reply to comment #7)
> >> Source/WebCore/rendering/RenderListItem.cpp:381
> >> +    if (!logicalHeight() && (!firstLineBox() || (style()->overflowX() == OHIDDEN && style()->overflowY() == OHIDDEN)))
> 
> 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!

It would allow the painting of the list item but it would still get clipped later.  At the time I figured that overflow:auto or overflow:scroll may still require something to get painted in the list item so I was letting them through - but that's not the case at all. 

You're right about the testing. I disregarded block content, which also needs to be fixed. So the check is better as:

if (!logicalHeight() && style()->overflowY() != OVISIBLE))

If the list item has height:0, you only want to paint it if the list item allows any block or inline content to overflow unclipped. A height of zero won't allow paint other type of overflow in practice. It's a shortcut out of unnecessary painting and this seems to the only case where there's something to do.

[Robert Hogan]

Created attachment 127628 [details]
Patch

[Julien Chaffraix]

Comment on attachment 127628 [details]
Patch

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

High level comment: I would rather see the test split between the one from the CSS test suite vs your extended testing. This would ease maintenance when we include the new test suite.

Also I think dhyatt@ should look at this one. I don't see anything harmful in the change but I would like him to confirm my gut feeling.

> Source/WebCore/rendering/RenderListItem.cpp:381
> +    if (!logicalHeight() && (style()->overflowY() != OVISIBLE || style()->overflowX() != OVISIBLE))

In this case, you can just check overflowX() and don't check overflowY(). This is what we do in the rendering code (for example RenderLayer) and it works because there is some logic in CSSStyleSelector to adjust our RenderStyle in this case.

Also this is basically checking if we have an overflow clip so it should be !hasOverflowClip() instead of poking the style.

> LayoutTests/fast/css/heightless-list-item.html:15
> +  #t1

Please use a better name like div-list-item. Also, it's used twice so it should likely be a class not an id selector.

> LayoutTests/fast/css/heightless-list-item.html:48
> +  <p>Test passes if <strong>3 green "PASS"</strong> are each preceded by a filled disc.</p>

You now have more than 3 green PASS ;)

> LayoutTests/fast/css/heightless-list-item.html:76
> +    <li style="overflow-y:hidden;">PASS</li>

Looks like it should be factored in your style as a class. Same for overflow-x.

[Robert Hogan]

Created attachment 132592 [details]
Patch

[Robert Hogan]

(In reply to comment #11)
> 
> Also this is basically checking if we have an overflow clip so it should be !hasOverflowClip() instead of poking the style.

The setHasOverflowClip() bit isn't set in this situation so I stayed with poking the style.

[Julien Chaffraix]

Comment on attachment 132592 [details]
Patch

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

> Source/WebCore/rendering/RenderListItem.cpp:385
> +    if (!logicalHeight() && style()->overflowX() != OVISIBLE)

> The setHasOverflowClip() bit isn't set in this situation so I stayed with poking the style.

It doesn't sound right at all! Do you know why this is happening? (I am fine with opening a new bug and putting a FIXME here about this sorry state as I know this will bring us trouble down the road).

> LayoutTests/fast/css/heightless-list-item.html:43
> +  li#yhidden { overflow-y:hidden; }
> +  li#xhidden { overflow-x:hidden; }

I would love the same CSS massaging applied to the -expected.html file.

[Robert Hogan]

Created attachment 133327 [details]
Patch

[Julien Chaffraix]

Comment on attachment 133327 [details]
Patch

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

> Source/WebCore/rendering/RenderListItem.cpp:384
> +    // Paint a zero-height list item if it allows content overflow without clipping.

allows content _to_ overflow ..?

Overall, this comment is just a transcription of the code below so it could be dropped. This was just a bad optimization and we should be comparing the repaint rectangle vs the overflow rectangle like some other paint functions AFAICT.

[Robert Hogan]

Committed r111755: <http://trac.webkit.org/changeset/111755>