[Webkit-unassigned] [Bug 13332] List marker not displayed on items with overflow

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=13332


Dave Hyatt <hyatt at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #110734|review?                     |review-
               Flag|                            |




--- Comment #34 from Dave Hyatt <hyatt at apple.com>  2011-10-18 11:17:02 PST ---
(From update of attachment 110734)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.