Bug 4855 - List item's bullets fail to redraw correctly after their style is set with JavaScript.
Summary: List item's bullets fail to redraw correctly after their style is set with Ja...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 420+
Hardware: Macintosh OS X 10.4
: P2 Normal
Assignee: Nobody
URL: http://projectmagma.net/~myrdred/test...
Keywords:
: 5455 6538 (view as bug list)
Depends on:
Blocks:
 
Reported: 2005-09-05 11:25 PDT by Alexei Svitkine
Modified: 2006-04-12 16:19 PDT (History)
2 users (show)

See Also:


Attachments
Patch (10.25 KB, patch)
2006-03-16 16:41 PST, mitz
hyatt: review-
Details | Formatted Diff | Diff
Turn list markers into well-behaved boxes (392.06 KB, patch)
2006-04-06 08:00 PDT, mitz
hyatt: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexei Svitkine 2005-09-05 11:25:41 PDT
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.
Comment 1 Alexei Svitkine 2005-09-05 11:30:11 PDT
Forgot to mention, but see http://projectmagma.net/~myrdred/test.html for this bug in action.
Comment 2 Mark Rowe (bdash) 2005-09-06 23:01:24 PDT
Confirmed with WebKit 412.7 and ToT.  Behaves as intended in Gecko-based browsers.
Comment 3 mitz 2006-02-08 13:07:33 PST
*** Bug 6538 has been marked as a duplicate of this bug. ***
Comment 4 mitz 2006-02-08 13:12:00 PST
Bug 6538 has another testcase: attachment 5674 [details].
Comment 5 mitz 2006-02-21 21:57:28 PST
See also bug 6844
Comment 6 Darin Adler 2006-03-02 07:13:09 PST
Bug 7542 describes something similar that seems to be a regression.
Comment 7 Justin Garcia 2006-03-02 12:32:06 PST
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. 
Comment 8 mitz 2006-03-09 03:13:08 PST
*** Bug 5455 has been marked as a duplicate of this bug. ***
Comment 9 mitz 2006-03-16 09:32:23 PST
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).
Comment 10 mitz 2006-03-16 16:41:10 PST
Created attachment 7120 [details]
Patch
Comment 11 Justin Garcia 2006-03-16 19:16:22 PST
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) {
Comment 12 Dave Hyatt 2006-03-17 01:22:20 PST
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.
Comment 13 Dave Hyatt 2006-03-17 01:31:51 PST
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. ;)
Comment 14 mitz 2006-03-17 05:20:06 PST
(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).
Comment 15 mitz 2006-04-06 08:00:18 PDT
Created attachment 7539 [details]
Turn list markers into well-behaved boxes
Comment 16 Dave Hyatt 2006-04-11 12:11:58 PDT
Comment on attachment 7539 [details]
Turn list markers into well-behaved boxes

r=me.  This is great.
Comment 17 Justin Garcia 2006-04-12 16:19:56 PDT
about to land this