[webkit-reviews] review denied: [Bug 13332] List marker not displayed on items with overflow : [Attachment 110166] More tests. Check for clip context and force marker paint.

[bugzilla-daemon at webkit.org]

Dave Hyatt <hyatt at apple.com> has denied Konstantin Shcheglov
<scheglov at google.com>'s request for review:
Bug 13332: List marker not displayed on items with overflow
https://bugs.webkit.org/show_bug.cgi?id=13332

Attachment 110166: More tests. Check for clip context and force marker paint.
https://bugs.webkit.org/attachment.cgi?id=110166&action=review

------- Additional Comments from Dave Hyatt <hyatt at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=110166&action=review


> Source/WebCore/rendering/RenderListItem.cpp:363
> +    // Marker paints itself only on PaintPhaseForeground.

Change to "The marker paints itself only during PaintPhaseForeground."

> Source/WebCore/rendering/RenderListItem.cpp:368
> +	   // Check if we are on clipped parent.

"Check if we are inside a clipped parent."

> Source/WebCore/rendering/RenderListItem.cpp:370
> +	   for (RenderBox* o = m_marker->parentBox(); !isClipped; o =
o->parentBox()) {

This check isn't quite right and it tells me you need another test with the
marker inside a <span> that is inside overflow:hidden. :)

You should just use a containingBlock() loop instead of parent() or
parentBox(). That way you'll look only at the blocks between you and the list
marker, and those blocks are the only ones that can have overflow:hidden set on
them.

> Source/WebCore/rendering/RenderListItem.cpp:381
> +	   for (RenderBox* o = m_marker->parentBox(); ; o = o->parentBox()) {

This loop is incorrect in the same way.  Use the containing block chain.

Also, why not just combine this into the previous loop? I don't think there's
any harm in computing the coordinates as you go, even if it turns out the
marker wasn't clipped.

Note there are some other coordinate issues (e.g., dealing with relative
positioning or transforms on the intermediate blocks), but I think that can
wait for a followup bug.