I'm presuming this is a regression from retained layers (bug 564991). Steps to reproduce: 1. load attachment 478026 [details] 2. make the window small enough (in height) to scroll 3. scroll the browser window down with the scrollbar so only a small slice of the upper blue element is visible 4. move the mouse over that visible part of the upper blue element 5. use the scrollwheel to scroll more of that element into view Expected results: 4. element turns from blue to fuchsia 5. parts scrolled into view are also fuchsia (as long as the pointer is in the element) Actual results: 4. same as expected 5. parts scrolled into view are blue (It wouldn't surprise me if this is the same issue as some of the other regressions marked as dependencies of bug 564991, but this is a particularly simple testcase so it may be useful.)

Confirmed that this regressed between Linux x86_64 nightlies 2010-07-15-03-mozilla-central and 2010-07-16-03-mozilla-central, as expected.

I can reproduce this some of the time on Windows, with a small horizontal blue strip being painted.

I guess we need to not intersect the invalidate rect with the scroll port for the purposes of invalidating thebes layers?

Correct!!!

So I guess we need to pass both the intersected and non-intersected rect up the invalidate chain then?

Why? If there's a layer associated with the scrollframe, then you need to invalidate appropriately in that layer, but why would anything above it care?

I think what Timothy's thinking is that InvalidateInternal on the scrolled frame should have invalidated the layer before InvalidateInternal on the scrollframe restricted the invalid rect.

For some reason I can only reproduce this with smooth scrolling enabled.

(In reply to comment #7) > I think what Timothy's thinking is that InvalidateInternal on the scrolled > frame should have invalidated the layer before InvalidateInternal on the > scrollframe restricted the invalid rect. Yeah, but the scrollframe doesn't have a container layer. I think we need to do FrameLayerBuilder::InvalidateThebesLayerContents(frame, r) where r is the unrestricted rect, and frame is the frame that has the container layer for the scrollframe.

ah, you mean the scrolled frame doesn't have a container layer. Right. Don't forget to fix nsXULScrollFrame::InvalidateInternal as well.

So this fixes the problem and passes try server. It's a little more complicated than I wanted. What do you think of this?

Instead, how about in nsHTMLScrollFrame::InvalidateInternal, if scrollport clipping changes the invalid rect from the scrolled child and mScrollingActive, walk up to the nearest frame with a container layer and do an InvalidateThebesLayerContents on it?

We'd lose the special processing we get with the other implementations of InvalidateInternal. block frame's implementation has an optimization, but I think SVG effects and transforms rely on it for correctness.

But those have (active or inactive) layers of their own, so scrolled-out-of-view stuff can't be cached in a retained ThebesLayer if there's an SVG effect or transform around it.

Unless I'm missing something, I don't think SVG effects get their own layer currently.

We create a BasicLayerManager for the affected subtree, and then render all that content into the destination. Again, there can be no retained ThebesLayer caching content that is clipped out by scrollframes under SVG effects.

Hmm, I think I'm wrong here. How about this: if scrolling is active and we need to invalidate ThebesLayers, just don't clip invalidation to the scrollport?

I guess that's bad.

What if we had a flag to invalidate ONLY ThebesLayers? Then when we clip to the scrollport, split the invalidation into two parts: invalidation of ThebesLayers, which uses the unclipped rect, and invalidation of the window, which uses the clipped rect. We can stop propagating when we get to a point where both "only ThebesLayers" and "no ThebesLayers" are set. That's similar to what you've got, but with less code changes.

Yes, I like that, that's better.

Comment on attachment 482797 [details] [diff] [review] Part 3. The real fix. + aFlags | seperateThebes ? INVALIDATE_NO_THEBES_LAYERS : 0); It's not immediately obvious to me how the precedence works here, so for clarity please put parens around the ?: expression. Is it possible to write a reasonable test here?

(In reply to comment #24) > + aFlags | seperateThebes ? INVALIDATE_NO_THEBES_LAYERS : 0); > > It's not immediately obvious to me how the precedence works here, so for > clarity please put parens around the ?: expression. Good call. > Is it possible to write a reasonable test here? I think so. I remembered after posting the patches that I should write a test for this.

(In reply to comment #25) > (In reply to comment #24) > > + aFlags | seperateThebes ? INVALIDATE_NO_THEBES_LAYERS : 0); > > > > It's not immediately obvious to me how the precedence works here, so for > > clarity please put parens around the ?: expression. > > Good call. Indeed, the parens are needed to make it correct!

Here is the test I plan to add.

http://hg.mozilla.org/mozilla-central/rev/d8485927c4cc http://hg.mozilla.org/mozilla-central/rev/bed182d7f22d http://hg.mozilla.org/mozilla-central/rev/8188183a6402 http://hg.mozilla.org/mozilla-central/rev/417a9fc988ec