In this testcase, the red stuff should be clipped by the scrollbox with the black border. With APZ enabled, this doesn't happen. The scroll frame has a display port and clears the clip state for its scrolled contents. However, during layer building it's ending up in an inactive BasicLayerManager (because it's inside an nsDisplaySVGEffects), so all display items are considered having the same active geometry root and get flattened into the same layer, and we ignore the scroll frame's clip because we never call ComputeFrameMetrics on it. I have a few ideas how we could fix this, ranked from hacky to less hacky: (1) Abuse nsDisplayBuilder::ShouldBuildScrollInfoItemsForHoisting to check in ScrollFrameHelper::BuildDisplayList whether we're inside an nsDisplaySVGEffects and then take the not-using-displayport code paths (1a) Same as (1), but rename ShouldBuildScrollInfoItemsForHoisting to IsInsideInactiveItem or !SupportsActiveLayersForCurrentItems or something like that (2) During layer building in the inactive layer manager, don't ignore a display item's active geometry root if it's a scroll frame. This would cause additional BasicPaintedLayers to be created and the scroll clip to be set on the BasicPaintedLayer and applied during BasicPaintedLayer composition (3) Similar to (2), but don't build separate layers and apply the scroll frame's clip on each display item during ProcessDisplayItems when nsLayoutUtils::GetAnimatedGeometryRootFor(item, mBuilder, mManager) is different from mContainerAnimatedGeometryRoot. This would result in one call to ComputeFrameMetrics per scrolled display item. (4) Similar to (3), but don't go through nsLayoutUtils::GetAnimatedGeometryRootFor and instead, during display list building, annotate each display item with a chain of scroll frames whose clips should be respected in the final composition of this item. Any other ideas? (4) would also give us a way to fix bug 1147673.

Isn't this similar to the lock screen notification scroller on B2G? When I dealt with that (by adding the original hoisting code) that worked because we did sync scrolling there, using the hoisted scrollinfo layer to create an APZ. I don't know much about how the code has evolved since then but it might be worth checking what's happening in the lockscreen case.

Oh actually my comment is probably irrelevant given this is a clipping change/issue. Never mind.

(In reply to Markus Stange [:mstange] from comment #0) > (3) Similar to (2), but don't build separate layers and apply the scroll > frame's clip on each display item during ProcessDisplayItems when > nsLayoutUtils::GetAnimatedGeometryRootFor(item, mBuilder, mManager) is > different from mContainerAnimatedGeometryRoot. This would result in one call > to ComputeFrameMetrics per scrolled display item. I'm not sure I understand. If we don't create separate layers then the clip can only be correct until we do async scrolling, no? > (4) Similar to (3), but don't go through > nsLayoutUtils::GetAnimatedGeometryRootFor and instead, during display list > building, annotate each display item with a chain of scroll frames whose > clips should be respected in the final composition of this item. What do we do with the annotation on the display item? How does that clipping information make its way to the compositor so the final clipping is correct?

(In reply to Timothy Nikkel (:tn) from comment #3) > (In reply to Markus Stange [:mstange] from comment #0) > > (3) Similar to (2), but don't build separate layers and apply the scroll > > frame's clip on each display item during ProcessDisplayItems when > > nsLayoutUtils::GetAnimatedGeometryRootFor(item, mBuilder, mManager) is > > different from mContainerAnimatedGeometryRoot. This would result in one call > > to ComputeFrameMetrics per scrolled display item. > > I'm not sure I understand. If we don't create separate layers then the clip > can only be correct until we do async scrolling, no? Those separate layers I was talking about would only be built in the BasicLayerManager, so they wouldn't be able to be async scrolled either way. So these things can't be async-scrolled either way. Only the layer that we flatten into can be async-scrolled, and that layer's scroll clips aren't affected by scroll clips inside the flattened nsDisplaySVGEffects. > > (4) Similar to (3), but don't go through > > nsLayoutUtils::GetAnimatedGeometryRootFor and instead, during display list > > building, annotate each display item with a chain of scroll frames whose > > clips should be respected in the final composition of this item. > > What do we do with the annotation on the display item? Something very similar to what my upcoming patch does with the item's animated geometry root - gather the clips that can't be applied by the compositor due to missing layerization and apply them to the display items during ContainerState::ProcessDisplayItems. > How does that > clipping information make its way to the compositor so the final clipping is > correct? That's the point here - the compositor can't apply these clips because there are no compositor layers for it, so we need to do the clipping before composition. I've implemented (3) because it seemed simplest and because almost all of the code for (3) would be reused in (4).

Bug 1178745 - Add an nsIScrollableFrame API for getting the scroll clip.

Bug 1178745 - Move some code around.

Bug 1178745 - Respect scroll clips when flattening.

The patches here are on top of those in bug 990974.

Comment on attachment 8629080 [details] MozReview Request: Bug 1178745 - Add an nsIScrollableFrame API for getting the scroll clip. https://reviewboard.mozilla.org/r/12527/#review11039 Ship It!

Comment on attachment 8629081 [details] MozReview Request: Bug 1178745 - Move some code around. https://reviewboard.mozilla.org/r/12529/#review11041 Ship It!

Comment on attachment 8629082 [details] MozReview Request: Bug 1178745 - Respect scroll clips when flattening. https://reviewboard.mozilla.org/r/12531/#review11043 Ship It!

https://hg.mozilla.org/integration/mozilla-inbound/rev/6069c09d8f46 https://hg.mozilla.org/integration/mozilla-inbound/rev/ed36e531514f https://hg.mozilla.org/integration/mozilla-inbound/rev/79b69023d18f

https://hg.mozilla.org/mozilla-central/rev/6069c09d8f46 https://hg.mozilla.org/mozilla-central/rev/ed36e531514f https://hg.mozilla.org/mozilla-central/rev/79b69023d18f

This added two overridden virtual methods without the override keyword that recent clang fatally warns about. I fixed that in: https://hg.mozilla.org/integration/mozilla-inbound/rev/c51f9fc02d33