Implement scroll transactions #14470

gterzian · 2016-12-06T18:08:16Z

@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...

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #13249 (github issue number if applicable).

There are tests for these changes OR
These changes do not require tests because _____

This change is

glennw · 2016-12-08T18:30:48Z

r? @mbrubeck

notriddle · 2016-12-08T20:24:44Z

@gterzian Maybe you should sign up for https://janitor.technology/? It'll give you a real Linux environment, complete with NoVNC if you want to watch an occasionally-refreshed video feed of Servo running in X.

gterzian · 2016-12-09T08:39:29Z

@notriddle thanks! I will check it out..

mbrubeck · 2016-12-10T00:21:04Z

components/compositing/compositor.rs

@@ -396,6 +399,7 @@ impl<Window: WindowMethods> IOCompositor<Window> {
            last_composite_time: 0,
            ready_to_save_state: ReadyState::Unknown,
            scroll_in_progress: false,
+            in_scroll_transaction: None,


Should on_scroll_start_window_event and on_scroll_end_window_event also modify this field?

@mbrubeck Thank you for your suggestion. I have done some additional testing to be sure, and it seems that doing anything with in_scroll_transaction in those functions is unnecessary.

That is because, if those two functions are called (when the OS actually sends start/end events, which only Mac OS does at this point) the scroll transaction is, as far as I can tell, completely transparent.

To be precise, there is only one case in which the "transaction" will result in a change in the ScrollZoomEvent that will be sent to Webrender: When scroll_in_progress is false, and we receive a scroll after 80 ms. In this case a new "start event" is simulated by the transaction.

The only risk therefore, in my opinion, is that we would send inappropriate, or double, ScrollEventPhase::Start. This could happen if on_scroll_start_window_event were to be first called, followed by the transaction code sending another start phase after that. That is currently impossible, because on_scroll_start_window_event will always set scroll_in_progress to true.

Off course the scroll event stuff is quite complex, so it is entirely possible I have missed a specific case, yet at this stage I haven't found one yet.

An additional note: I have tried to implement the transactions with scroll_in_progress, however that field is used to implement a concept that overlaps, yet differs significantly from what those 'transactions' currently do. (For example, when the user has stopped actively scrolling, but Mac still sends additional scroll events that reflect the 'natural scrolling' on Mac, scroll_in_progress will be set to false, while we actually still need to keep the transaction open or else the scrolling will stop unnaturally on a Mac).

mbrubeck · 2016-12-12T22:20:09Z

@bors-servo r+

bors-servo · 2016-12-12T22:20:10Z

📌 Commit 2b623bf has been approved by mbrubeck

bors-servo · 2016-12-12T22:33:18Z

⌛ Testing commit 2b623bf with merge 750f666...


        Auto merge of #14470 - gterzian:implement_scroll_transactions, r=mbru…

@glennw

…beck Implement scroll transactions  @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... ---  - [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).  - [ ] There are tests for these changes OR - [ ] These changes do not require tests because _____   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14470)

mbrubeck · 2016-12-12T22:54:20Z

@bors-servo r-

Oops, I forgot that this has intermediate commits that should be squashed.


        Implement scroll transactions

working proof of concept transactions make the transaction transparent on mac simplify match further simplify match

KiChjang · 2016-12-12T23:01:00Z

@bors-servo r=mbrubeck clean

bors-servo · 2016-12-12T23:01:01Z

📌 Commit 1b9078a has been approved by mbrubeck

bors-servo · 2016-12-13T00:22:52Z

⌛ Testing commit 1b9078a with merge b7b056f...


        Auto merge of #14470 - gterzian:implement_scroll_transactions, r=mbru…

@glennw

…beck Implement scroll transactions  @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... ---  - [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).  - [ ] There are tests for these changes OR - [ ] These changes do not require tests because _____   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14470)

bors-servo · 2016-12-13T00:23:27Z

💔 Test failed - mac-rel-wpt2

KiChjang · 2016-12-13T00:26:51Z

@bors-servo retry

Oh great mac-rel-wpt2 builder, are we playing the network error game again?

bors-servo · 2016-12-13T00:26:54Z

⌛ Testing commit 1b9078a with merge 7258375...


        Auto merge of #14470 - gterzian:implement_scroll_transactions, r=mbru…

@glennw

…beck Implement scroll transactions  @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... ---  - [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).  - [ ] There are tests for these changes OR - [ ] These changes do not require tests because _____   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14470)

bors-servo · 2016-12-13T00:27:14Z

💔 Test failed - mac-rel-wpt2

KiChjang · 2016-12-13T00:27:32Z

@bors-servo retry

Yup, definitely playing that game

bors-servo · 2016-12-13T00:27:35Z

⌛ Testing commit 1b9078a with merge 92d380c...


        Auto merge of #14470 - gterzian:implement_scroll_transactions, r=mbru…

@glennw

…beck Implement scroll transactions  @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... ---  - [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).  - [ ] There are tests for these changes OR - [ ] These changes do not require tests because _____   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14470)

KiChjang · 2016-12-13T00:31:26Z

#Winning

bors-servo · 2016-12-13T02:10:46Z

☀️ Test successful - arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-dev


        Auto merge of #14597 - gterzian:fix_scroll_transactions, r=mbrubeck

@mbrubeck

FIX for Implement scroll transactions  Follow up on #14470 @mbrubeck @KiChjang @glennw I just found out in the context of servo/webrender#600 I forgot to add a case for the very first scroll event, or else the scrolling on that PR, in a non-Mac OS environment, will only start after an 80ms pause following the initial scroll event... Sorry this slipped through my initial testing... ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [ ] These changes fix #__ (github issue number if applicable).  - [ ] There are tests for these changes OR - [ ] These changes do not require tests because _____   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14597)


        servo: Merge #14597 - FIX for Implement scroll transactions (from gte…

@mbrubeck

…rzian:fix_scroll_transactions); r=mbrubeck  Follow up on servo/servo#14470 @mbrubeck @KiChjang @glennw I just found out in the context of servo/webrender#600 I forgot to add a case for the very first scroll event, or else the scrolling on that PR, in a non-Mac OS environment, will only start after an 80ms pause following the initial scroll event... Sorry this slipped through my initial testing... ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [ ] These changes fix #__ (github issue number if applicable).  - [ ] There are tests for these changes OR - [ ] These changes do not require tests because _____  Source-Repo: https://github.com/servo/servo Source-Revision: 139c111091210a5002bda668d14debb2e1c68ca9

highfive assigned metajack Dec 6, 2016

highfive added the S-awaiting-review label Dec 6, 2016

highfive assigned mbrubeck and unassigned metajack Dec 8, 2016

mbrubeck requested changes Dec 10, 2016

View changes

highfive added S-awaiting-merge and removed S-awaiting-review labels Dec 12, 2016

mbrubeck added S-needs-squash and removed S-awaiting-merge labels Dec 12, 2016

Implement scroll transactions

Loading status checks…

1b9078a

working proof of concept transactions make the transaction transparent on mac simplify match further simplify match

KiChjang force-pushed the gterzian:implement_scroll_transactions branch from 2b623bf to 1b9078a Dec 12, 2016

highfive added the S-awaiting-review label Dec 12, 2016

highfive added S-awaiting-merge and removed S-awaiting-review S-needs-squash labels Dec 12, 2016

highfive added S-tests-failed and removed S-awaiting-merge labels Dec 13, 2016

highfive added S-awaiting-merge and removed S-tests-failed labels Dec 13, 2016

highfive added S-tests-failed and removed S-awaiting-merge labels Dec 13, 2016

highfive added S-awaiting-merge and removed S-tests-failed labels Dec 13, 2016

bors-servo merged commit 1b9078a into servo:master Dec 13, 2016
3 checks passed

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

highfive removed the S-awaiting-merge label Dec 13, 2016

gterzian mentioned this pull request Dec 13, 2016

Implement scroll handoff servo/webrender#599

Merged

gterzian deleted the gterzian:implement_scroll_transactions branch Dec 14, 2016

gterzian mentioned this pull request Dec 14, 2016

FIX for Implement scroll transactions #14597

Merged

2 of 5 tasks complete

jdm mentioned this pull request Nov 1, 2018

Trackpad scrolling has dead zones #22073

Closed

servo / servo

Implement scroll transactions #14470

Implement scroll transactions #14470

gterzian commented Dec 6, 2016 •

edited

glennw commented Dec 8, 2016

notriddle commented Dec 8, 2016

gterzian commented Dec 9, 2016

This comment has been minimized.

This comment has been minimized.

mbrubeck commented Dec 12, 2016

bors-servo commented Dec 12, 2016

bors-servo commented Dec 12, 2016

mbrubeck commented Dec 12, 2016

KiChjang commented Dec 12, 2016

bors-servo commented Dec 12, 2016

bors-servo commented Dec 13, 2016

bors-servo commented Dec 13, 2016

KiChjang commented Dec 13, 2016

bors-servo commented Dec 13, 2016

bors-servo commented Dec 13, 2016

KiChjang commented Dec 13, 2016

bors-servo commented Dec 13, 2016

KiChjang commented Dec 13, 2016

bors-servo commented Dec 13, 2016

servo / servo

Join GitHub today

Implement scroll transactions #14470

Implement scroll transactions #14470

Conversation

gterzian commented Dec 6, 2016 • edited

glennw commented Dec 8, 2016

notriddle commented Dec 8, 2016

gterzian commented Dec 9, 2016

This comment has been minimized.

mbrubeck Dec 10, 2016

This comment has been minimized.

gterzian Dec 10, 2016 • edited

mbrubeck commented Dec 12, 2016

bors-servo commented Dec 12, 2016

bors-servo commented Dec 12, 2016

mbrubeck commented Dec 12, 2016

KiChjang commented Dec 12, 2016

bors-servo commented Dec 12, 2016

bors-servo commented Dec 13, 2016

bors-servo commented Dec 13, 2016

KiChjang commented Dec 13, 2016

bors-servo commented Dec 13, 2016

bors-servo commented Dec 13, 2016

KiChjang commented Dec 13, 2016

bors-servo commented Dec 13, 2016

KiChjang commented Dec 13, 2016

bors-servo commented Dec 13, 2016

gterzian commented Dec 6, 2016 •

edited

gterzian Dec 10, 2016 •

edited