Bug 32295

Summary:

Typing in Wave repaints the whole screen

Product:

WebKit

Reporter:

Eric Seidel (no email) <eric>

Component:

Text

Assignee:

James Robinson <jamesr>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

commit-queue, darin, dglazkov, hyatt, jamesr, mitz, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Attachments:

Description	Flags
reduction	none
Strawman patch that adds a finer-grained check for the fullLayout flag	jamesr: commit-queue-
better patch and test	mjs: review-
Cleans up !firstChild() case for fullLayout flag	mjs: review-
Patch with correct ChangeLog format	none
Patch with correct copyright line	none
Test expectation PNG	none

Eric Seidel (no email)

Reported 2009-12-08 16:15:11 PST

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>

Attachments
reduction (287 bytes, text/html) 2009-12-08 16:15 PST, Eric Seidel (no email)	no flags	Details
Strawman patch that adds a finer-grained check for the fullLayout flag (2.05 KB, patch) 2009-12-13 18:07 PST, James Robinson	jamesr: commit-queue-	Details Formatted Diff Diff
better patch and test (5.90 KB, patch) 2009-12-14 15:05 PST, James Robinson	mjs: review-	Details Formatted Diff Diff
Cleans up !firstChild() case for fullLayout flag (2.05 KB, patch) 2009-12-14 18:58 PST, James Robinson	mjs: review-	Details Formatted Diff Diff
Patch with correct ChangeLog format (5.80 KB, patch) 2009-12-29 14:30 PST, James Robinson	no flags	Details Formatted Diff Diff
Patch with correct copyright line (6.15 KB, patch) 2010-01-04 14:32 PST, James Robinson	no flags	Details Formatted Diff Diff
Test expectation PNG (11.71 KB, image/png) 2010-01-06 12:23 PST, James Robinson	no flags	Details
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.

James Robinson

Comment 1 2009-12-09 16:54:10 PST

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>

James Robinson

Comment 2 2009-12-09 23:17:18 PST

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.

James Robinson

Comment 3 2009-12-10 00:09:01 PST

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.

James Robinson

Comment 4 2009-12-10 16:37:11 PST

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?

James Robinson

Comment 5 2009-12-13 17:34:57 PST

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.

James Robinson

Comment 6 2009-12-13 18:07:40 PST

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>

WebKit Review Bot

Comment 7 2009-12-13 18:10:29 PST

style-queue ran check-webkit-style on attachment 44772 [details] without any errors.

Darin Adler

Comment 8 2009-12-14 10:31:09 PST

I think Hyatt or Dan should review this one.

Eric Seidel (no email)

Comment 9 2009-12-14 10:43:16 PST

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.

James Robinson

Comment 10 2009-12-14 15:05:34 PST

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.

James Robinson

Comment 11 2009-12-14 18:58:41 PST

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.

WebKit Review Bot

Comment 12 2009-12-14 21:12:05 PST

style-queue ran check-webkit-style on attachment 44832 [details] without any errors.

Maciej Stachowiak

Comment 13 2009-12-29 05:20:02 PST

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.

Maciej Stachowiak

Comment 14 2009-12-29 05:24:57 PST

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.

James Robinson

Comment 15 2009-12-29 14:30:17 PST

Created attachment 45622 [details] Patch with correct ChangeLog format Thanks for the review. This fixes the formatting of the ChangeLog entries.

WebKit Review Bot

Comment 16 2009-12-29 14:34:17 PST

style-queue ran check-webkit-style on attachment 45622 [details] without any errors.

James Robinson

Comment 17 2010-01-04 14:32:55 PST

Created attachment 45829 [details] Patch with correct copyright line Has correct copyright line for 2010.

WebKit Review Bot

Comment 18 2010-01-04 14:38:10 PST

style-queue ran check-webkit-style on attachment 45829 [details] without any errors.

Eric Seidel (no email)

Comment 19 2010-01-05 16:40:52 PST

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!

WebKit Commit Bot

Comment 20 2010-01-05 17:15:33 PST

Comment on attachment 45829 [details] Patch with correct copyright line Clearing flags on attachment: 45829 Committed r52839: <http://trac.webkit.org/changeset/52839>

WebKit Commit Bot

Comment 21 2010-01-05 17:15:41 PST

All reviewed patches have been landed. Closing bug.

Dimitri Glazkov (Google)

Comment 22 2010-01-06 09:01:13 PST

Please land PNG from this patch. It wasn't part of the commit.

Darin Adler

Comment 23 2010-01-06 09:39:45 PST

(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?

James Robinson

Comment 24 2010-01-06 12:23:30 PST

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.

Note You need to log in before you can comment on or make changes to this bug.