Closed Bug 428156 Opened 15 years ago Closed 15 years ago

ComputeRepaintRegionForCopy breaks when scrolling inside a clipped area


(Core :: Web Painting, defect)

Not set





(Reporter: roc, Assigned: roc)



(Keywords: regression, testcase)


(2 files, 1 obsolete file)

See testcase.

Open the testcase, click inside the scrollable area, and then use down-arrow to scroll down to the bottom. The text "Hello" should scroll into view, but it doesn't appear to. If you force a repaint (e.g. by resizing the window), it appears.

I created this testcase to try to confirm my suspicions in but now I'm not sure that's actually the reason for this bug.
I think you forgot to upload the testcase.
Attached file oops (obsolete) —
That looks more like a patch to me ;)
Attached file oops^2
Attachment #314965 - Attachment is obsolete: true
wanted1.9 since it's a regression from FF2.
Flags: wanted1.9+
Keywords: regression, testcase
Attached patch fixSplinter Review
Three things in this patch:

-- Make nsDisplayClip track which frame is doing the clipping (so later we can tell if it's moving or not)

-- When we decide we don't need to traverse the descendants of the moving frame, add "summary display items" to represent what could have been painted by those descendants. This prevents various display list optimizations throwing out information we need later.

-- Make AddItemsToRegion handle the case of non-moving frames clipping moving frames.
Attachment #315727 - Flags: superreview?(dbaron)
Attachment #315727 - Flags: review?(dbaron)
Whiteboard: [needs review]
Comment on attachment 315727 [details] [diff] [review]

>     if (aBuilder->GetRootMovingFrame() == this &&
>         !PresContext()->GetRenderedPositionVaryingContent()) {
>       // No position-varying content has been rendered in this prescontext.
>       // Therefore there is no need to descend into analyzing the moving frame's
>       // descendants looking for such content, because any bitblit will
>-      // not be copying position-varying graphics.
>+      // not be copying position-varying graphics. However, to keep things
>+      // sane we still need display items representing the frame subtree.
>+      // We need to add these summaries to every list that the child could
>+      // contribute to. This avoids display list optimizations optimizing
>+      // away entire lists because they appear to be empty.

I'm a little surprised this is the only place where we optimize away lists such that nsDisplaySummary items are needed.  Are there really no other such optimizations, or do they really not have the same problems?

Attachment #315727 - Flags: superreview?(dbaron)
Attachment #315727 - Flags: superreview+
Attachment #315727 - Flags: review?(dbaron)
Attachment #315727 - Flags: review+
AFAIK it's the only place where we decide not to traverse a subtree that's visible.
Comment on attachment 315727 [details] [diff] [review]

This fixes a gaping hole in our code that manages invalidation due to scrolling. I don't know of any real sites that are being affected, but IMHO it's a major 1.9 regression.

The patch is quite safe because it's designed to just make us invalidate more. It might slow down our scrolling in some cases but I've tested troublesome pages like GMail and they have not regressed.
Attachment #315727 - Flags: approval1.9?
Comment on attachment 315727 [details] [diff] [review]

a=beltzner, this probably should have been a blocker
Attachment #315727 - Flags: approval1.9? → approval1.9+
checked in
Closed: 15 years ago
Resolution: --- → FIXED
Whiteboard: [needs review]
I backed this out to see if it would fix test failures. I doubt it's responsible, but I'm desperate.
Resolution: FIXED → ---
This did not cause the test failures. I'll reland it.
Whiteboard: [needs landing]
Closed: 15 years ago15 years ago
Resolution: --- → FIXED
Whiteboard: [needs landing]
Roc, is there any chance this could have caused crash a5985130-0ed5-11dd-a2cf-001cc45a2ce4 (browser area went solid black, then crashed)?
I suppose it's possible. Have you got a bug filed for that crash?
Component: Layout: View Rendering → Layout: Web Painting
You need to log in before you can comment on or make changes to this bug.