Created attachment 44495 [details] reduction Typing in Wave repaints the whole screen We've reduced the offending HTML. Seems we're laying out too much: <div style="height:100%; position:relative;"> <div style="position:absolute; left:0"> <div style="height: 200px"></div> <div contenteditable="true"> TYPE HERE AND SEE IT REPAINT THE WHOLE SCREEN </div> </div> </div> </div>
Reduced slightly further: <div style="position:absolute"> <div style="height: 200px"></div> <div contenteditable="true"> TYPE HERE AND SEE IT REPAINT THE WHOLE SCREEN </div> </div> </div>
http://livedom.validator.nu/?%3Cdiv%20style%3D%22position%3Aabsolute%3B%20left%3A0%22%3E%0A%20%20%3Cdiv%20style%3D%22height%3A200px%22%3E%3C%2Fdiv%3E%0A%20%20%3Cdiv%20contenteditable%3D%22true%22%3E%0A%20%20%20%20%20TYPE%20HERE%0A%20%20%3C%2Fdiv%3E%0A%3C%2Fdiv%3E Bizarrely enough adding <!DOCTYPE html> reduces the repaint rectangles (according to Quartz Debug) to be much closer to reasonable, although they seem a bit large.
The RenderObject corresponding to the root <div> that seems to generating the seemingly bad paint rectangle has childrenInline() set to true at the call to RenderBlock::layoutInlineChildren(), but firstChild()->isInline() is false. That seems bad - its only children are block elements, so I have no idea why childrenInline() would be returning true. I don't think RenderBlock::layoutInlineChildren() should be being called at all on this div.
Further reduction: <div style="position:absolute"> <div contenteditable="true"> TYPE HERE </div> </div> The entire <body> is getting marked for repaint because: RenderBlockLineLayout.cpp's RenderBlock::layoutInlineChildren() is called on the RenderBox for the <body>. This object's only child is the RenderBox for the position:absolute div, which is not itself inline but since it does not have a specified top:/left: does depend on its position in the natural flow. The check at line 826 is: http://trac.webkit.org/browser/trunk/WebCore/rendering/RenderBlockLineLayout.cpp#L824: bool fullLayout = !firstLineBox() || !firstChild() || selfNeedsLayout() || relayoutChildren; Since the <body> has no non-whitespace text content, it never has any line boxes and so the first check means that fullLayout is always true for this RenderBox. This means when the code hits line 888, it always repaints the RenderBox's layer()'s repaintRect - which is the size of the body. I believe the intent of this check is to ensure that fullLayout is set to true whenever line boxes need to be built but have not been yet. Removing the !firstLineBox() clause or replacing it with !m_everHadLayout causes fast/repaint/line-flow-with-floats-6.html to fail pixel tests as when the innerHTML of the float:left <span> is replaced fullLayout is not set. I think that the fullLayout flag should only be set when !firstLineBox() is true _and_ the RenderBox will need line boxes (i.e. has some inline content). I am not sure how to implement this efficiently. Does this sound correct?
Another fix would be to switch the default value of 'm_childrenInline' to 'false', so that when a RenderBlock has no in-flow children at all it uses the layoutBlockChildren() codepath instead of layoutInlineChildren(). This would also disambiguate the "need line boxes but have not generated them yet" and the "will never need line boxes" cases.
Created attachment 44772 [details] Strawman patch that adds a finer-grained check for the fullLayout flag This patch adds a check in RenderBlock::layoutInlineChildren() to try to differentiate the case of a RenderBlock having no firstLineBox() because line boxes need to be regenerated and having no firstLineBox() because it does not need any (i.e. all of its children are positioned or floats). I do not think this patch is the best possible solution and I think it needs some additional tests, but I'd appreciate feedback from people more knowledgeable about this code on the general approach. I think this codepath is actually getting hit quite often since this fairly common idiom will cause it: <div style="position:relative; height:600px; width:900px; overflow:hidden;" > <div style="position:absolute; height:600px; width:1000px; left:50px;"> stuff that should be trimmed </div> </div>
style-queue ran check-webkit-style on attachment 44772 [details] without any errors.
I think Hyatt or Dan should review this one.
Comment on attachment 44772 [details] Strawman patch that adds a finer-grained check for the fullLayout flag This can't be checked in as-is w/o a test of course. Also, I think that this should be moved into a nicely named static function to try and make this code more self-documenting.
Created attachment 44825 [details] better patch and test This patch is slightly better since it avoids doing another walk through the RenderBlock's children. I've also avoided changing when fullLayout is set in preference for changing when the RenderBlock is mutated. I'd greatly appreciate some input on what this code is supposed to be doing. There seem to be a lot of signals used to indicate that a RenderBlock needs to have some or all of its lineboxes regenerated: RenderBlock::m_needsLayout RenderBlock::firstLineBox() == NULL RenderText::m_linesDirty InlineBox::m_dirty but I can't figure out which should be set under which circumstances.
Created attachment 44832 [details] Cleans up !firstChild() case for fullLayout flag This patch cleans up one of the fullLayout cases but does not change any behavior.
style-queue ran check-webkit-style on attachment 44832 [details] without any errors.
Comment on attachment 44832 [details] Cleans up !firstChild() case for fullLayout flag If I am reading this correctly, then this patch is a cleanup that is not a fix for the cited bug. Please submit it with its own bug if possible.
Comment on attachment 44825 [details] better patch and test I suggest editing the CHangeLog to match the standard format. It should be: Typing in Wave repaints the whole screen https://bugs.webkit.org/show_bug.cgi?id=32295 Marks a RenderBlock without.... etc rather than putting the big long paragraph first and burying the number. Also the WebCore ChangeLog says "No new tests" rather than mentioning the new tests. Please fix these and resubmit. For what it's worth: the description of the change sounds sensible to me, but I can't readily tell if the code change does what the ChangeLog describes. Hopefully a layout expert can weigh in on that aspect.
Created attachment 45622 [details] Patch with correct ChangeLog format Thanks for the review. This fixes the formatting of the ChangeLog entries.
style-queue ran check-webkit-style on attachment 45622 [details] without any errors.
Created attachment 45829 [details] Patch with correct copyright line Has correct copyright line for 2010.
style-queue ran check-webkit-style on attachment 45829 [details] without any errors.
Comment on attachment 45829 [details] Patch with correct copyright line We talked about this in person, and I watched your conversation with Hyatt. This seems like a totally safe fix, for a clear bug. This whole loop definitely needs a bath, but I think that this fix is good and we should go with it. I think that you should discuss with Hyatt and Mitz further how we can improve this loop in the future. Thanks!
Comment on attachment 45829 [details] Patch with correct copyright line Clearing flags on attachment: 45829 Committed r52839: <http://trac.webkit.org/changeset/52839>
All reviewed patches have been landed. Closing bug.
Please land PNG from this patch. It wasn't part of the commit.
(In reply to comment #22) > Please land PNG from this patch. It wasn't part of the commit. The PNG was not in the patch, which is why it wasn't committed. James, can you provide the PNG?
Created attachment 45978 [details] Test expectation PNG Yes, I forgot to include the PNG in the patch. Attached here since I do not have commit rights.