516885 - Scrolling inside a div containing block elements with margins leaves visual glitches

Status: RESOLVED FIXED

(Core :: Web Painting, defect, P2)

Description

[Mathieu Pellerin]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a2pre) Gecko/20090915 Namoroka/3.6a2pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a2pre) Gecko/20090915 Namoroka/3.6a2pre

Scrolling inside a div containing block elements with margins leaves visual glitches in between blocks.

Reproducible: Always

Steps to Reproduce:
1. Open the test case attached to this bug
2. Scroll the div region with the mouse wheel back and forth

Actual Results:  
Visual glitches appears in between the p elements

Expected Results:  
No visual glitches.

Disabling smooth scrolling makes it easier to see glitches.

Comment 1

[Mathieu Pellerin]

Attachment: test case

Comment 2

[Mathieu Pellerin]

Attachment: screenshot of test case

Comment 3

[Ria Klaassen (not reading all bugmail)]

Confirmed on Windows Vista with latest trunk. Not reproducible on Shiretoko.
These kinds of bugs are currently mostly for :roc.

Comment 4

[Mathieu Pellerin]

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2a2pre) Gecko/20090911 Namoroka/3.6a2pre

Also visible on linux builds.

Comment 5

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

Attachment: fix

The immediately problem was confusion over the interpretation of aVisibleRegion in ComputeVisibility. The underlying problem is that we're overloading aVisibleRegion to mean the union of the visible region before scrolling and the visible region after scrolling, but not doing it consistently, which is partly because that overloading is just really confusing.

This patch simplifies the code (I think) by passing two regions: aVisibleRegion for the region visible after the move, and aVisibleRegionBeforeMove for the region visible before the move. All the ComputeVisibility methods are updated to deal with both regions --- I think the resulting logic is clearer --- and this bug goes away.

Comment 6

[Ashley Bischoff (blog at handcoding.com)]

-> NEW (since there's a patch)

Comment 7

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

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

+  // If we're analyzing moving content, then it doesn't really matter
+  // what we set mNeedAlpha to, so let's conservatively set it to true so
+  // we don't have to worry about getting it "correct" for the moving case.

So analyzing moving content is always separate from painting (i.e.,
different display lists)?

(I'm a little rusty on this...)

Should you assert in ComputeVisibility (which?) that
aVisibleRegionBeforeMove being non-null is equivalent to
aBuilder->HasMovingFrames()?  Or are they equivalent?

Also, I'm a little worried about null references getting passed to
AccumulateVisibleRegionOfMovingContent; have you checked that that can't
happen?


I like these changes; I think they make things clearer.  Sorry for the delay in getting to them.  r=dbaron

Comment 8

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

(In reply to comment #7)
> (From update of attachment 404752 [details] [diff] [review])
> +  // If we're analyzing moving content, then it doesn't really matter
> +  // what we set mNeedAlpha to, so let's conservatively set it to true so
> +  // we don't have to worry about getting it "correct" for the moving case.
> 
> So analyzing moving content is always separate from painting (i.e.,
> different display lists)?

That is currently true, yes. In principle we could make scrolling a bit more efficient by reusing the list for painting, but I don't expect to do that anytime soon. Note that setting mNeedAlpha to true wouldn't break that usage, just be a tiny deoptimization.

> Should you assert in ComputeVisibility (which?) that
> aVisibleRegionBeforeMove being non-null is equivalent to
> aBuilder->HasMovingFrames()?  Or are they equivalent?

They should be equivalent. I'll assert this in every ComputeVisibility implementation.

> Also, I'm a little worried about null references getting passed to
> AccumulateVisibleRegionOfMovingContent; have you checked that that can't
> happen?

There are only two call sites. The one in nsDisplayList::ComputeVisibility passes in local regions, so they can't be null. The one in nsDisplayClip::ComputeVisibility dereferences *aVisibleRegionBeforeMove, which can't be null because we're guarded by if (aBuilder->HasMovingFrames()).

Comment 9

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

http://hg.mozilla.org/mozilla-central/rev/e95541e43a26

Comment 10

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

Attachment: fix assertion due to nsDisplayTransform

nsDisplayTransform incorrectly passes a non-null aVisibleRegionBeforeMove even in non-scrolling situations. This is harmless but triggers assertions.

Comment 11

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

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/4f82b0ff1a78

Comment 12

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

Comment on attachment 409183 [details] [diff] [review]
fix assertion due to nsDisplayTransform

r=dbaron (but maybe wrap at less than 80 characters?)

Comment 13

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

I landed the assertion fix on mozilla-central:
http://hg.mozilla.org/mozilla-central/rev/c69a51243e06

We should probably land it on 1.9.2 as well, perhaps?

Comment 14

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

Yes.

Comment 15

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

assertion fix: http://hg.mozilla.org/releases/mozilla-1.9.2/rev/4fb28b7ec57b