Bug 78726 - Text should overflow when list item height set to 0
: Text should overflow when list item height set to 0
Status: RESOLVED FIXED
: WebKit
CSS
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
: http://www.gtalbot.org/BrowserBugsSec...
:
:
:
  Show dependency treegraph
 
Reported: 2012-02-15 11:17 PST by
Modified: 2012-03-22 13:52 PST (History)


Attachments
Patch (6.17 KB, patch)
2012-02-15 15:41 PST, Robert Hogan
no flags Review Patch | Details | Formatted Diff | Diff
Patch (9.55 KB, patch)
2012-02-17 12:02 PST, Robert Hogan
no flags Review Patch | Details | Formatted Diff | Diff
Patch (13.77 KB, patch)
2012-03-19 09:12 PST, Robert Hogan
no flags Review Patch | Details | Formatted Diff | Diff
Patch (13.07 KB, patch)
2012-03-22 13:04 PST, Robert Hogan
jchaffraix: review+
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2012-02-15 11:17:22 PST
"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."
------- Comment #1 From 2012-02-15 15:41:58 PST -------
Created an attachment (id=127254) [details]
Patch
------- Comment #2 From 2012-02-16 11:30:22 PST -------
Anyone care to take a look? It's just a one-liner.
------- Comment #3 From 2012-02-16 11:43:40 PST -------
Should probably forward-dup bug 33094 to this one....
------- Comment #4 From 2012-02-16 11:47:06 PST -------
*** Bug 33094 has been marked as a duplicate of this bug. ***
------- Comment #5 From 2012-02-16 11:50:36 PST -------
Test in the test suite is

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

Gérard
------- Comment #6 From 2012-02-16 15:03:52 PST -------
(From update of attachment 127254 [details])
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 &&?
------- Comment #7 From 2012-02-16 18:22:22 PST -------
(From update of attachment 127254 [details])
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.
------- Comment #8 From 2012-02-16 18:37:13 PST -------
(From update of attachment 127254 [details])
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'.
------- Comment #9 From 2012-02-17 11:24:51 PST -------
(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.
------- Comment #10 From 2012-02-17 12:02:39 PST -------
Created an attachment (id=127628) [details]
Patch
------- Comment #11 From 2012-02-21 13:28:39 PST -------
(From update of attachment 127628 [details])
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.
------- Comment #12 From 2012-03-19 09:12:16 PST -------
Created an attachment (id=132592) [details]
Patch
------- Comment #13 From 2012-03-20 11:31:27 PST -------
(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.
------- Comment #14 From 2012-03-20 20:54:13 PST -------
(From update of attachment 132592 [details])
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.
------- Comment #15 From 2012-03-22 13:04:03 PST -------
Created an attachment (id=133327) [details]
Patch
------- Comment #16 From 2012-03-22 13:27:26 PST -------
(From update of attachment 133327 [details])
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.
------- Comment #17 From 2012-03-22 13:52:52 PST -------
Committed r111755: <http://trac.webkit.org/changeset/111755>