Summary: | List item's bullets fail to redraw correctly after their style is set with JavaScript. | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Alexei Svitkine <myrd> | ||||||
Component: | Layout and Rendering | Assignee: | Nobody <webkit-unassigned> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | bry_fox, mitz | ||||||
Priority: | P2 | ||||||||
Version: | 420+ | ||||||||
Hardware: | Mac | ||||||||
OS: | OS X 10.4 | ||||||||
URL: | http://projectmagma.net/~myrdred/test.html | ||||||||
Attachments: |
|
Description
Alexei Svitkine
2005-09-05 11:25:41 PDT
Forgot to mention, but see http://projectmagma.net/~myrdred/test.html for this bug in action. Confirmed with WebKit 412.7 and ToT. Behaves as intended in Gecko-based browsers. Bug 6538 has another testcase: attachment 5674 [details]. Bug 7542 describes something similar that seems to be a regression. 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. 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
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 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.
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
Comment on attachment 7539 [details]
Turn list markers into well-behaved boxes
r=me. This is great.
about to land this |