Botond Ballo [:botond] Assignee
	Description • 8 years ago

In bug 1214151, we fixed the async scroll adjustment of position:fixed elements inside position:sticky elements in cases where the async scroll transform does not cause the sticky element to cross the boundary where it transitions from fixed to scrolling or vice versa (that is, in cases where the sticky element is either in the fixed state both before and after the async scroll, or in the scrolling state both before and after the async scroll).

In the case where an async scroll does cross this boundary, additional work needs to be done. That's what this bug is tracking.

A rough outline of the changes to AsyncCompositionManager::AlignFixedAndStickyLayers that need to be done for this:

  - Where we use IntervalOverlap() to clamp a proposed translation,
    take note of the difference between the original and the clamped
    values (this is the "unconsumed" portion of the translation).

  - If some but not all of the translation is consumed (as happens
    when we cross the boundar), come up with a new value of
    |aCurrentTransformForRoot| that reflects the unconsumed portion.
    This basically involves performing all the transformations that
    were done to derive the original translation from
    |aCurrentTransformForRoot| and |aPreviousTransformForRoot|,
    in reverse (keeping |aPreviousTransformForRoot| fixed).

  - Traverse the subtree rooted at the sticky layer, using the new
    value of |aCurrentTransformForRoot|. (Note that this means we
    either can't use ForEachNode() any longer, or we have to keep
    the |aCurrentTransformForRoot| values in a separate stack, or
    we have to make an explicit recursive call with the new value
    of |aCurrentTransformForRoot| while returning Traversal::Skip.)

	Botond Ballo [:botond] Assignee
	Comment 1 • 8 years ago

(In reply to Botond Ballo [:botond] from comment #0)
>     This basically involves performing all the transformations that
>     were done to derive the original translation from
>     |aCurrentTransformForRoot| and |aPreviousTransformForRoot|,
>     in reverse (keeping |aPreviousTransformForRoot| fixed).

Markus pointed out in bug 1214151 comment 13 that there might be an opportunity to simplify some of these transfomations because:

> AFAIK the root scroll frame container is the only scenario where 
> there can be a transform between a fixed layer and the first 
> unscrolled ancestor layer

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 2 • 8 years ago

Not sure if this should be a P2 or P3, depends on how common this case is. Marking it P2 for now, feel free to bump it to P3 if you feel that's more appropriate.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 3 • 8 years ago

Do we have a testcase for this floating around somewhere?

	Markus Stange [:mstange]
	Comment 4 • 8 years ago

attachment 8673036 [details], or the page linked in bug 1300864.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 5 • 8 years ago

Thanks!

	Botond Ballo [:botond] Assignee
	Comment 6 • 8 years ago

I'm just going to fix this.

	Botond Ballo [:botond] Assignee
	Comment 7 • 8 years ago

I have some WIP patches that need some cleanup before they're ready for review.

For now, I'm doing a Try push with them to make sure we're not breaking any reftests:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8791a6ea71ca

	Botond Ballo [:botond] Assignee
	Comment 8 • 8 years ago

Werror bustage. Not sure why my local build didn't catch it, I had --enable-warnings-as-errors in my mozconfig...

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5794167bec42

	Botond Ballo [:botond] Assignee
	Comment 9 • 8 years ago

Moar bustage...

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a980978b8a73

	Botond Ballo [:botond] Assignee
	Comment 18 • 8 years ago

I took the opportunity to clean up AlignFixedAndStickyLayers a bit further along the way. I also made the simplifications Markus suggested in comment 1.

For a couple of the patches, I'm asking Kats to review as well because they're a bit more risky in terms of regression potential.

	Markus Stange [:mstange]
	Comment 19 • 8 years ago
mozreview-review

Comment on attachment 8795029 [details]
Bug 1288210 - Improve the use of strongly typed units in AsyncCompositionManager.

https://reviewboard.mozilla.org/r/81216/#review79794

	Markus Stange [:mstange]
	Comment 20 • 8 years ago
mozreview-review

Comment on attachment 8795030 [details]
Bug 1288210 - Fixed a bug in how the ancestor transform is computed in AlignFixedAndStickyLayers.

https://reviewboard.mozilla.org/r/81218/#review79796

	Markus Stange [:mstange]
	Comment 21 • 8 years ago
mozreview-review

Comment on attachment 8795031 [details]
Bug 1288210 - Simplify AlignFixedAndStickyLayers by not considering the ancestor transform.

https://reviewboard.mozilla.org/r/81220/#review79798

	Markus Stange [:mstange]
	Comment 22 • 8 years ago
mozreview-review

Comment on attachment 8795032 [details]
Bug 1288210 - Simplify AlignFixedAndStickyLayers by not considering the local transform.

https://reviewboard.mozilla.org/r/81222/#review79802

::: gfx/layers/composite/AsyncCompositionManager.cpp:530
(Diff revision 1)
>            // TODO: There's a unit mismatch here, as |translation| is in ParentLayer
>            //       space while |stickyOuter| and |stickyInner| are in Layer space.

Does this comment still apply? |translation| is a LayerPoint now.

	Markus Stange [:mstange]
	Comment 23 • 8 years ago
mozreview-review

Comment on attachment 8795033 [details]
Bug 1288210 - Update the comment above AlignFixedAndStickyLayers().

https://reviewboard.mozilla.org/r/81224/#review79804

	Markus Stange [:mstange]
	Comment 24 • 8 years ago
mozreview-review

Comment on attachment 8795034 [details]
Bug 1288210 - Don't use ForEachNode in AlignFixedAndStickyLayers.

https://reviewboard.mozilla.org/r/81226/#review79806

	Markus Stange [:mstange]
	Comment 25 • 8 years ago
mozreview-review

Comment on attachment 8795034 [details]
Bug 1288210 - Don't use ForEachNode in AlignFixedAndStickyLayers.

https://reviewboard.mozilla.org/r/81226/#review79808

	Markus Stange [:mstange]
	Comment 26 • 8 years ago
mozreview-review

Comment on attachment 8795035 [details]
Bug 1288210 - In AlignFixedAndStickyLayers, propagate the unconsumed portion of the adjustment to descendant layers.

https://reviewboard.mozilla.org/r/81228/#review79810

::: gfx/layers/composite/AsyncCompositionManager.cpp:560
(Diff revision 1)
> +    LayerPoint newTransformedAnchor = unconsumedTranslation + anchor;
> +    ParentLayerPoint newTransformedAnchorInSubtreeRootSpace =
> +        aPreviousTransformForRoot.TransformPoint(newTransformedAnchor);
> +    LayerToParentLayerMatrix4x4 newTransform = aPreviousTransformForRoot;
> +    newTransform.PostTranslate(newTransformedAnchorInSubtreeRootSpace -
> +                               offsetAnchorInSubtreeRootSpace);

I really like the variable names here.

	Markus Stange [:mstange]
	Comment 27 • 8 years ago
mozreview-review

Comment on attachment 8795036 [details]
Bug 1288210 - Reftest.

https://reviewboard.mozilla.org/r/81230/#review79812

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 28 • 8 years ago
mozreview-review

Comment on attachment 8795031 [details]
Bug 1288210 - Simplify AlignFixedAndStickyLayers by not considering the ancestor transform.

https://reviewboard.mozilla.org/r/81220/#review79814

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 29 • 8 years ago
mozreview-review

Comment on attachment 8795032 [details]
Bug 1288210 - Simplify AlignFixedAndStickyLayers by not considering the local transform.

https://reviewboard.mozilla.org/r/81222/#review79816

::: gfx/layers/composite/AsyncCompositionManager.cpp:530
(Diff revision 1)
>            // TODO: There's a unit mismatch here, as |translation| is in ParentLayer
>            //       space while |stickyOuter| and |stickyInner| are in Layer space.

Can we drop this TODO now?

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 30 • 8 years ago

Oh, heh. Look like Markus had the same comment - it didn't show up in MozReview though.

(In reply to Botond Ballo [:botond] from comment #18)
> For a couple of the patches, I'm asking Kats to review as well because
> they're a bit more risky in terms of regression potential.

The changes look straightforward assuming your assumptions are correct, and I like that you left the old code in for DEBUG builds, so it will trip if the assumptions turn out to be invalid. LGTM.

	Botond Ballo [:botond] Assignee
	Comment 31 • 8 years ago

(In reply to Markus Stange [:mstange] from comment #22)
> ::: gfx/layers/composite/AsyncCompositionManager.cpp:530
> (Diff revision 1)
> >            // TODO: There's a unit mismatch here, as |translation| is in ParentLayer
> >            //       space while |stickyOuter| and |stickyInner| are in Layer space.
> 
> Does this comment still apply? |translation| is a LayerPoint now.

Yup, good catch! I'll remove it.

	Botond Ballo [:botond] Assignee
	Comment 32 • 8 years ago

Addressed review comment. Doing one more try push as the patches series has changed since the last one:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7cf781e07261

	Botond Ballo [:botond] Assignee
	Comment 41 • 8 years ago

(In reply to Botond Ballo [:botond] from comment #32)
> Addressed review comment. Doing one more try push as the patches series has
> changed since the last one:
> 
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=7cf781e07261

Win7 opt R-e10s runs are timing out intermittently, but I see they are without my patches too:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c0e4d75db879

so I think we're good to go.

	Pulsebot
	Comment 42 • 8 years ago

Pushed by bballo@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0ae35c34f4e9
Improve the use of strongly typed units in AsyncCompositionManager. r=mstange
https://hg.mozilla.org/integration/autoland/rev/d758ed99896d
Fixed a bug in how the ancestor transform is computed in AlignFixedAndStickyLayers. r=mstange
https://hg.mozilla.org/integration/autoland/rev/d5732c396703
Simplify AlignFixedAndStickyLayers by not considering the ancestor transform. r=kats,mstange
https://hg.mozilla.org/integration/autoland/rev/72a754e5f8e0
Simplify AlignFixedAndStickyLayers by not considering the local transform. r=kats,mstange
https://hg.mozilla.org/integration/autoland/rev/e9723fed5995
Update the comment above AlignFixedAndStickyLayers(). r=mstange
https://hg.mozilla.org/integration/autoland/rev/eb59ffa183cd
Don't use ForEachNode in AlignFixedAndStickyLayers. r=mstange
https://hg.mozilla.org/integration/autoland/rev/4672d3063ce0
In AlignFixedAndStickyLayers, propagate the unconsumed portion of the adjustment to descendant layers. r=mstange
https://hg.mozilla.org/integration/autoland/rev/9df68a52f118
Reftest. r=mstange

	Carsten Book [:Tomcat]
	Comment 43 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/0ae35c34f4e9
https://hg.mozilla.org/mozilla-central/rev/d758ed99896d
https://hg.mozilla.org/mozilla-central/rev/d5732c396703
https://hg.mozilla.org/mozilla-central/rev/72a754e5f8e0
https://hg.mozilla.org/mozilla-central/rev/e9723fed5995
https://hg.mozilla.org/mozilla-central/rev/eb59ffa183cd
https://hg.mozilla.org/mozilla-central/rev/4672d3063ce0
https://hg.mozilla.org/mozilla-central/rev/9df68a52f118

	Botond Ballo [:botond] Assignee
	Comment 44 • 8 years ago

I'd prefer to let this ride the trains rather than uplifting it, for the following reasons:

  - Now that we know bug 1300864 is unrelated to this, we haven't actually had
    a report about this edge case in the wild.

  - The patch assumes that certain transforms are identity, and asserts in debug
    builds if they're not. While we're pretty sure these assumption should hold,
    riding the trains gives us more time to validate them.

I'm happy to revisit this decision if someone objects.