[webkit-reviews] review denied: [Bug 13332] List marker not displayed on items with overflow : [Attachment 109954] More tests. Fixes for RenderLayerBacking, inline-block and overflow:hidden in OL parent.

[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 109954: More tests. Fixes for RenderLayerBacking, inline-block and
overflow:hidden in OL parent.
https://bugs.webkit.org/attachment.cgi?id=109954&action=review

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


> Source/WebCore/rendering/RenderBox.cpp:1160
> +    if (paintInfo.phase == PaintPhaseListMarkers)
> +	   for (RenderObject* child = firstChild(); child; child =
child->nextSibling())
> +	       if (child->isListMarker())
> +		   return false;

I don't much like this code and am trying to think of a better approach.

The reason I am minusing though is that whatever test case led you to do this,
you didn't patch the RenderLayer version. Find the test case that led you to
add this code and then change the overflow:hidden object to also have
position:relative. The RenderLayer clips will then be pushed, and since you
only paint list markers during the time that the foreground clip is respected
in RenderLayer, you'll still have a problem.

Even then, this doesn't seem good enough to me. What about a marker placed
inside an overflow:hidden child div that also has a RenderLayer? I'm not even
sure we propagate the overflow properly in this case.

As I mentioned before, this bug is hard. :(

Another possible approach that avoids using paint phases would be to change
outside list markers completely to have the list item actually do it. You would
not need a special paint phase for this if the list item actually paints the
marker. Basically figure out the translation, jump right to it and paint it
before calling your base class paint. If you do it in the background phase I
think it would work with RenderLayers too.