make scrollbars scroll the visual viewport offset
Categories
(Core :: Panning and Zooming, enhancement)
Tracking
()
| Tracking | Status | |
|---|---|---|
| firefox80 | --- | fixed |
People
(Reporter: tnikkel, Assigned: tnikkel)
References
Details
Attachments
(5 files)
|
Assignee | |
Comment 1•4 years ago
|
||
I could have use apz.allow_zooming but using a separate pref means we can flip the pref to check if the new scrollbar code is the source of a regression. Also I haven't tested the code on Fenix at all, so we can disable it there for now.
|
|
Assignee | |
Comment 2•4 years ago
|
||
Depends on D82684
|
|
Assignee | |
Comment 3•4 years ago
|
||
Now that layout places the scroll thumbs at the visual viewport offset instead of the layout scroll position we need to update how the compositor adjusts them.
Depends on D82685
|
|
Assignee | |
Comment 4•4 years ago
|
||
The scrollbar is now positioned using the visual viewport offset, so we need to update when that changes.
Depends on D82686
|
|
Assignee | |
Comment 5•4 years ago
|
||
We already have relative updates (they use a base offset and compute the difference from that to get the offset to apply) but they only seem to know about layout scroll positions. Ideally these two would be merged.
We need to communicate with apz to do the scroll because we need to update the visual viewport offset (apz is in charge of that), and usually we need to smooth scroll to the new offset.
Depends on D82687
|
||
Comment 6•4 years ago
|
||
I'm having trouble making sense of that last patch. Can you provide more context in this bug as to what kind of problem that patch is solving? In general it's also good to link this bug to the other ones (presumably 1639450 and 1642500) that this work supports and describe what kind of problems this bug is solving or how these patches help those bugs.
|
|
Assignee | |
Comment 7•4 years ago
|
||
Right sorry. I'll explain a little here and then update the commit message with some more details.
The last patch is basically to make clicking in the scrollbar track outside of the scrollthumb "work", so it's pretty close to bug 1639450. Clicking in the scrollbar track usually does a page scroll via nsSliderFrame::PageScroll. This eventually ends up in ScrollFrameHelper::ScrollBy where turn the request from a relative one ("scroll by a page") into an absolute one ("scroll to this position").
This page scroll is typically a smooth scroll and is currently done on the main thread/layout side. Once we start scrolling the visual viewport offset with the scrollbars we can no longer do this purely on the layout side, we at least need the help of the compositor side. I think the simplest way to do this is to hand the scroll request off to the compositor and have it handle the whole thing.
Now we need to consider the following situation: user clicks scrollbar track to page scroll, smooth scroll gets partway complete on the compositor, user clicks again on scrollbar track for a further page scroll. The main thread can't send an absolute scroll offset update request to the compositor at this point because it has outdated information (it needs a 'starting position' to add the page scroll offset amount) so it'll end up scrolling to the wrong place. It has to send a relative scroll offset update.
We already have a mechanism to send relative scroll offset updates. It is implemented by sending a base scroll offset and the desired scroll offset, and then on the compositor send the difference between those two is computed and then added to the scroll offset.
I used a new mechanism (so called "pure relative") that just sends a relative offset update without any absolute scroll positions. The reason I did this is because the existing relative scroll offset update mechanism is not aware of visual viewport offsets, but rather only layout scroll position. For example, here
the value we use for the base scroll offset (mApzScrollPos) is set to the layout scroll position. It may be entirely reasonable to make this existing mechanism vv offset aware, but I wanted to implement something to get it working with a smaller chance of regressions to things that already exist and work. We could unify these two mechanisms either before or after landing this bug by either: 1) making the existing relative scroll update mechanism work with vv offsets and using it for this bug, 2) moving the existing relative scroll offset updates consumers to the new "pure relative" way.
- assumes that there isn't a good reason to encode relative scroll offset updates as the difference between two scroll positions that I missed.
So there are a couple of open questions there. Is a purely relative scroll offset update a good idea? Should we move to the pure relative way? Or to the existing difference between absolute scroll offsets way? And how much of that work is what we want to do right now vs follow ups.
|
|
Assignee | |
Updated•4 years ago
|
|
||
Updated•4 years ago
|
|
|
||
Comment 8•4 years ago
|
||
Sorry for the delay, was on PTO Friday and AFK for most of the weekend.
(In reply to Timothy Nikkel (:tnikkel) from comment #7)
Right sorry. I'll explain a little here and then update the commit message with some more details.
Thanks, the explanation makes sense and helps a lot!
The last patch is basically to make clicking in the scrollbar track outside of the scrollthumb "work", so it's pretty close to bug 1639450.
In the long term I think we'll want to handle this on the APZ side, the way we do scrollthumb dragging. But that might be a bit complicated so for the short term doing it on the main thread seems like a reasonable option. And we might need the main-thread mechanism even in the long term as a fallback if there are cases we can't handle in the compositor.
Clicking in the scrollbar track usually does a page scroll via nsSliderFrame::PageScroll. This eventually ends up in ScrollFrameHelper::ScrollBy where turn the request from a relative one ("scroll by a page") into an absolute one ("scroll to this position").
This page scroll is typically a smooth scroll and is currently done on the main thread/layout side. Once we start scrolling the visual viewport offset with the scrollbars we can no longer do this purely on the layout side, we at least need the help of the compositor side. I think the simplest way to do this is to hand the scroll request off to the compositor and have it handle the whole thing.
Seems reasonable, yes.
Now we need to consider the following situation: user clicks scrollbar track to page scroll, smooth scroll gets partway complete on the compositor, user clicks again on scrollbar track for a further page scroll. The main thread can't send an absolute scroll offset update request to the compositor at this point because it has outdated information (it needs a 'starting position' to add the page scroll offset amount) so it'll end up scrolling to the wrong place. It has to send a relative scroll offset update.
The existing mechanism for sending smooth scrolls to the compositor lives in ScrollFrameHelper::ApzSmoothScrollTo and involves setting mApzSmoothScrollDestination to the scroll position at the end of the smooth scroll. Currently that is (I believe) a layout viewport position, and only ever used for deduplicating smooth scroll requests, but in theory it should be possible to have it be a visual viewport position, and then use it as a "base" for computing a new destination in the case of multiple consecutive page scrolls. I'm not saying we should do this, since I'm not sure offhand if the complexity/error-prone-ness of this approach will be more or less than what you have now. But I just wanted to mention it as a possible alternative solution.
We already have a mechanism to send relative scroll offset updates. It is implemented by sending a base scroll offset and the desired scroll offset, and then on the compositor send the difference between those two is computed and then added to the scroll offset.
I used a new mechanism (so called "pure relative") that just sends a relative offset update without any absolute scroll positions. The reason I did this is because the existing relative scroll offset update mechanism is not aware of visual viewport offsets, but rather only layout scroll position. For example, here
the value we use for the base scroll offset (mApzScrollPos) is set to the layout scroll position. It may be entirely reasonable to make this existing mechanism vv offset aware, but I wanted to implement something to get it working with a smaller chance of regressions to things that already exist and work. We could unify these two mechanisms either before or after landing this bug by either: 1) making the existing relative scroll update mechanism work with vv offsets and using it for this bug, 2) moving the existing relative scroll offset updates consumers to the new "pure relative" way.
Ok. Seems like we have a lot of different options here for future consolidation :) One thing I just thought of that we might want to do regardless of which option we do is introduce a new type so that we can distinguish between layout-viewport scroll offsets (e.g. mApzScrollPos) and visual-viewport scroll offsets. That might make things a little less error-prone, and might have saved you some time during your investigations.
- assumes that there isn't a good reason to encode relative scroll offset updates as the difference between two scroll positions that I missed.
I don't think there is. At least, I just looked at all the places FrameMetrics::mBaseScrollOffset gets used and it seems to always be in combination with either FrameMetrics::mScrollOffset or FrameMetrics::mSmoothScrollOffset. So in theory we could just have mScrollOffset or mSmoothScrollOffset hold the "pure relative" value and use the mIsRelative flag to know that it's a relative value rather than an absolute value.
So there are a couple of open questions there. Is a purely relative scroll offset update a good idea? Should we move to the pure relative way? Or to the existing difference between absolute scroll offsets way? And how much of that work is what we want to do right now vs follow ups.
Yeah, I don't know the answers here, but if you have a working solution now I'm inclined to get that landed first. Then we should add a test or two to cover the situation these patches fix, and then look at followups that improve the code structure without regressing the tests.
|
|
||
Comment 9•4 years ago
|
||
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #8)
- assumes that there isn't a good reason to encode relative scroll offset updates as the difference between two scroll positions that I missed.
I don't think there is. At least, I just looked at all the places
FrameMetrics::mBaseScrollOffsetgets used and it seems to always be in combination with eitherFrameMetrics::mScrollOffsetorFrameMetrics::mSmoothScrollOffset. So in theory we could just havemScrollOffsetormSmoothScrollOffsethold the "pure relative" value and use themIsRelativeflag to know that it's a relative value rather than an absolute value.
Oh, maybe I was wrong. There's the use at https://searchfox.org/mozilla-central/rev/af5cff556a9482d7aa2267a82f2ccfaf18e797e9/gfx/layers/FrameMetrics.h#268 which I think can't be supported if we just have a pure relative offset in the metrics.
|
|
Assignee | |
Comment 10•4 years ago
|
||
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #9)
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #8)
- assumes that there isn't a good reason to encode relative scroll offset updates as the difference between two scroll positions that I missed.
I don't think there is. At least, I just looked at all the places
FrameMetrics::mBaseScrollOffsetgets used and it seems to always be in combination with eitherFrameMetrics::mScrollOffsetorFrameMetrics::mSmoothScrollOffset. So in theory we could just havemScrollOffsetormSmoothScrollOffsethold the "pure relative" value and use themIsRelativeflag to know that it's a relative value rather than an absolute value.Oh, maybe I was wrong. There's the use at https://searchfox.org/mozilla-central/rev/af5cff556a9482d7aa2267a82f2ccfaf18e797e9/gfx/layers/FrameMetrics.h#268 which I think can't be supported if we just have a pure relative offset in the metrics.
Hmm, that seems broken because mBaseScrollOffset is always layout scroll position but mScrollOffset is vv offset.
So if we need to support this we can keep sending a base scroll offset for this purpose only.
|
|
Assignee | |
Comment 11•4 years ago
|
||
After updating my tree and testing with the new patches the smooth scroll animation sometimes stops on the first frame because it hits this line
looking into why that is.
|
|
Assignee | |
Comment 12•4 years ago
•
|
||
The problem seems to be webrender specific: when starting an APZ animation we will get sample times that have large non-monotonic jumps in them regularly:
0x13db6f000 AsyncPanZoomController::StartAnimation mLastSampleTime 13.079496
0x13db6f000 AsyncPanZoomController::UpdateAnimation mLastSampleTime 11.261767
0x13db6f000 AsyncPanZoomController::UpdateAnimation mLastSampleTime 13.096266
mLastSampleTime comes from two different sources, either GetFrameTime() or WebRenderBridgeParent::SetAPZSampleTime. The WebRenderBridgeParent::SetAPZSampleTime ones seem to be the problem.
This causes us to scroll to the right place but instantly instead of smoothly when it happens. This clearly does not seem like an issue with these patches but it would be good to resolve it before landing because otherwise these patches would regress the smooth scrolling for any users with webrender and zooming turned on (it happens even if you never pinch zoom though).
|
|
Assignee | |
Comment 13•4 years ago
|
||
Filed bug 1653796 for that issue.
|
||
Comment 14•4 years ago
|
||
Pushed by tnikkel@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/d71765b6908f Create a pref to gate the new scrollbar code on. r=kats https://hg.mozilla.org/integration/autoland/rev/c99841ff8519 Update scrollbar attrs to use visual viewport offset. r=kats https://hg.mozilla.org/integration/autoland/rev/251aa3acc01f Update how the compositor adjust scroll thumbs. r=kats https://hg.mozilla.org/integration/autoland/rev/8309d62060f0 Update scrollbar position when the visual viewport offset changes. r=kats https://hg.mozilla.org/integration/autoland/rev/138e7b575614 Use purely relative scroll offset updates for many scrollbar initiated scrolls. r=kats
|
||
Comment 15•4 years ago
|
||
| bugherder | ||
https://hg.mozilla.org/mozilla-central/rev/d71765b6908f
https://hg.mozilla.org/mozilla-central/rev/c99841ff8519
https://hg.mozilla.org/mozilla-central/rev/251aa3acc01f
https://hg.mozilla.org/mozilla-central/rev/8309d62060f0
https://hg.mozilla.org/mozilla-central/rev/138e7b575614
|
||
Comment 16•4 years ago
|
||
How should this interact with general.smoothScroll? I have it disabled, but now I get the smooth scrolling effect when e.g. pressing the arrow keys. Is that intentional?
On a related note, page down sometimes scrolls immediately on the first press, then the animation works on the following ones. Maybe it figures it's running behind and skips the animation.
|
|
Assignee | |
Comment 17•4 years ago
|
||
(In reply to Laurențiu Nicola from comment #16)
How should this interact with
general.smoothScroll? I have it disabled, but now I get the smooth scrolling effect when e.g. pressing the arrow keys. Is that intentional?
Hmm, interesting, I'll look into this.
On a related note, page down sometimes scrolls immediately on the first press, then the animation works on the following ones. Maybe it figures it's running behind and skips the animation.
That's bug 1653796.
Description
•