So the contents is run off the scrolling element. You can easily reproduce this issue on Scroll bar test in UI tests. See a screenshot of the Scroll bar test I am attaching.

Another strange thing about this page is that in addition to being able to be overscrolled horizontally, which is expected, the divs can be overscrolled vertically, which is unexpected - since the page is scrollable, they should be handing off their overscroll to the page, and the page should overscroll.

(In reply to Botond Ballo [:botond] from comment #1) > Another strange thing about this page is that in addition to being able to > be overscrolled horizontally, which is expected, the divs can be > overscrolled vertically, which is unexpected - since the page is scrollable, > they should be handing off their overscroll to the page, and the page should > overscroll. This is caused by the scroll parents being set incorrectly. I filed bug 1039623 for it.

Based on some initial investigation, I believe this is unrelated to CSS transforms, but indicates something more fundamentally wrong with the APZ overscroll transform.

It seems this is a general issue with async scaling of layers that are not at the origin. It appears that ContainerLayers are always at the origin, and are clipped to their actual area. ThebesLayers inside them have a translation transform that moves them from the origin to where they actually start. For example, consider a ThebesLayer whose content is at y=300. Assume it's the only child of a ContainerLayer. The ContainerLayer starts at y=0, but is clipped to start at y=300. The ThebesLayer, meanwhile, has a transform of y=300 relative to the ContainerLayer. When we zoom something asynchronously (whether by pinch-zooming, or for the overscroll effect), an async scale is placed on the ContainerLayer. Imagine that in our example above we zoom the layer out by 2x, so it gets an async transform with scale 0.5. After the scale, its origin is still at y=0. When the y=300 translation is then applied to the ThebesLayer, that is composed with the 0.5 scale, yielding a y=150 translation. As a result, the ThebesLayer is now misplaced on the page. This problem does not arise during pinch-zooming, because pinch-zooming is restricted to the root scrollable element, whose content is at the origin. However, overscrolling also imposes an async scale, and it applies to subframes, which are not necessarily at the origin, so we run into this problem. Matt Woodrow suggested the following suggestion: when applying the async scale to the ContainerLayer (presumably in AsyncCompositionManager), first translate the layer so that its content starts at the origin, then apply the scale, and then translate back.

This patch implements Matt Woodrow's suggestion from the previous comment.

Try push: https://tbpl.mozilla.org/?tree=Try&rev=4065a71ad092

Comment on attachment 8459830 [details] [diff] [review] bug1035598.patch Review of attachment 8459830 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/composite/AsyncCompositionManager.cpp @@ +519,5 @@ > + // apply the tree transform, and translate back. > + if (!(result.Is2D() && result.As2D().IsTranslation())) { > + if (auto* shadowClipRect = aLayer->AsLayerComposite()->GetShadowClipRect()) { > + Matrix originToClip = Matrix::Translation(shadowClipRect->x, shadowClipRect->y); > + if (!originToClip.IsIdentity()) { // avoid gratuitous matrix multiplications check shadowClipRect.TopLeft() == LayerPoint?() rather than checking the matrix. Not a huge fan of auto in this case since it's not clear what type to use for the point comparison. @@ +522,5 @@ > + Matrix originToClip = Matrix::Translation(shadowClipRect->x, shadowClipRect->y); > + if (!originToClip.IsIdentity()) { // avoid gratuitous matrix multiplications > + Matrix clipToOrigin = originToClip; > + if (clipToOrigin.Invert()) { > + result = Matrix4x4::From2D(clipToOrigin) * result * Matrix4x4::From2D(originToClip); I think you should use nsLayoutUtils::ChangeMatrixBasis for this, either by converting it to Matrix4x4 or adding an overloaded version. Doing a full matrix inversion and two matrix multiplies is going to be a lot slower than just applying a pre/post translate.

We could probably move ChangeMatrixBasis into gfxUtils too (or even the matrix class itself), I think it makes more sense there.

Comment on attachment 8459830 [details] [diff] [review] bug1035598.patch Review of attachment 8459830 [details] [diff] [review]: ----------------------------------------------------------------- While you're in here, can you also s/treeTransform/asyncTransform/? I don't think we use "tree transform" anywhere else in the code or docs and it's confusing. Matt's review is worth more than mine here, I don't know this code too well. Also watch out for when I land bug 1040906 as might have to rebase.

This patch addresses all review comments, including: - using ChangeBasis() instead of Invert() and multiplications - using the point instead of the translation matrix to check for identity - removing the use of 'auto' - renaming 'treeTransform' to 'asyncTransform'

Another Try push to make sure the refactoring didn't mess anything up: https://tbpl.mozilla.org/?tree=Try&rev=36616c0ac9db

Comment on attachment 8460590 [details] [diff] [review] Part 4 - Apply async scale correctly to ContainerLayers whose content does not start at the origin Review of attachment 8460590 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/composite/AsyncCompositionManager.cpp @@ +518,5 @@ > + // to the clipped area. If the tree transform includes a scale component, > + // then applying it to container as-is will produce incorrect results. In > + // such cases, translate the layer so that the clip rect starts at the origin, > + // apply the tree transform, and translate back. > + if (!(result.Is2D() && result.As2D().IsTranslation())) { The Is2D/As2D checks are probably more expensive than just doing ChangeBasis for translation-only transforms. Might as well drop it and keep the code simple.

Updated Part 4 patch to perform the change of basis even if the async transform is translation-only. Carrying r+.

https://hg.mozilla.org/integration/mozilla-inbound/rev/ff19c6d84718 https://hg.mozilla.org/integration/mozilla-inbound/rev/ffcb807fb0fa https://hg.mozilla.org/integration/mozilla-inbound/rev/e3085fc5b30d https://hg.mozilla.org/integration/mozilla-inbound/rev/558b722b4872

https://hg.mozilla.org/mozilla-central/rev/ff19c6d84718 https://hg.mozilla.org/mozilla-central/rev/ffcb807fb0fa https://hg.mozilla.org/mozilla-central/rev/e3085fc5b30d https://hg.mozilla.org/mozilla-central/rev/558b722b4872

[Blocking Requested - why for this release]: [Feature/regressing bug #]: Overscrolling (bug 1020045). [User impact if declined]: The overscroll effect is wrong for scrollable elements not at the origin. The further away the element is from the origin, the worse it looks. [Describe test coverage new/current, TBPL]: On master for 3 weeks. [Risks and why]: Touching AsyncCompositionManager code is not particularly low-risk, but this patch has been on master for 3 weeks without any reported regressions. [String/UUID change made/needed]: None.

Discussed in triage - the risk outweighs the win at this point due to branch code delta, and the impact seems low given the relative popularity of side scrolling mobile web content, so not blocking.

Just to clarify, in case the title may have confused things: this bug affects both horizontal and vertical overscrolling of elements whose origin is not at the origin of the screen.