371536 - Stop creating views for positioned elements

Status: RESOLVED FIXED

(Core :: Web Painting, defect)

Description

[Eli Friedman]

Attachment: WIP 1

Patch coming up.  Not a very big patch, but I haven't really been able to get it right. The patch works, but it invalidates too much, I think (not entirely sure how much more it invalidates than the old code, but in any case it's not very efficient.)

The nsContainerFrame change is obvious.

The nsAbsoluteContainingBlock changes make it do invalidation, which it wasn't doing before because the view took care of it.  I'm repainting way too much, though; suggestions?  (The frame should be able to invalidate itself, except that it doesn't know where it will end up until it finishes reflowing.  Is there anything we can do about that?)

The nsBlockFrame change is needed because otherwise dynamic clip changes don't invalidate enough.  Of course it breaks the optimization, but I'm not sure how to fix it.  I guess one way would be change http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/base/nsCSSFrameConstructor.cpp&rev=1.1316#9623 to do "aFrame->GetParent()->Invalidate(invalidRect)" if the frame has a parent.
It would invalidate too much for small clip changes, though.  Any better suggestions?  (I guess if we really wanted to invalidate the minimum amount, we'd have to add a special change hint for clipping changes; I'm not sure if it's worth it, though.)

My primary testcases so far have been calendar.google.com and http://www.e-gandalf.net/mozilla/bugs/140789/dynl-c31.html . I probably need to test relative and fixed positioning more.

Comment 1

[Eli Friedman]

Attachment: Patch v1

Okay, tried out the clip fix; seemed to work just fine, and I think it's correct.

Still not completely sure about the abs. pos. invalidation thing, but anything less seems to mess up somehow.

Comment 2

[Eli Friedman]

Oops, I accidentally included the patch to bug 371620; you can ignore that.

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

After reflowing an abs-pos frame, we need to repaint the union of the overflow areas only if the frame moved. (And it might be best to just Invalidate the two rectangles separately; if they moved a long way, the union might be much bigger than necessary.) If the frame didn't move then we only need to repaint the difference in the frame areas due to it changing size. The general invariant for frame repainting during reflow is that the parent frame does invalidations resulting from the child frame moving or changing size, and the child frame is responsible for everything else related to its frame subtree.

So this nsCSSFrameConstructor change fixes clipping?

Comment 4

[Eli Friedman]

Attachment: Patch v2

Okay, this should do it. Summary of changes:

The nsCSSFrameConstructor change tells the parent frame to invalidate the area instead of the frame itself because the frame itself can wrongly end up clipping out the invalidates. (Found in http://www.e-gandalf.net/mozilla/bugs/140789/dynl-c31.html)

The nsFrameManager change fixes a bug found on Google Calendar with setting a frame with overflowing positioned children to display: none.

The nsAbsoluteContaingBlock changes deal with invalidates caused by moves which were previously handled by the view.

And finally, the nsContainerFrame change stops creating views for abs. pos. frames.  (I'll remove the fix to bug 371620 before I check in.)

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Is the nsCSSFrameConstructor change safe when the child frame is a popup frame --- i.e. could we be redirecting the invalidate to the wrong widget? Maybe we need an IsFrameOfType check for popups which we can use here.

Other than that, it all looks good.

Comment 6

[Eli Friedman]

(In reply to comment #5)
> Is the nsCSSFrameConstructor change safe when the child frame is a popup frame
> --- i.e. could we be redirecting the invalidate to the wrong widget? Maybe we
> need an IsFrameOfType check for popups which we can use here.

I think the !HasView() check should be sufficient for the moment, since all popups have views; we'll need to come up with a better check at some point, though.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 256422 [details] [diff] [review]
Patch v2

of course

Comment 8

[Eli Friedman]

Checked in.