1187804 - Fixed items inside iframe could move when scrolling the outer document

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Xidorn Quan [:xidorn] UTC+11]

Attachment: testcase

Steps to reproduce:
1. Open the attached testcase
2. Scroll the content inside the iframe
3. Scroll the outer document

Expected result:
The fixed <input> box should always stay unmoved related to the iframe

Actual result:
The fixed box moves in the different direction of scrolling, and backs to its correct position after a while.

I could reproduce this on both e10s and non-e10s window.

Comment 1

[Alice0775 White]

Maybe related to Bug 1166301

Disabled APZ (layers.async-pan-zoom.enabled =  false ) helps.
And it seems not a regression.

Comment 2

[Alice0775 White]

Regression window(slightly different behavior) with force enable APZ:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=c3376e427908&tochange=1b0655f6b110

Comment 3

[Botond Ballo [:botond]]

Attachment: MozReview Request: Bug 1187804 - Annotate fixed-position layers with the scroll id of the scroll frame that they are fixed with respect to. r=mattwoodrow

Bug 1187804 - Annotate fixed-position layers with the scroll id of the scroll frame that they are fixed with respect to. r=mattwoodrow

Comment 4

[Botond Ballo [:botond]]

Attachment: MozReview Request: Bug 1187804 - Un-adjust fixed layers by the async transform of the scroll frame that they're fixed with respect to, not of the nearest ancestor scroll frame. r=kats

Bug 1187804 - Un-adjust fixed layers by the async transform of the scroll frame that they're fixed with respect to, not of the nearest ancestor scroll frame. r=kats

Comment 5

[Botond Ballo [:botond]]

Attachment: MozReview Request: Bug 1187804 - When a layer is scrolled by multiple scroll frames, do an AlignFixedAndStickyLayers pass on its subtree for each of the scroll frames. r=kats

Bug 1187804 - When a layer is scrolled by multiple scroll frames, do an AlignFixedAndStickyLayers pass on its subtree for each of the scroll frames. r=kats

Comment 6

[Botond Ballo [:botond]]

Attachment: MozReview Request: Bug 1187804 - Reftests for async scrolling with position:fixed in an iframe. r=kats

Bug 1187804 - Reftests for async scrolling with position:fixed in an iframe. r=kats

Comment 7

[Botond Ballo [:botond]]

The attached patches fix this problem, and a related problem for which Markus also wrote a testcase, and lay the groundwork for fixing bug 1166301.

The patches basically implement part of step (1) of the plan described here: https://etherpad.mozilla.org/apz-background-fixed. The remaining part is handling the case where in between a fixed layer and the ancestor scroll frame that it's fixed with respect to, there are additional scroll frames with async transforms that also need to be un-applied. This is more of an edge case so I'm going to leave fixing it for a follow-up, but for the record the general idea of the fix I had in mind is changing the condition "is aTransformScrollId equal to the the fixed layer's target scroll id" to "is aTransformScrollId on the path from the fixed layer to its target scroll id".

Comment 8

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8648314 [details]
MozReview Request: Bug 1187804 - Un-adjust fixed layers by the async transform of the scroll frame that they're fixed with respect to, not of the nearest ancestor scroll frame. r=kats

https://reviewboard.mozilla.org/r/16193/#review14507

Comment 9

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8648315 [details]
MozReview Request: Bug 1187804 - When a layer is scrolled by multiple scroll frames, do an AlignFixedAndStickyLayers pass on its subtree for each of the scroll frames. r=kats

https://reviewboard.mozilla.org/r/16195/#review14505

So functionally this looks fine. However it seems inefficent now to be walking the entire subtree rooted at each scrollable layer. Instead we could just have a single pass after all the async transforms are applied to do the fixed-pos things (this would end up calling AlignFixedAndStickyLayers from TransformShadowTree rather than ApplyAsyncContentTransformToTree). We can defer that to a follow-up though since it may not be as easy to compute the transformWithoutOverscrollOrOmta value. And it would probably be easier to do after converting Fennec to APZ and ripping out the TransformScrollableLayer codepath.

::: gfx/layers/composite/AsyncCompositionManager.cpp:705
(Diff revision 1)
>      combinedAsyncTransformWithoutOverscroll *= asyncTransformWithoutOverscroll;

Remove the combinedAsyncTransformWithoutOverscroll variable, it's not needed any more.

Comment 10

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8648316 [details]
MozReview Request: Bug 1187804 - Reftests for async scrolling with position:fixed in an iframe. r=kats

https://reviewboard.mozilla.org/r/16197/#review14509

Thanks for adding tests! :)

Some minor comments below; they all apply to both tests.

::: layout/reftests/async-scrolling/position-fixed-iframe-1.html:4
(Diff revision 1)
> +      reftest-displayport-w="800" reftest-displayport-h="2000"

I'm not sure the displayport stuff is actually needed here or in the subdocument; it doesn't look like anything in the test extends beyond the 1000px height mark.

::: layout/reftests/async-scrolling/position-fixed-iframe-1.html:10
(Diff revision 1)
> +    <html reftest-async-scroll

Don't need the reftest-async-scroll empty attribute inside the subdocument; reftest-content.js will walk the entire subtree given that the outermost documentElement has this attribute.

::: layout/reftests/async-scrolling/position-fixed-iframe-1.html:16
(Diff revision 1)
> +    </body>

I'd like to see a non-fixed element in this page as well. You can put one just below the fixed element so that when the async-scroll is applied it gets hidden by the fixed element. Or you can put it somewhere else and have a corresponding thing on the ref page. The intent of this is just as a sanity to make sure that the async-scroll is actually taking effect.

Comment 11

[Kartikaya Gupta (email:[e-mail])]

https://reviewboard.mozilla.org/r/16191/#review14511

Just a heads-up that this patch touches the same code as bug 1180295 (part 2) and one of us will need to rebase. I'm happy to rebase mine on top since mine is more likely to get backed out.

Comment 12

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #10)
> Thanks for adding tests! :)

(Note: it was Markus who wrote the tests, as well as the Part 1 patch. ReviewBoard isn't very good about showing the author of the patch on the review page (I have bug 1194913 on file about that)).

Comment 13

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #10)
> ::: layout/reftests/async-scrolling/position-fixed-iframe-1.html:4
> (Diff revision 1)
> > +      reftest-displayport-w="800" reftest-displayport-h="2000"
> 
> I'm not sure the displayport stuff is actually needed here or in the
> subdocument; it doesn't look like anything in the test extends beyond the
> 1000px height mark.

It appears to be necessary: if there's no displayport specified, reftest-content.js doesn't call nsIDOMWindowUtils.setDisplayPortForElement() [1] and the scroll frame is not layerized.

[1] http://mxr.mozilla.org/mozilla-central/source/layout/tools/reftest/reftest-content.js?rev=d82d621f2351#228

Comment 14

[Kartikaya Gupta (email:[e-mail])]

Ah, that makes sense. Thanks for checking!

Comment 15

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8648313 [details]
MozReview Request: Bug 1187804 - Annotate fixed-position layers with the scroll id of the scroll frame that they are fixed with respect to. r=mattwoodrow

https://reviewboard.mozilla.org/r/16191/#review14555

Ship It!

::: gfx/layers/Layers.h:1156
(Diff revision 1)
> -   * (such as when asynchronously scrolling and zooming).
> +        mFixedPositionData->mScrollId != aScrollId ||

Please keep the comments for these two functions, and maybe add mention of the aScrollId addition.

::: gfx/layers/Layers.h:1765
(Diff revision 1)
> +  nsAutoPtr<FixedPositionData> mFixedPositionData;

We prefer UniquePtr<T> now I believe

Comment 16

[Matt Woodrow (:mattwoodrow)]

https://reviewboard.mozilla.org/r/16191/#review14557

Ship It!

Comment 17

[Botond Ballo [:botond]]

Addressed review comments. Not pushing them to MozReview right now due to bug 1195455, but you can see the patches in these new Try pushes:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=49cb6bd0fcbc (APZ disabled)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e3101d99e615 (APZ enabled)

Comment 18

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4e445d0cc426
https://hg.mozilla.org/integration/mozilla-inbound/rev/9b1e6aaf1151
https://hg.mozilla.org/integration/mozilla-inbound/rev/49a495622112
https://hg.mozilla.org/integration/mozilla-inbound/rev/54dc2dfe1805

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/4e445d0cc426
https://hg.mozilla.org/mozilla-central/rev/9b1e6aaf1151
https://hg.mozilla.org/mozilla-central/rev/49a495622112
https://hg.mozilla.org/mozilla-central/rev/54dc2dfe1805