Fixed items inside iframe could move when scrolling the outer document

RESOLVED FIXED in Firefox 43

Status

()

Core
Panning and Zooming
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: xidorn, Assigned: botond)

Tracking

({regression})

Trunk
mozilla43
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox42 affected, firefox43 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(5 attachments)

(Reporter)

Description

3 years ago
Created attachment 8639174 [details]
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

3 years ago
Maybe related to Bug 1166301

Disabled APZ (layers.async-pan-zoom.enabled =  false ) helps.
And it seems not a regression.
Component: DOM → Panning and Zooming

Comment 2

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

Comment 3

3 years ago
Created 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

Bug 1187804 - Annotate fixed-position layers with the scroll id of the scroll frame that they are fixed with respect to. r=mattwoodrow
Attachment #8648313 - Flags: review?(matt.woodrow)
(Assignee)

Comment 4

3 years ago
Created 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

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
Attachment #8648314 - Flags: review?(bugmail.mozilla)
(Assignee)

Comment 5

3 years ago
Created 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

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
Attachment #8648315 - Flags: review?(bugmail.mozilla)
(Assignee)

Comment 6

3 years ago
Created attachment 8648316 [details]
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
Attachment #8648316 - Flags: review?(bugmail.mozilla)
(Assignee)

Comment 7

3 years ago
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".
Assignee: nobody → botond
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
Attachment #8648314 - Flags: review?(bugmail.mozilla) → review+
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.
Attachment #8648315 - Flags: review?(bugmail.mozilla) → review+
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.
Attachment #8648316 - Flags: review?(bugmail.mozilla) → review+
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.
(Assignee)

Comment 12

3 years ago
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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)).
(Assignee)

Comment 13

3 years ago
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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
Ah, that makes sense. Thanks for checking!
Attachment #8648313 - Flags: review?(matt.woodrow) → review+
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
(Assignee)

Comment 17

3 years ago
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)
Blocks: 1201889
Keywords: regressionwindow-wanted
QA Whiteboard: [good first verify]
You need to log in before you can comment on or make changes to this bug.