Following the changes in bug 1423011, the scroll offset stored in the scroll frame will reflect the layout viewport offset for the root scroll frame of the root content document on all pages. Before the changes in 1423011, the layout viewport offset was always (0,0) and did not have any usage. Now the layout viewport offset is set to it’s actual value and can change depending on the position of the visual viewport. The main thread positions the displayport relative to the scroll frame’s scroll offset for the root content document which means it will now position the diaplyport relative to the layout viewport offset. So the main thread needs to know about the visual viewport offset as well stored in a separate place. The way to do it is to update the nspressshell to store the visual viewport and layout viewport.

It probably makes sense to do bug 1471708 first, since that will make it clearer that Layout is already storing the visual viewport *size* somewhere. If we then start storing the visual viewport *offset* in this bug, we may want to consider storing the two together in the same place as a rect.

Whoops, I meant for the dependency order between this bug and bug 1471708 to be reversed, although it's not a hard dependency. (We intend to start work on this soon, without necessarily waiting for bug 1471708.)

Comment on attachment 8991467 [details] Bug 1470267 - Store the visual viewport offset in nsIPresShell. https://reviewboard.mozilla.org/r/256346/#review263238 Code analysis found 1 defect in this patch: - 1 defect found by clang-tidy You can run this analysis locally with: - `./mach static-analysis check path/to/file.cpp` (C/C++) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: layout/base/nsIPresShell.h:1670 (Diff revision 1) > nsSize GetScrollPositionClampingScrollPortSize() { > NS_ASSERTION(mScrollPositionClampingScrollPortSizeSet, "asking for scroll port when its not set?"); > return mScrollPositionClampingScrollPortSize; > } > > + void SetScrollPositionClampingScrollPortOffset(nsPoint aScrollOffset) { Warning: The parameter 'aScrollOffset' is copied for each invocation but only used as a const reference; consider making it a const reference [clang-tidy: performance-unnecessary-value-param] void SetScrollPositionClampingScrollPortOffset(nsPoint aScrollOffset) { ~~~~~~~ ^ const &

Comment on attachment 8991467 [details] Bug 1470267 - Store the visual viewport offset in nsIPresShell. https://reviewboard.mozilla.org/r/256346/#review263248 ::: commit-message-3d20b:1 (Diff revision 1) > +Bug 1470267 - PresShell needs to have information on layout viewport offset and visual viewport offset. r?botond A few suggestions for improving this message: * The change we're making only concerns the visual viewport offset, so we should not mention the layout viewport offset. * We generally prefer using the active voice to describe what the patch does: "Give PresShell information on ..." rather than "PresShell needs to have information on ..." * "Give information on" is kind of vague. "Store the visual viewport offset in PresShell" would be more specific. * We're actually storing it in nsIPresShell, not PresShell, so "Store the visual viewport offset in nsIPresShell" would be more accurate. Note that there is no requirement that the first line of the commit message matches the bug title. (On the other hand, some of the mentioned improvements, such as removing mention of the layout viewport offset, could be made to the bug title as well; feel free to edit it.) ::: commit-message-3d20b:5 (Diff revision 1) > +Bug 1470267 - PresShell needs to have information on layout viewport offset and visual viewport offset. r?botond > + > +Added a data member to nsIPresShell to store the visual viewport offset. APZ > +will update the visual viewport offset in the presShell for root scroll frames > +each time a scroll offset change is detected. I would say "on every repaint request" instead of "each time a scroll offset change is detected" (again, to be more specific). ::: gfx/layers/apz/util/APZCCallbackHelper.cpp:312 (Diff revision 1) > // We don't currently support zooming for subframes, so nothing extra > // needs to be done beyond the tasks common to this and UpdateRootFrame. > ScrollFrame(content, aMetrics); > if (nsCOMPtr<nsIPresShell> shell = GetPresShell(content)) { > + // Update the visual viewport offset in layout for root scroll frames only > + if (shell->GetRootScrollFrame()) { This just checks that the pres shell _has_ a root scroll frame, which will typically always be true. We want to check whether the scroll frame to which this repaint request pertains, _is_ the pres shell's root scroll frame. To do that, we need the scroll frame object. We have existing code in ScrollFrame() that gets it (it's the nsIScrollableFrame), so we might as well move this logic into ScrollFrame(). (That would also avoid having to duplicate it between UpdateRootFrame() and UpdateSubFrame()). ::: layout/base/nsIPresShell.h:1674 (Diff revision 1) > > + void SetScrollPositionClampingScrollPortOffset(nsPoint aScrollOffset) { > + mScrollPositionClampingScrollPortOffset = aScrollOffset; > + } > + > + nsPoint GetScrollPositionClampingScrollPortOffset() { Please mark the getter as 'const' ::: layout/base/nsIPresShell.h:1756 (Diff revision 1) > // Count of the number of times this presshell has been painted to a window. > uint64_t mPaintCount; > > nsSize mScrollPositionClampingScrollPortSize; > > + nsPoint mScrollPositionClampingScrollPortOffset; Since we're moving in the direction of renaming "scroll position clamping scroll port" to "visual viewport" (bug 1471708), I would prefer naming this "mVisualViewportOffset" from the beginning. (I realize there is a consistency argument to be made for keeping the two names in sync, but since we're already planning on renaming the size, we might as well use the new name for the offset from the beginning.)

Comment on attachment 8991467 [details] Bug 1470267 - Store the visual viewport offset in nsIPresShell. https://reviewboard.mozilla.org/r/256346/#review263570 ::: gfx/layers/apz/util/APZCCallbackHelper.cpp:78 (Diff revision 2) > margins.top = margins.bottom = margins.TopBottom() / 2; > aFrameMetrics.SetDisplayPortMargins(margins); > } > > +static already_AddRefed<nsIPresShell> > +GetPresShell(const nsIContent* aContent) Moved GetPresShell as we need to define it before using it in ScrollFrame.

Comment on attachment 8991467 [details] Bug 1470267 - Store the visual viewport offset in nsIPresShell. https://reviewboard.mozilla.org/r/256346/#review263578 Thanks, this is looking much better! ::: layout/base/nsIPresShell.h:1674 (Diff revision 2) > > + void SetVisualViewportOffset(const nsPoint& aScrollOffset) { > + mVisualViewportOffset = aScrollOffset; > + } > + > + nsPoint GetVisualViewportOffset() { This method is still not marked const.

Comment on attachment 8991467 [details] Bug 1470267 - Store the visual viewport offset in nsIPresShell. https://reviewboard.mozilla.org/r/256346/#review263592

Pushed by bballo@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/7ce64da43ceb Store the visual viewport offset in nsIPresShell. r=botond