428156 - ComputeRepaintRegionForCopy breaks when scrolling inside a clipped area

Status: RESOLVED FIXED

(Core :: Web Painting, defect)

Description

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

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 https://bugzilla.mozilla.org/show_bug.cgi?id=424915#c23 but now I'm not sure that's actually the reason for this bug.

Comment 1

[Martijn Wargers (dead)]

I think you forgot to upload the testcase.

Comment 2

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

Attachment: oops

Comment 3

[Martijn Wargers (dead)]

That looks more like a patch to me ;)

Comment 4

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

Attachment: oops^2

Comment 5

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

wanted1.9 since it's a regression from FF2.

Comment 6

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

Attachment: fix

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.

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 315727 [details] [diff] [review]
fix

>     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?

r+sr=dbaron

Comment 8

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

AFAIK it's the only place where we decide not to traverse a subtree that's visible.

Comment 9

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

Comment on attachment 315727 [details] [diff] [review]
fix

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.

Comment 10

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 315727 [details] [diff] [review]
fix

a=beltzner, this probably should have been a blocker

Comment 11

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

checked in

Comment 12

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

I backed this out to see if it would fix test failures. I doubt it's responsible, but I'm desperate.

Comment 13

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

This did not cause the test failures. I'll reland it.

Comment 14

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

Relanded

Comment 15

[Doug Wright]

Roc, is there any chance this could have caused crash a5985130-0ed5-11dd-a2cf-001cc45a2ce4 (browser area went solid black, then crashed)?

Comment 16

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

I suppose it's possible. Have you got a bug filed for that crash?