Closed Bug 371536 Opened 18 years ago Closed 18 years ago

Stop creating views for positioned elements

Categories

(Core :: Web Painting, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: sharparrow1, Assigned: sharparrow1)

References

Details

Attachments

(1 file, 2 obsolete files)

Attached patch WIP 1 (obsolete) — Splinter Review
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.
Attached patch Patch v1 (obsolete) — Splinter Review
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.
Attachment #256299 - Attachment is obsolete: true
Attachment #256388 - Flags: review?(roc)
Oops, I accidentally included the patch to bug 371620; you can ignore that.
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?
Attached patch Patch v2Splinter Review
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.)
Attachment #256388 - Attachment is obsolete: true
Attachment #256422 - Flags: review?(roc)
Attachment #256388 - Flags: review?(roc)
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.
(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 on attachment 256422 [details] [diff] [review]
Patch v2

of course
Attachment #256422 - Flags: superreview+
Attachment #256422 - Flags: review?(roc)
Attachment #256422 - Flags: review+
Checked in.
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Depends on: 373433
Blocks: 254910
Depends on: 386391
Blocks: 241638
Depends on: 408656
Component: Layout: View Rendering → Layout: Web Painting
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: