GetVisualViewportOffset appears to have a typo
Categories
(Core :: Panning and Zooming, defect, P3)
Tracking
()
| Tracking | Status | |
|---|---|---|
| firefox80 | --- | fixed |
People
(Reporter: kats, Assigned: kats)
References
Details
Attachments
(3 files)
https://searchfox.org/mozilla-central/rev/fac90408bcf52ca88a3dcd2ef30a379b68ab24e2/layout/generic/nsGfxScrollFrame.cpp#4232 - it's checking to see if the visual viewport size is set, instead of the offset. Presumably the two were always set together and so this never caused problems. But with work on bug 1644271 I think I'm running into a problem caused by this, maybe.
|
Assignee | |
Comment 1•4 years ago
|
||
|
||
Comment 2•4 years ago
|
||
It's not a typo; the function GetVisualViewportOffset() (added in bug 1484597) predates IsVisualViewportOffsetSet() (added in bug 1509575), so IsVisualViewportSizeSet() was the only thing available to call at the time to check if we'd be returning a valid quantity.
I do agree we should probably change it now; see also bug 1488969.
|
|
Assignee | |
Comment 3•4 years ago
|
||
Ah, good to know. I'll need to investigate the failures that are resulting from the change.
|
|
Assignee | |
Comment 4•4 years ago
|
||
I investigated the test_frame_reconstruction_scroll_restore.html failure. Quick summary of the test is: (a) scroll down, (b) shrink scrollframe and do frame reconstruction, (c) expand scrollframe and do frame reconstruction. In step (b) the scroll position should be clamped and in step (c) it should remain at the clamped position. What's failing is that in step (c) we go back to the scroll position that we had at (a).
It looks like there are times when the visual viewport size is not set but the visual viewport offset is set. Note that this is on a non-RCD presShell, so I was a bit surprised by the fact that the VV offset was set at all, but maybe that's ok. Anyway,
During step (c), when we do the second frame reconstruction, we save the scroll position which we get from the visual viewport offset here and that's the point at which this patch makes a meaningful difference. Previously we would be reading the scroll position (y=0) but with this patch we read the visual scroll offset y=4379 instead. Which is clearly wrong because this is happening after the scrollframe has been shortened, so y=4379 is not even a valid scroll offset.
So tracing backwards from there, the visual scroll offset I believe is supposed to get updated during the ScrollToImpl call here, after the scroll origin gets promoted from Restore to Other in ScrollToWithOrigin during ScrollToRestoredPosition from step (b). But that doesn't happen, for at least two reasons. One is because the scroll offset we pass in is already clamped here and so fails the check here. This is fixable by undoing most of this change which was later fixed better. But the second reason is just that the clamped scroll position is 0, which is the same as the existing scroll position on the scrollframe, so we take the early-exit here and never even try to adjust the visual viewport offset.
Working around this second problem seems harder, almost like the ScrollToRestoredPosition code should be directly updating the visual viewport offset. At any rate the code expects that quantity to be updated before it re-queries it and I don't see how that can happen.
|
||
Comment 5•4 years ago
|
||
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #4)
At any rate the code expects that quantity to be updated before it re-queries it and I don't see how that can happen.
It probably made sense before visual viewport stuff got added. The ScrollToWithOrigin call above would finish and the scroll offset would be updated after it.
|
|
||
Comment 6•4 years ago
|
||
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #4)
But the second reason is just that the clamped scroll position is 0, which is the same as the existing scroll position on the scrollframe, so we take the early-exit here and never even try to adjust the visual viewport offset.
Seems like we should do the visual viewport offset stuff even if we take the early exit, no?
|
|
||
Comment 7•4 years ago
|
||
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #4)
Working around this second problem seems harder, almost like the
ScrollToRestoredPositioncode should be directly updating the visual viewport offset. At any rate the code expects that quantity to be updated before it re-queries it and I don't see how that can happen.
Can we look at PresShell::GetPendingVisualScrollUpdate? It seems like we would want to look at that in most places where we look at the visual viewport offset anyways (if the pending visual scroll update is going to be the visual viewport offset on the next frame why would we want to look at the old visual viewport offset?).
|
|
Assignee | |
Comment 8•4 years ago
|
||
(In reply to Timothy Nikkel (:tnikkel) from comment #7)
Can we look at PresShell::GetPendingVisualScrollUpdate?
I don't think that'll work as-is, because the check here restricts setting the PendingVisualScrollUpdate to the RCD RSF. And this code runs on all RSFs. In particular this test is running in a non-RCD RSF so we don't call PresShell::ScrollToVisual.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #4)
It looks like there are times when the visual viewport size is not set but the visual viewport offset is set. Note that this is on a non-RCD presShell, so I was a bit surprised by the fact that the VV offset was set at all, but maybe that's ok.
Maybe it's not ok? I mean conceptually you only have a visual viewport on the RCD-RSF and in all the other scrollframes the VV offset is the same as the scroll position. Which is what's sort of happening in the code now, except that the VV offset isn't always updated exactly when the scroll position changes, and so if we read the VV offset instead of the scroll position it can give us the wrong result sometimes.
|
|
||
Comment 9•4 years ago
|
||
Perhaps, for a non-RCD it would be better to ignore (or never set) PresShell::mVisualViewportOffset, and when we need a visual viewport offset (e.g. for the Visual Viewport API), just use the layout viewport offset instead?
|
|
Assignee | |
Comment 10•4 years ago
|
||
I think I would like to move towards a more unified model, such that every presShell has a visual viewport set all the time, and code that uses it doesn't need to be checking if it's dealing with the RCD and/or if the VV is set. For non-RCD presShells the visual viewport would just mirror the layout viewport, but the callers of the APIs wouldn't need to know that.
Such a change will encompass this bug as well as bug 1488969, and will be non-trivial to do. However I think it will clean up the code significantly and give us a better path moving forward. Thoughts?
|
|
||
Comment 11•4 years ago
|
||
That would be fine as well. In that case, I think that would mean removing some existing "is RCD" checks, such as the one you mentioned that guards the ScrollToVisual() call.
|
|
Assignee | |
Comment 12•4 years ago
|
||
A lot of the code and issues mentioned in previous comments have changed recently. Here's a new try push to see where we stand:
|
|
Assignee | |
Comment 13•4 years ago
|
||
The test_frame_reconstruction_scroll_restore.html test is still failing. I investigated and it's the same problem as described above in comments 8-11. Basically seems to be a mismatch of conditions of when we update the visual viewport offset on the presshell. When scrolling a root scrollframe (regardless of RCD or not), we update the VV offset here. But then during scroll position restoration we only update it for the RCD, here.
Given that this test runs inside an iframe and is not the RCD, the initial scroll will set the VV offset of the presshell, but then the ScrollToRestoredPosition will not correct it with the new value later. So then at some later point we go to SaveState and read the VV offset here which comes back with a stale value.
I think removing the RCD checks is the way to go here.
|
|
Assignee | |
Comment 14•4 years ago
|
||
|
|
Assignee | |
Comment 15•4 years ago
|
||
This adds another case to this test, which is specifically intended to bypass
any short-circuit resulting from the clamped scroll offset being the same as
the scrollframe's existing scroll offset. The pre-existing case in the tests
shortens the scrollframe to be non-scrollable, so the clamped mRestorePos for
the scrollframe is 0, which is the same as the default scroll position of the
reconstructed scrollframe. The new case ensures the clamped mRestorePos is
greater than zero, to verify that scroll restoration works as intended in that
scenario as well.
|
|
Assignee | |
Comment 16•4 years ago
|
||
Depends on D81739
|
|
Assignee | |
Comment 17•4 years ago
|
||
Instead of checking for the visual viewport size being set, we check for
the offset being set. This makes more sense becuase we're going to be
reading the offset, not the size.
Depends on D81740
|
||
Comment 18•4 years ago
|
||
Pushed by kgupta@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/df4f6829484a Add another case to the test for scroll restoration during frame reconstruction. r=botond https://hg.mozilla.org/integration/autoland/rev/2074703b711c Move towards setting the visual viewport offset on all presShells. r=botond https://hg.mozilla.org/integration/autoland/rev/17dcdaec53ae Use a more appropriate check when reading the visual viewport offset. r=botond
|
||
Comment 19•4 years ago
|
||
| bugherder | ||
https://hg.mozilla.org/mozilla-central/rev/df4f6829484a
https://hg.mozilla.org/mozilla-central/rev/2074703b711c
https://hg.mozilla.org/mozilla-central/rev/17dcdaec53ae
Description
•