Bug 13332 – List marker not displayed on items with overflow

• Component: Layout and Rendering
• Status: RESOLVED DUPLICATE

[Adele Peterson]

List marker not displayed on items with overflow

See test.  If there is an element without overflow at the beginning of the list item, then the marker shows up.  Otherwise, if the list item starts with an overflowed element, then the marker is not displayed.

[Adele Peterson]

Created attachment 14011 [details]
testcase

[Adele Peterson]

<rdar://problem/5127195>

*** Bug 9228 has been marked as a duplicate of this bug. ***

Another test case attached to bug 9228.

*** Bug 15372 has been marked as a duplicate of this bug. ***

[Jungshik Shin]

Just FYI, http://im.qq.com/  (one of top 100 sites in China) suffers from this
bug. Items in the right-hand sidebar (titled '最新动态') do not have bullets. (set your browser encoding to UTF-8 to view Chinese characters in this comment)

[Wesley Moore]

*** Bug 24153 has been marked as a duplicate of this bug. ***

[Andreas Hammar]

*** Bug 65080 has been marked as a duplicate of this bug. ***

[Konstantin Shcheglov]

I'm working on this bug  now.

Problem is caused by RenderBox::pushContentsClip().
If DIV has overflow clip, then it is applied before paintObject(), i.e. content.
However RenderListItem::updateMarkerLocation() adds marker to its child DIV (actually parent of first RenderLineBoxList), with negative X coordinate.
But RenderBox::overflowClipRect() includes only rectangle of DIV content, so excludes RenderListMarker painting rectangle. So, marker is not painted if first  child has "overflow: hidden" specified.

I'm not sure how to solve this.
May be change RenderBox::overflowClipRect() to ask (recursively) children to update clip. Only RenderListMarker will do this. Drawbacks:
1. performance hit;
2. may be will cause some problems with leaking content outside of clip because clip will be bigger (on negative side, so probably fine, but still worth to keep in mind).

[Konstantin Shcheglov]

Created attachment 109040 [details]
Include marker rectangle into clip.

[Dave Hyatt]

Comment on attachment 109040 [details]
Include marker rectangle into clip.

This is pretty obviously wrong. You only patched the non-layer case, and you have to handle both cases. In addition doing a unite will cause content that might have spilled out of the overflow:hidden box to become visible.

Any approach that solves this problem has to allow the marker to paint while still preserving an accurate clip for everything else. One idea might be to paint markers at the same time as your own background/shadows etc. so that the overflow clip won't have been pushed yet.

[Konstantin Shcheglov]

Created attachment 109195 [details]
Use special PaintPhaseListMarkers

[Dave Hyatt]

Comment on attachment 109195 [details]
Use special PaintPhaseListMarkers

You're missing a couple of places (and I'd like to see a test for each place I'm about to call out).

paintFloats you need to patch. Make a float with a list marker inside somewhere.

You also missed InlineBox::paint. Make a div with display:inline-block and then put a list inside it somewhere.

You missed RenderLayerBacking::paintIntoLayer. To test this one make a layer with a 3d transform (translateZ(0) is sufficient I think) and put a list marker inside it.

The fact that these places are hard to spot is a problem. You might consider making a helper function called by these places to paint all the phases (assuming that's possible).

[Dave Hyatt]

For completeness I suppose you should include RenderScrollbarPart as well, although a list marker inside a scrollbar would be pretty silly!! If you're making a helper function, though, you could patch it.

[Konstantin Shcheglov]

Created attachment 109859 [details]
More tests. Fixes for RenderLayerBacking, inline-block and overflow:hidden in OL parent.

[Konstantin Shcheglov]

(In reply to comment #13)

  I've added tests and tweaks for display:inline-block and translateZ(0).
  I've updated RenderScrollbarPart, can not test it.

  I tried to add helper method, but rolled this back.
  Not all places use exactly same set of phases, for example no PaintPhaseChildBlockBackgrounds in RenderLayerBacking.

  I've found (and added  test) new problem - when we put OL into DIV with overflow:hidden, in this case we still want to apply clip. So, clip should be ignored only on PaintPhaseListMarkers and only if current renderer contains list marker, because this is artificial construct.

[Konstantin Shcheglov]

Created attachment 109954 [details]
More tests. Fixes for RenderLayerBacking, inline-block and overflow:hidden in OL parent.

[Dave Hyatt]

Comment on attachment 109954 [details]
More tests. Fixes for RenderLayerBacking, inline-block and overflow:hidden in OL parent.

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.

[Dave Hyatt]

You could possibly limit an outside marker painting hack to just ask if the marker is inside any overflow:hidden objects in the containing block chain between it and the list item (including the list item), and only if so do you do a special attempt to paint it yourself.

[Konstantin Shcheglov]

(In reply to comment #18)
> (From update of attachment 109954 [details])
> 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.

  Actually this code was added not because we don't paint markers, but because we paint too much of them.
  We should respect clip of _parent_ DIV, including layer.

  In case of RenderLayer we already have clip.
  As I see, RenderLayer uses clip from Renderer with overflow:hidden.
  So, I can not add any new test here.

 
> 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.

  Yes, here we have problem.
  We paint when layer clip is already set.
  I've added test.

[Konstantin Shcheglov]

Created attachment 110165 [details]
More  tests. Check for clip context and force marker paint.

[WebKit Review Bot]

Attachment 110165 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1

Source/WebCore/rendering/RenderListItem.cpp:369:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 1 in 23 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Konstantin Shcheglov]

Created attachment 110166 [details]
More tests. Check for clip context and force marker paint.

[Dave Hyatt]

Comment on attachment 110166 [details]
More tests. Check for clip context and force marker paint.

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.

[Dave Hyatt]

Also, I think you need to paint during the background phase of the list item. Otherwise the clip can have been pushed in the RenderLayer case. When painting a clipped marker, you can do it during the background phase and then just make a new paint info with the phase shifted to foreground before calling it.

Make a new test like overflow-hidden-relative.html but put the overflow:hidden;position:relative on the <li>. You'll see it fails with your latest patch. It will start passing if you do the clipped marker paint during the background phase of the list item and then push a new paintinfo shifted to foreground when calling paint on the marker box.

[Konstantin Shcheglov]

(In reply to comment #25)

It seems that we should go deeper.

1. For LI with clipped DIV we don't get PaintPhaseBlockBackground. I've tried to use PaintPhaseChildBlockBackground instead, but... next item.

2. For LI with layer such way of painting does not help. I get PaintPhaseBlockBackground, but at this time paintInfo.rect (and I suspect that clip too) is set to paint only item itself. I think that this may be related to "list-style-position" property. By default marker is outside of item. So, it seems correct that layer does not allow us paint marker on item. We may want to paint marker _for_ item, but not _on_ item.

I see following description about markers and "overflow:hidden".
---
In CSS 2.1, a UA may hide the marker if the element's 'overflow' is other than 'visible'. (This is expected to change in the future.)
---


So, I see two possible solutions:

1. Follow "may hide" and ignore this case, rollback to PaintPhaseForeground.

2. Go deeper and initiate marker painting from RenderList. It sounds odd for me, but keeping in mind "list-style-position" property this may be logical step.

[Konstantin Shcheglov]

Hm...
Given that there are no RenderList, we will need also to create one.
Looks not very hard, just create super class for HTML[D,O,U]ListElement and override createRenderer().

[Dave Hyatt]

You need to add the marker as visual overflow of the list item, and then you'll be fine.

[Konstantin Shcheglov]

Created attachment 110718 [details]
Paint marker in LI, add overflows. More tests.

[Konstantin Shcheglov]

Main changes in latest patch.

1. Paint _all_ markers in RenderListItem::paintMarker(). At the end there are no much reason to use half-backed solution.

2. Because we paint all markers in LI, if I understand this correctly, there are no need to add overflows for all parents of marker.

3. To prevent painting marker two times, from LI and from InlineBox, use special PaintPhaseListMarkers.

4. Existing tests were changed: 2 of them because we now paint marker on background, and 1 because marker is now visible.

[Early Warning System Bot]

Comment on attachment 110718 [details]
Paint marker in LI, add overflows. More tests.

Attachment 110718 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10044005

[Gyuyoung Kim]

Comment on attachment 110718 [details]
Paint marker in LI, add overflows. More tests.

Attachment 110718 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/10045011

[Konstantin Shcheglov]

Created attachment 110734 [details]
Paint marker in LI, add overflows. More tests.

Have to recreate attachment after merge.

[Dave Hyatt]

Comment on attachment 110734 [details]
Paint marker in LI, add overflows. More tests.

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.

[Konstantin Shcheglov]

> 
> > 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.

  Well, I definitely don't want to make things worse and paint incorrectly.

  What is wrong?
  It passes all existing List tests, with exception of painting markers in background.

  Should I add more tests?
  For which cases?

[Dave Hyatt]

(In reply to comment #35)
>
>   What is wrong?
>   It passes all existing List tests, with exception of painting markers in background.
> 

Well, the main issue is that inside markers should be clipped by overflow:hidden, so by painting them outside the overflow clip, you're causing incorrect rendering. Only outside markers are supposed to ignore the clips. Inside markers are considered part of the line content.

As for what's broken with your outside list marker path, mainly relative positioning and transforms are broken in this painting path for all markers. I don't think that's a big issue and doesn't have to be fixed now, but inside markers not respecting them would just be weird.

Please fix this. It's a major issue considering the alternative is setting list-style-position to inside which in turn is bugged in Firefox.
There's no way to cross-browser solve this without using a background-image for the bullet.

[Brad Vogel]

Bump. This would be great to fix.

[Robert Buchholz]

Chromium issue: https://code.google.com/p/chromium/issues/detail?id=344941

[cathiechen]

This has been fixed in chromium. Recently, I'm porting the implement to WebKit.
Upload the patch in:
https://bugs.webkit.org/show_bug.cgi?id=192980

It would be very nice if you guys could code review it.

[Frédéric Wang (:fredw)]

@Konstantin: Are you still working on this bug? It seems it can just be marked as a duplicate of bug 192980 now.

[Konstantin Shcheglov]

No, I'm not working on this bug anymore.

[Frédéric Wang (:fredw)]

*** This bug has been marked as a duplicate of bug 192980 ***