Bug 8969 – REGRESSION: typing in textfield repaints whole web page at gamefaqs.com

• Component: Forms
• Status: RESOLVED FIXED

[Dave Hyatt]

Another one of these repaint problems.  Typing in the search field on www.gamefaqs.com repaints nearly the whole page.  Turn on paint flashing to see the problem.

Created attachment 8392 [details]
Reduction

This reduction shows full-page repainting, but with any text field you'll see some repainting outside the field, i.e. even <input> on its own on the page repaints outside for no apparent reason. I think that's the real problem.

Created attachment 8394 [details]
It can get sluggish because of layout

This test case is exaggerated, but on a 1.8GHz iMac G5 the native text fields are noticeably slower even under normal circumstances. The problem illustrated by this test case is that typing in the text field causes layout all the way down from the frame to the inner div. Even with only children needing layout, relayout of the ancestors takes a while and causes unnecessary repainting.

A way to schedule relayout of only a subtree would be helpful in this case, where you know that changes couldn't have propagated outside the input element (and perhaps in a few other cases to be identified later).

Comment on attachment 8394 [details]
It can get sluggish because of layout

In shipping Safari (old text field implementation) both text fields are fast.

Created attachment 8404 [details]
Relayout only the subtree rooted at the text field

First cut at a patch for the relayout issue.

Created attachment 8405 [details]
Relayout only the subtree rooted at the text field

Created attachment 8434 [details]
Test case for the repaint issue

When you click Test, the entire 400px-tall div is repainted.

Comment on attachment 8405 [details]
Relayout only the subtree rooted at the text field

I think the approach in the patch can be generalized to all clipping containers.

The repaint is caused by layoutInlineChildren:

    // FIXME: Do something better when floats are present.
    bool fullLayout = !firstLineBox() || !firstChild() || selfNeedsLayout() || relayoutChildren || containsFloats();

...and...

        if (hasFloat)
            fullLayout = true; // FIXME: Will need to find a way to optimize floats some day.

(once fullLayout is true, the block is marked as needing layout, so everything gets repainted; I think that's actually the only effect of marking at that stage).

[Dave Hyatt]

The patch looks good.  One thing I thought of though...

The recalcStyle step that is done only on the textfield node.  I don't think you can get away with that, since if an ancestor style has changed, and you inherit from that ancestor, then doing a style recalc only on yourself will lead to you having incorrect style information while doing the layout.  I think you have to just do the style recalc on the whole document.

Created attachment 8481 [details]
Revised patch

Created attachment 8483 [details]
Revised patch

[Dave Hyatt]

Comment on attachment 8483 [details]
Revised patch

This is ready for text fields.  I do see one issue if we do try to extend this to overflow sections...

-    root->updateWidgetPositions();
+    if (!subtree)
+        static_cast<RenderCanvas*>(root)->updateWidgetPositions();

If the subtree contains widgets, then you'll need to run updateWidgetPositions but only on the widgets in the subtree.

Created attachment 8565 [details]
Updated patch

Added an explicit layout request in RenderContainer::removeChildNode, since otherwise layout wouldn't happen when a text field is removed. This was actually detected by fast/forms/textfield-focus-out.html but I overlooked it!

Comment on attachment 8483 [details]
Revised patch

Clearing review flag to remove bug from the commit queue.

[Dave Hyatt]

I don't quite understand the RenderContainer addition.  If a layout is targeted at the textfield itself, that should not be constrained to the subtree, so why is there a problem?  The only layouts that should be constrained to subtrees are those that are scheduled by the children/descendants of the text field.

Created attachment 8570 [details]
Patch

> If a layout is targeted at the textfield itself, that should
> not be constrained to the subtree
Now *that* was the real problem in all previous versions of the patch. I fixed it by moving the isTextField check in markContainingBlocksForRelayout so that it won't check the layout target, and everything seems to work.

[Dave Hyatt]

Comment on attachment 8570 [details]
Patch

r=me