Bug 13332 - List marker not displayed on items with overflow
Summary: List marker not displayed on items with overflow
Status: RESOLVED DUPLICATE of bug 192980
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 523.x (Safari 3)
Hardware: Macintosh OS X 10.4
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
: 9228 15372 24153 65080 (view as bug list)
Depends on:
Blocks:
 
Reported: 2007-04-11 11:10 PDT by Adele Peterson
Modified: 2019-01-08 01:44 PST (History)
15 users (show)

See Also:


Attachments
testcase (670 bytes, text/html)
2007-04-11 11:11 PDT, Adele Peterson
no flags Details
Include marker rectangle into clip. (11.53 KB, patch)
2011-09-28 11:12 PDT, Konstantin Shcheglov
hyatt: review-
Details | Formatted Diff | Diff
Use special PaintPhaseListMarkers (21.10 KB, patch)
2011-09-29 13:25 PDT, Konstantin Shcheglov
hyatt: review-
Details | Formatted Diff | Diff
More tests. Fixes for RenderLayerBacking, inline-block and overflow:hidden in OL parent. (34.02 KB, patch)
2011-10-05 14:05 PDT, Konstantin Shcheglov
no flags Details | Formatted Diff | Diff
More tests. Fixes for RenderLayerBacking, inline-block and overflow:hidden in OL parent. (34.02 KB, patch)
2011-10-06 06:25 PDT, Konstantin Shcheglov
hyatt: review-
hyatt: commit-queue-
Details | Formatted Diff | Diff
More tests. Check for clip context and force marker paint. (36.16 KB, patch)
2011-10-07 10:14 PDT, Konstantin Shcheglov
no flags Details | Formatted Diff | Diff
More tests. Check for clip context and force marker paint. (36.10 KB, patch)
2011-10-07 10:20 PDT, Konstantin Shcheglov
hyatt: review-
hyatt: commit-queue-
Details | Formatted Diff | Diff
Paint marker in LI, add overflows. More tests. (228.21 KB, patch)
2011-10-12 12:10 PDT, Konstantin Shcheglov
webkit-ews: commit-queue-
Details | Formatted Diff | Diff
Paint marker in LI, add overflows. More tests. (228.16 KB, patch)
2011-10-12 13:21 PDT, Konstantin Shcheglov
hyatt: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adele Peterson 2007-04-11 11:10:46 PDT
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.
Comment 1 Adele Peterson 2007-04-11 11:11:14 PDT
Created attachment 14011 [details]
testcase
Comment 2 Adele Peterson 2007-04-11 11:15:33 PDT
<rdar://problem/5127195>
Comment 3 mitz 2007-04-11 11:19:36 PDT
*** Bug 9228 has been marked as a duplicate of this bug. ***
Comment 4 mitz 2007-04-11 11:20:11 PDT
Another test case attached to bug 9228.
Comment 5 mitz 2007-10-04 09:02:56 PDT
*** Bug 15372 has been marked as a duplicate of this bug. ***
Comment 6 Jungshik Shin 2008-02-22 13:57:23 PST
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)

Comment 7 Wesley Moore 2009-02-24 20:58:14 PST
*** Bug 24153 has been marked as a duplicate of this bug. ***
Comment 8 Andreas Hammar 2011-07-24 10:27:51 PDT
*** Bug 65080 has been marked as a duplicate of this bug. ***
Comment 9 Konstantin Shcheglov 2011-09-27 13:10:42 PDT
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).
Comment 10 Konstantin Shcheglov 2011-09-28 11:12:55 PDT
Created attachment 109040 [details]
Include marker rectangle into clip.
Comment 11 Dave Hyatt 2011-09-28 13:10:30 PDT
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.
Comment 12 Konstantin Shcheglov 2011-09-29 13:25:46 PDT
Created attachment 109195 [details]
Use special PaintPhaseListMarkers
Comment 13 Dave Hyatt 2011-10-03 09:47:27 PDT
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).
Comment 14 Dave Hyatt 2011-10-03 09:48:18 PDT
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.
Comment 15 Konstantin Shcheglov 2011-10-05 14:05:48 PDT
Created attachment 109859 [details]
More tests. Fixes for RenderLayerBacking, inline-block and overflow:hidden in OL parent.
Comment 16 Konstantin Shcheglov 2011-10-05 14:11:40 PDT
(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.
Comment 17 Konstantin Shcheglov 2011-10-06 06:25:38 PDT
Created attachment 109954 [details]
More tests. Fixes for RenderLayerBacking, inline-block and overflow:hidden in OL parent.
Comment 18 Dave Hyatt 2011-10-06 12:21:19 PDT
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.
Comment 19 Dave Hyatt 2011-10-06 12:24:43 PDT
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.
Comment 20 Konstantin Shcheglov 2011-10-06 13:47:46 PDT
(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.
Comment 21 Konstantin Shcheglov 2011-10-07 10:14:25 PDT
Created attachment 110165 [details]
More  tests. Check for clip context and force marker paint.
Comment 22 WebKit Review Bot 2011-10-07 10:18:11 PDT
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.
Comment 23 Konstantin Shcheglov 2011-10-07 10:20:30 PDT
Created attachment 110166 [details]
More tests. Check for clip context and force marker paint.
Comment 24 Dave Hyatt 2011-10-07 12:23:09 PDT
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.
Comment 25 Dave Hyatt 2011-10-07 12:29:45 PDT
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.
Comment 26 Konstantin Shcheglov 2011-10-07 13:41:02 PDT
(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.
Comment 27 Konstantin Shcheglov 2011-10-07 14:03:20 PDT
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().
Comment 28 Dave Hyatt 2011-10-07 14:21:12 PDT
You need to add the marker as visual overflow of the list item, and then you'll be fine.
Comment 29 Konstantin Shcheglov 2011-10-12 12:10:16 PDT
Created attachment 110718 [details]
Paint marker in LI, add overflows. More tests.
Comment 30 Konstantin Shcheglov 2011-10-12 12:19:05 PDT
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.
Comment 31 Early Warning System Bot 2011-10-12 12:39:24 PDT
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
Comment 32 Gyuyoung Kim 2011-10-12 13:07:34 PDT
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
Comment 33 Konstantin Shcheglov 2011-10-12 13:21:52 PDT
Created attachment 110734 [details]
Paint marker in LI, add overflows. More tests.

Have to recreate attachment after merge.
Comment 34 Dave Hyatt 2011-10-18 11:17:02 PDT
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.
Comment 35 Konstantin Shcheglov 2011-10-18 11:50:15 PDT
> 
> > 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?
Comment 36 Dave Hyatt 2011-10-18 12:25:07 PDT
(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.
Comment 37 caqittepp-549 2012-11-06 06:04:54 PST
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.
Comment 38 Brad Vogel 2013-01-02 23:49:50 PST
Bump. This would be great to fix.
Comment 39 Robert Buchholz 2015-01-20 00:49:36 PST
Chromium issue: https://code.google.com/p/chromium/issues/detail?id=344941
Comment 40 cathiechen 2018-12-27 23:52:08 PST
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.
Comment 41 Frédéric Wang (:fredw) 2019-01-04 05:52:25 PST
@Konstantin: Are you still working on this bug? It seems it can just be marked as a duplicate of bug 192980 now.
Comment 42 Konstantin Shcheglov 2019-01-04 11:09:08 PST
No, I'm not working on this bug anymore.
Comment 43 Frédéric Wang (:fredw) 2019-01-08 01:44:33 PST

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