[webkit-reviews] review denied: [Bug 13332] List marker not displayed on items with overflow : [Attachment 110734] Paint marker in LI, add overflows. More tests.

[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 110734: Paint marker in LI, add overflows. More tests.
https://bugs.webkit.org/attachment.cgi?id=110734&action=review

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


> Source/WebCore/rendering/PaintPhase.h:45
> +    PaintPhaseListMarkers,

You should not need a new paint phase.

> Source/WebCore/rendering/RenderListItem.cpp:344
> +void RenderListItem::paintMarker(PaintInfo& paintInfo, const LayoutPoint&
paintOffset)

This should be limited to outside list markers and not inside list markers,
which should continue to paint normally. The thing to understand about this
painting code path is that you're getting many many things wrong. I'm
overlooking it because I think it's more important to handle the
overflow:hidden case than to get some of these other cases right. However
broadening the scope to include inside list markers and to cause them to paint
incorrectly in those cases is not good.

> Source/WebCore/rendering/RenderListItem.cpp:355
> +    // Use PaintPhaseListMarkers to prevent marker painting by inline box.
> +    PaintInfo markerPaintInfo(paintInfo);
> +    markerPaintInfo.phase = PaintPhaseListMarkers;

You don't need a paint phase to do this. Just subclass InlineBox and have an
InlineListMarkerBox whose paint() method bails if you're an outside list marker
and calls the base class otherwise.

> Source/WebCore/rendering/RenderListItem.cpp:360
> +	   markerOffset.move(o->logicalLeft(), o->logicalTop());

Should probably have a FIXME here to at least support relative positioning.

> Source/WebCore/rendering/RenderListItem.h:61
> +    virtual void paintMarker(PaintInfo&, const LayoutPoint&);

I don't see why this needs to be virtual.

> Source/WebCore/rendering/RenderListMarker.cpp:1107
> -    if (paintInfo.phase != PaintPhaseForeground)
> +    if (paintInfo.phase != PaintPhaseListMarkers)

Use a list marker inline box subclass to handle controlling whether you paint
or not.