Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upImplement scroll handoff #599
Conversation
|
@gterzian OK, I tested this on Linux. It doesn't seem to break anything, but it also doesn't seem to work - the old behaviour persists where if I keep the cursor in the nested scroll area, and scroll to the bottom, it doesn't scroll the outer scroll area. I tried this with both a mouse wheel and also scrolling on my touchpad. I might have some time during the week to debug this and see what's going on. |
|
@gterzian I did a little bit of investigation on Linux. It looks like on Linux we never receive TouchStart / TouchEnd events. From a very quick look in glutin, it seems that it does provide these events on Linux, but only for touch events (although I don't receive them even when using the touchpad). I'm not actually sure whether we should expect to rely on always getting a start/end event, or if we should expect to receive Scroll / Move events without a corresponding start / end. Perhaps @mbrubeck could comment on this? I wonder if it's possible to update the logic in this PR to work without relying on the start / end events? |
|
@gterzian PS the above now explains why we see these issues on Linux and they are unrelated to the CAN_OVERSCROLL flag. |
|
Yes, macOS is the only platforms that groups wheel events into units, by giving you start+end events with phases like NSEventPhaseBegan and NSEventPhaseEnded. Unless you connect a traditional USB mouse to your Mac - in that case, even macOS can't help you, because the hardware has no way to tell whether your finger is still on the scroll wheel. (In terms of native wheel events, USB mouse wheel ticks result in wheel events with scroll phase NSEventPhaseNone.) So on all non-Mac platforms, and on Mac for events with NSEventPhaseNone, you need to do your own grouping of events. In Firefox, we do this based on pause time and mouse movement. The code is around here: http://searchfox.org/mozilla-central/source/gfx/layers/apz/src/InputBlockState.cpp#361 |
|
|
|
Ok so looking at the Firefox code, it seems that the input events are grouped together into transactions, where a 'transaction' represents pretty much a continuous scroll gesture. The In Servo it's currently Webrender that does the scrolling, and Webrender receives those scroll events from the compositor (from looking at this it seems the compositor is already grouping events together in some way: https://github.com/servo/servo/blob/master/components/compositing/compositor.rs#L218). I guess the choice we face is either to implement a 'scroll transaction' in compositor, and send events to webrender always with either We could also implement those transactions in webrender. By simply always starting/ending the transaction when we do receive A downside of doing those transactions in WR might be that we don't know what the original event was, so we can't change our 'transaction logic' based on the scrolling hardware used. (in FF there is a base then again, implementing some quick and dirty transaction logic right here in webrender might be a good start... Please let me know what you think... |
|
I discussed with @mbrubeck - we think it makes sense to do the grouping the Servo compositor - that lets WR deal with the high level concept of scroll transactions, and leaves glutin to deal with raw events. |
|
@glennw great! thanks for your input, I will start looking around compositor... |
5f239a9
to
9a75258
e2c2aa3
to
df0e96a
…beck Implement scroll transactions <!-- Please describe your changes on the following line: --> @glennw Here is a first pass at faking Start scroll events by way of 'transactions', as suggested by @mstange at servo/webrender#599 (comment) Since I still don't have a Linux environment available for testing(and my Mac doesn't have enough resources to run a VM at the moment), I tested this with both servo/webrender#599 and servo/webrender#600 on a Mac by: * disabling start and end events by removing the content of these two functions: https://github.com/servo/servo/blob/master/components/compositing/compositor.rs#L1080 and https://github.com/servo/servo/blob/master/components/compositing/compositor.rs#L1093 * Setting `CAN_OVERSCROLL` to false for Mac OS in Webrender https://github.com/servo/webrender/blob/master/webrender/src/frame.rs#L29 * This PR also requires a `./mach update-cargo -a` The desired behavior of both Webrender PR's, based on my manual testing, now also works when there are no end or start scroll events provided by the os. The scroll transactions do not affect normal scrolling on Mac OS, and both PR still work as before on that platform. Both PR in Webrender need some re-basing and cleaning up, as does this one, and I first wanted to put this proposal forward, and also ask if someone has the time to do some testing in a real Linux environment... --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: --> - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #13249 (github issue number if applicable). <!-- Either: --> - [ ] There are tests for these changes OR - [ ] These changes do not require tests because _____ <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. --> <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14470) <!-- Reviewable:end -->
…beck Implement scroll transactions <!-- Please describe your changes on the following line: --> @glennw Here is a first pass at faking Start scroll events by way of 'transactions', as suggested by @mstange at servo/webrender#599 (comment) Since I still don't have a Linux environment available for testing(and my Mac doesn't have enough resources to run a VM at the moment), I tested this with both servo/webrender#599 and servo/webrender#600 on a Mac by: * disabling start and end events by removing the content of these two functions: https://github.com/servo/servo/blob/master/components/compositing/compositor.rs#L1080 and https://github.com/servo/servo/blob/master/components/compositing/compositor.rs#L1093 * Setting `CAN_OVERSCROLL` to false for Mac OS in Webrender https://github.com/servo/webrender/blob/master/webrender/src/frame.rs#L29 * This PR also requires a `./mach update-cargo -a` The desired behavior of both Webrender PR's, based on my manual testing, now also works when there are no end or start scroll events provided by the os. The scroll transactions do not affect normal scrolling on Mac OS, and both PR still work as before on that platform. Both PR in Webrender need some re-basing and cleaning up, as does this one, and I first wanted to put this proposal forward, and also ask if someone has the time to do some testing in a real Linux environment... --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: --> - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #13249 (github issue number if applicable). <!-- Either: --> - [ ] There are tests for these changes OR - [ ] These changes do not require tests because _____ <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. --> <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14470) <!-- Reviewable:end -->
…beck Implement scroll transactions <!-- Please describe your changes on the following line: --> @glennw Here is a first pass at faking Start scroll events by way of 'transactions', as suggested by @mstange at servo/webrender#599 (comment) Since I still don't have a Linux environment available for testing(and my Mac doesn't have enough resources to run a VM at the moment), I tested this with both servo/webrender#599 and servo/webrender#600 on a Mac by: * disabling start and end events by removing the content of these two functions: https://github.com/servo/servo/blob/master/components/compositing/compositor.rs#L1080 and https://github.com/servo/servo/blob/master/components/compositing/compositor.rs#L1093 * Setting `CAN_OVERSCROLL` to false for Mac OS in Webrender https://github.com/servo/webrender/blob/master/webrender/src/frame.rs#L29 * This PR also requires a `./mach update-cargo -a` The desired behavior of both Webrender PR's, based on my manual testing, now also works when there are no end or start scroll events provided by the os. The scroll transactions do not affect normal scrolling on Mac OS, and both PR still work as before on that platform. Both PR in Webrender need some re-basing and cleaning up, as does this one, and I first wanted to put this proposal forward, and also ask if someone has the time to do some testing in a real Linux environment... --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: --> - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #13249 (github issue number if applicable). <!-- Either: --> - [ ] There are tests for these changes OR - [ ] These changes do not require tests because _____ <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. --> <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14470) <!-- Reviewable:end -->
…beck Implement scroll transactions <!-- Please describe your changes on the following line: --> @glennw Here is a first pass at faking Start scroll events by way of 'transactions', as suggested by @mstange at servo/webrender#599 (comment) Since I still don't have a Linux environment available for testing(and my Mac doesn't have enough resources to run a VM at the moment), I tested this with both servo/webrender#599 and servo/webrender#600 on a Mac by: * disabling start and end events by removing the content of these two functions: https://github.com/servo/servo/blob/master/components/compositing/compositor.rs#L1080 and https://github.com/servo/servo/blob/master/components/compositing/compositor.rs#L1093 * Setting `CAN_OVERSCROLL` to false for Mac OS in Webrender https://github.com/servo/webrender/blob/master/webrender/src/frame.rs#L29 * This PR also requires a `./mach update-cargo -a` The desired behavior of both Webrender PR's, based on my manual testing, now also works when there are no end or start scroll events provided by the os. The scroll transactions do not affect normal scrolling on Mac OS, and both PR still work as before on that platform. Both PR in Webrender need some re-basing and cleaning up, as does this one, and I first wanted to put this proposal forward, and also ask if someone has the time to do some testing in a real Linux environment... --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: --> - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #13249 (github issue number if applicable). <!-- Either: --> - [ ] There are tests for these changes OR - [ ] These changes do not require tests because _____ <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. --> <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14470) <!-- Reviewable:end -->
1b71327
to
e0c4589
|
@glennw the Servo "scroll transaction" part has landed(servo/servo#14470), and I have rebased to remove the conflicts. It's actually hard to test it once again, because I see the Servo side of https://github.com/servo/webrender/pull/540/files has not landed yet... I did test it extensively as part of the Servo PR. Once thing to note is that some details are still left to be implemented: "the scroll will be handed off to the next outer scrollable element which is actually scrollable in that direction". Currently I just hand-off the scroll to the root scroll layer(even though I think I am certainly not checking for "which is actually scrollable in that direction". Do you think it would be best to further work on those features in this PR, or rather look at this one has a good first step, and leave those notes in a follow-up issue on Servo? |
|
I think it's fine to land this as a first step - it's a net improvement. I've landed the WR update in Servo, so it should be possible to rebase this, test it, and get it merged once the two formatting nits are fixed. |
| @@ -445,12 +480,12 @@ impl Frame { | |||
| if CAN_OVERSCROLL { | |||
| layer.stretch_overscroll_spring(); | |||
| } | |||
|
|
|||
This comment has been minimized.
This comment has been minimized.
| scrolled_a_layer = scrolled_a_layer || | ||
| layer.scrolling.offset != original_layer_scroll_offset || | ||
| layer.scrolling.started_bouncing_back; | ||
| } | ||
|
|
||
This comment has been minimized.
This comment has been minimized.
|
The PR is a bit over my head, but I don't see anything wrong with the code :) |
55f2554
to
10fc581
c79d4ac
to
d541621
|
@glennw OK i've rebased off of 4474785 (there were some deps issues with the current head), tested on a Mac in 'normal' mode, as well as with overscroll turned off and with start/end event handling in compositor turned off as well. Seems to work fine, when you have a chance to test in linux proper it would be much appreciated! |
|
@gterzian OK, I tested on Linux this morning - it doesn't seem to work for me on the test page referenced. I'll try to dig into why sometime this week. |
|
@glennw thanks! |
|
@gterzian Apologies - I haven't tested this yet on Linux - will try to get to it during this week :) |
|
@gterzian I'm unsure what changed, but this is working perfectly for me on Linux now :) |
|
@bors-servo r+ |
|
|
…d, r=glennw Implement scroll handoff "If a new scroll gesture starts while you're already scrolled all the way to the edge of the scrollable element under the mouse, then the scroll will be handed off to the next outer scrollable element which is actually scrollable in that direction" servo/servo#13249 (comment) Page used for testing can be found online [here](https://samuknet.github.io/test_cases/nestedScroll/). <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/599) <!-- Reviewable:end -->
|
|
|
@glennw thanks for all your help, it has been a long one! |
…plement_scroll_transactions); r=mbrubeck <!-- Please describe your changes on the following line: --> @glennw Here is a first pass at faking Start scroll events by way of 'transactions', as suggested by @mstange at servo/webrender#599 (comment) Since I still don't have a Linux environment available for testing(and my Mac doesn't have enough resources to run a VM at the moment), I tested this with both servo/webrender#599 and servo/webrender#600 on a Mac by: * disabling start and end events by removing the content of these two functions: https://github.com/servo/servo/blob/master/components/compositing/compositor.rs#L1080 and https://github.com/servo/servo/blob/master/components/compositing/compositor.rs#L1093 * Setting `CAN_OVERSCROLL` to false for Mac OS in Webrender https://github.com/servo/webrender/blob/master/webrender/src/frame.rs#L29 * This PR also requires a `./mach update-cargo -a` The desired behavior of both Webrender PR's, based on my manual testing, now also works when there are no end or start scroll events provided by the os. The scroll transactions do not affect normal scrolling on Mac OS, and both PR still work as before on that platform. Both PR in Webrender need some re-basing and cleaning up, as does this one, and I first wanted to put this proposal forward, and also ask if someone has the time to do some testing in a real Linux environment... --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: --> - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #13249 (github issue number if applicable). <!-- Either: --> - [ ] There are tests for these changes OR - [ ] These changes do not require tests because _____ <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. --> Source-Repo: https://github.com/servo/servo Source-Revision: 92d380c399204d327e74771f5a9b8b2a343acecc
…plement_scroll_transactions); r=mbrubeck <!-- Please describe your changes on the following line: --> glennw Here is a first pass at faking Start scroll events by way of 'transactions', as suggested by mstange at servo/webrender#599 (comment) Since I still don't have a Linux environment available for testing(and my Mac doesn't have enough resources to run a VM at the moment), I tested this with both servo/webrender#599 and servo/webrender#600 on a Mac by: * disabling start and end events by removing the content of these two functions: https://github.com/servo/servo/blob/master/components/compositing/compositor.rs#L1080 and https://github.com/servo/servo/blob/master/components/compositing/compositor.rs#L1093 * Setting `CAN_OVERSCROLL` to false for Mac OS in Webrender https://github.com/servo/webrender/blob/master/webrender/src/frame.rs#L29 * This PR also requires a `./mach update-cargo -a` The desired behavior of both Webrender PR's, based on my manual testing, now also works when there are no end or start scroll events provided by the os. The scroll transactions do not affect normal scrolling on Mac OS, and both PR still work as before on that platform. Both PR in Webrender need some re-basing and cleaning up, as does this one, and I first wanted to put this proposal forward, and also ask if someone has the time to do some testing in a real Linux environment... --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: --> - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #13249 (github issue number if applicable). <!-- Either: --> - [ ] There are tests for these changes OR - [ ] These changes do not require tests because _____ <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. --> Source-Repo: https://github.com/servo/servo Source-Revision: 92d380c399204d327e74771f5a9b8b2a343acecc UltraBlame original commit: 5e6f51510f8927cd014e93a7c9526da8465612cb
…plement_scroll_transactions); r=mbrubeck <!-- Please describe your changes on the following line: --> glennw Here is a first pass at faking Start scroll events by way of 'transactions', as suggested by mstange at servo/webrender#599 (comment) Since I still don't have a Linux environment available for testing(and my Mac doesn't have enough resources to run a VM at the moment), I tested this with both servo/webrender#599 and servo/webrender#600 on a Mac by: * disabling start and end events by removing the content of these two functions: https://github.com/servo/servo/blob/master/components/compositing/compositor.rs#L1080 and https://github.com/servo/servo/blob/master/components/compositing/compositor.rs#L1093 * Setting `CAN_OVERSCROLL` to false for Mac OS in Webrender https://github.com/servo/webrender/blob/master/webrender/src/frame.rs#L29 * This PR also requires a `./mach update-cargo -a` The desired behavior of both Webrender PR's, based on my manual testing, now also works when there are no end or start scroll events provided by the os. The scroll transactions do not affect normal scrolling on Mac OS, and both PR still work as before on that platform. Both PR in Webrender need some re-basing and cleaning up, as does this one, and I first wanted to put this proposal forward, and also ask if someone has the time to do some testing in a real Linux environment... --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: --> - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #13249 (github issue number if applicable). <!-- Either: --> - [ ] There are tests for these changes OR - [ ] These changes do not require tests because _____ <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. --> Source-Repo: https://github.com/servo/servo Source-Revision: 92d380c399204d327e74771f5a9b8b2a343acecc UltraBlame original commit: 5e6f51510f8927cd014e93a7c9526da8465612cb
…plement_scroll_transactions); r=mbrubeck <!-- Please describe your changes on the following line: --> glennw Here is a first pass at faking Start scroll events by way of 'transactions', as suggested by mstange at servo/webrender#599 (comment) Since I still don't have a Linux environment available for testing(and my Mac doesn't have enough resources to run a VM at the moment), I tested this with both servo/webrender#599 and servo/webrender#600 on a Mac by: * disabling start and end events by removing the content of these two functions: https://github.com/servo/servo/blob/master/components/compositing/compositor.rs#L1080 and https://github.com/servo/servo/blob/master/components/compositing/compositor.rs#L1093 * Setting `CAN_OVERSCROLL` to false for Mac OS in Webrender https://github.com/servo/webrender/blob/master/webrender/src/frame.rs#L29 * This PR also requires a `./mach update-cargo -a` The desired behavior of both Webrender PR's, based on my manual testing, now also works when there are no end or start scroll events provided by the os. The scroll transactions do not affect normal scrolling on Mac OS, and both PR still work as before on that platform. Both PR in Webrender need some re-basing and cleaning up, as does this one, and I first wanted to put this proposal forward, and also ask if someone has the time to do some testing in a real Linux environment... --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: --> - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #13249 (github issue number if applicable). <!-- Either: --> - [ ] There are tests for these changes OR - [ ] These changes do not require tests because _____ <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. --> Source-Repo: https://github.com/servo/servo Source-Revision: 92d380c399204d327e74771f5a9b8b2a343acecc UltraBlame original commit: 5e6f51510f8927cd014e93a7c9526da8465612cb
gterzian commentedNov 27, 2016
•
edited
"If a new scroll gesture starts while you're already scrolled all the way to the edge of the scrollable element under the mouse, then the scroll will be handed off to the next outer scrollable element which is actually scrollable in that direction" servo/servo#13249 (comment)
Page used for testing can be found online here.
This change is