Closed Bug 1187804 Opened 9 years ago Closed 9 years ago

Fixed items inside iframe could move when scrolling the outer document

Categories

(Core :: Panning and Zooming, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox42 --- affected
firefox43 --- fixed

People

(Reporter: xidorn, Assigned: botond)

References

Details

(Keywords: regression)

Attachments

(5 files)

Attached file testcase (deleted) —
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.
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
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)
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)
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)
Bug 1187804 - Reftests for async scrolling with position:fixed in an iframe. r=kats
Attachment #8648316 - Flags: review?(bugmail.mozilla)
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.
(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)).
(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
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
QA Whiteboard: [good first verify]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: