Bug 4855 – List item's bullets fail to redraw correctly after their style is set with JavaScript.

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Alexei Svitkine]

This happens with both the latest Safari build from source, and the shipping Safari for 10.4.2 as of 
September 5, 2005.

The test case is very simple, and works well everywhere (IE, FireFox, Opera) except Safari.

I set the onMouseOver and onMouseOut properties of the list-items in an unordered list to toggle the 
bullet style for those items. That is, when the mouse is over an item, the bullet should appear, and 
when it leaves the item, the bullet should disappear. This happens on all browsers except Safari.

What happens on Safari is that the bullet does appear when the mouse is over the item, but does not 
disappear when the mouse leaves the location.

Upon further investigation, it seems to be a redrawing problem - as the attributes behind the scenes 
seem to change (if I resize the window, it will redraw correctly). It just seems it isn't being redrawn 
properly when the JavaScript sets the listStyle to "none" as it should.

[Alexei Svitkine]

Forgot to mention, but see http://projectmagma.net/~myrdred/test.html for this bug in action.

[Mark Rowe (bdash)]

Confirmed with WebKit 412.7 and ToT.  Behaves as intended in Gecko-based browsers.

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

Bug 6538 has another testcase: attachment 5674 [details].

See also bug 6844

[Darin Adler]

Bug 7542 describes something similar that seems to be a regression.

[Justin Garcia]

We've seen this while working on list editing.  I'm not sure which rect is used to invalidate a region on style changes or node removal/insertion.  I do know that the RenderBox's position and size do not correspond to the position and size of the marker that is painted for outside list markers, but I assumed that the painting code would use getAbsoluteRepaintRect (which I fixed in 6844) when invalidating rects.  My changes didn't fix this redraw problem, so the painting code must not use getAbsoluteRepaintRect. 

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

Moving the code that updates the marker from RenderListItem::setStyle() to RenderListItem::layout() fixes this and similar bugs (it delays the changes long enough for the list item's repaint-before-layout to still cover the correct rect). The real fix will need to avoid updating the marker on every layout (a simple flag should work).

Actually, the above *almost* fixed the bug - one column of pixels from the antialiased edge of the disc is left behind, but this is a separate issue (that should be easy to fix).

Created attachment 7120 [details]
Patch

[Justin Garcia]

Comment on attachment 7120 [details]
Patch

This comment: 
+        (WebCore::RenderListItem::getAbsoluteRepaintRect): Unite with the marker rect
+        of inside markers as well, to leave room for antialiasing of disc and circle
+        markers.
Would be helpful here:
-    if (m_marker && m_marker->parent() == this && !m_marker->isInside()) {
+    if (m_marker && m_marker->parent() == this) {

[Dave Hyatt]

Comment on attachment 7120 [details]
Patch

We discussed some options on IRC.  You can't delay the style setting on the marker, since non-layout changes like foreground color have to propagate immediately.  You can't rely on setStyle always triggering a layout.

The bug here seems to only be an issue when a future layout is triggered by setStyle.  In this case the style relies on the layout repaint machinery, but that machinery is not good enough here.

Possible solutions include:
(1) Destroy the marker when setStyle triggers a layout of the list item.
(2) Repaint the marker when setStyle triggers a layout of the list item.  Could be checked by asking if selfNeedsLayout after the setStyle of the list item's base class has been done.

[Dave Hyatt]

I should add 

(3) Redo list markers to have genuine boxes that don't lie about their position/size, so that all the layout repaint machinery can do its job and we can stop adding more and more code to work around this problem. ;)

(In reply to comment #12)
> (1) Destroy the marker when setStyle triggers a layout of the list item.
> (2) Repaint the marker when setStyle triggers a layout of the list item.  Could
> be checked by asking if selfNeedsLayout after the setStyle of the list item's
> base class has been done.

We discovered that the above don't work: the marker doesn't invalidate itself since it doesn't implement a non-lying getAbsoluteRepaintRect. (I'm noting this here because I've already tried this once before and forgotten why it didn't work).

Created attachment 7539 [details]
Turn list markers into well-behaved boxes

[Dave Hyatt]

Comment on attachment 7539 [details]
Turn list markers into well-behaved boxes

r=me.  This is great.

[Justin Garcia]

about to land this