Summary: | Text should overflow when list item height set to 0 | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Robert Hogan <robert> | ||||||||||
Component: | CSS | Assignee: | Robert Hogan <robert> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | browserbugs2, bzbarsky, darin, jchaffraix, robert, simon.fraser, spamme | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
URL: | http://www.gtalbot.org/BrowserBugsSection/css21testsuite/heightless-list-item.html | ||||||||||||
Attachments: |
|
Description
Robert Hogan
2012-02-15 11:17:22 PST
Created attachment 127254 [details]
Patch
Anyone care to take a look? It's just a one-liner. Should probably forward-dup bug 33094 to this one.... *** Bug 33094 has been marked as a duplicate of this bug. *** Test in the test suite is http://test.csswg.org/suites/css2.1/nightly-unstable/html4/height-applies-to-010a.htm Gérard 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 &&? 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. 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'. (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. Created attachment 127628 [details]
Patch
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. Created attachment 132592 [details]
Patch
(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 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. Created attachment 133327 [details]
Patch
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. Committed r111755: <http://trac.webkit.org/changeset/111755> |