Bug 9749

Summary:

A nested content editable div causes unnecessary screen redraws.

Product:

WebKit

Reporter:

Beth Dakin <bdakin>

Component:

Layout and Rendering

Assignee:

Beth Dakin <bdakin>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

mitz

Priority:

Keywords:

InRadar, Regression

Version:

420+

Hardware:

Mac

OS:

OS X 10.4

Attachments:

Description	Flags
Test case	none
patch	none
New patch	mjs: review+

Beth Dakin

Reported 2006-07-05 22:55:34 PDT

* STEPS TO REPRODUCE 1. Load the test case. 2. Use Quartz Debug and enable "Flash screen updates." 3. Type into the content editable div. * RESULTS Each key stroke causes the whole outer div (which is most of the window) to redraw. This is a regression from shipping Safari.

Attachments
Test case (694 bytes, text/html) 2006-07-05 22:56 PDT, Beth Dakin	no flags	Details
patch (678 bytes, patch) 2006-07-06 20:14 PDT, Beth Dakin	no flags	Details Formatted Diff Diff
New patch (629 bytes, patch) 2006-07-09 18:37 PDT, Beth Dakin	mjs: review+	Details Formatted Diff Diff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.

Beth Dakin

Comment 1 2006-07-05 22:56:55 PDT

Created attachment 9220 [details] Test case We are repainting the outer div, and we used to not do that. It is being repainted from RenderBlock::layoutInlineChildren(). We call repaintViewRectangle() with the layer's repaint rect. I doubt that the repaint rect is wrong -- it seems more likely that we shouldn't be calling it in the first place, but I am not sure since I don't know enough about the repaint code.

Beth Dakin

Comment 2 2006-07-05 22:58:12 PDT

Also the bug goes away if you take away the absolute positioning from either div.

Beth Dakin

Comment 3 2006-07-06 20:14:41 PDT

Created attachment 9243 [details] patch I feel like this patch could not possibly be right. But it definitely hunts down the source of the problem. I remembered fixing a bug a while ago where the root cause was that a bunch of our old code held assumptions that positioned elements would always have line boxes, but it turns out that this isn't true. This assumption is also causing extra repaints because in layoutInlineChildren() we set fullLayout to true if we don't have any line boxes...meaning that positioned elements without line boxes always get a full layout. This is clearly overzealous. I imagine that the patch I attached is undezealous. (underzealous?? forgive me, for it has been a long day.) But I figure that attaching it is a good way to start a discussion about what the right fix is. Thoughts?

Beth Dakin

Comment 4 2006-07-06 20:15:23 PDT

Oh by the way, all of the layout tests pass. But I am still not sure this is right.

Beth Dakin

Comment 5 2006-07-09 18:37:57 PDT

Created attachment 9320 [details] New patch So Maciej and I talked about this on IRC, and we couldn't figure out why the check for the firstLineBox was there at all. So this patch just removes that check entirey. It causes no layout test failures. We are assuming that its presense in determining if we need a full layout is antiquated.

Maciej Stachowiak

Comment 6 2006-07-09 18:40:22 PDT

Comment on attachment 9320 [details] New patch r=me May the lord have mercy on my soul if this was wrong.

Beth Dakin

Comment 7 2006-07-09 19:26:35 PDT

I committed this with r15273

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