Keeping the currently scrolling layer, unless new gesture starts #600

gterzian · 2016-11-27T14:19:50Z

"If you scroll a long page, and somewhere on that page is a smaller scrollable element, you don't want that element to swallow the rest of your scroll gesture if it suddenly happens to move under your mouse. You want the current scroll gesture to keep scrolling the outer page." servo/servo#13249 (comment)

Page used for testing can be found online here.

This change is

kvark

one nit, otherwise looks good

kvark · 2016-11-28T16:52:38Z

webrender/src/frame.rs

-            Some(scroll_layer_id) => scroll_layer_id,
-            None => return false,
+        let scroll_layer_id = match (phase, self.get_scroll_layer(&cursor, root_scroll_layer_id),
+            self.current_scroll_layer_id) {


the indentation doesn't look right to me. Perhaps, align every tuple element after match (?

gterzian · 2016-11-30T08:05:41Z

@kvark thanks for your review, I will address your points in time... By the way this PR, just like Handoff scroll when end of layer reached, and new gesture starts #599, will require additional work because it currently only works on Mac OS, and breaks scrolling on other platforms...

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

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

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

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

kvark

LGTM as long as you sqash it

align match elements

gterzian · 2016-12-14T23:15:59Z

@glennw Ok I've just done some additional testing, and realized my previous implementations of 'scroll transactions' was missing one case see servo/servo#14597 other than that this one shouldn't break scrolling on Linux anymore, and still implement the desired behavior. I've tested this on my mac by disabling overscrolling, as well as manually removing the start/end events from the compositor...

glennw · 2016-12-14T23:32:29Z

@gterzian Great, thanks! Once that fix lands for Servo, I'll test this locally and get it merged :)

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

glennw · 2016-12-21T03:21:09Z

Tested on Linux on a few sites, and this seems to work well. Thanks!

glennw · 2016-12-21T03:21:13Z

@bors-servo r+

bors-servo · 2016-12-21T03:21:14Z

📌 Commit 11edf22 has been approved by glennw

bors-servo · 2016-12-21T03:21:18Z

⌛ Testing commit 11edf22 with merge 4474785...

Keeping the currently scrolling layer, unless new gesture starts "If you scroll a long page, and somewhere on that page is a smaller scrollable element, you don't want that element to swallow the rest of your scroll gesture if it suddenly happens to move under your mouse. You want the current scroll gesture to keep scrolling the outer page." servo/servo#13249 (comment) Page used for testing can be found online [here](https://samuknet.github.io/test_cases/nestedScroll/).  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/600)

bors-servo · 2016-12-21T03:30:49Z

☀️ Test successful - status-travis

gterzian · 2016-12-21T09:13:30Z

@glennw thanks for your help!

@glennw

…plement_scroll_transactions); r=mbrubeck  @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 _____  Source-Repo: https://github.com/servo/servo Source-Revision: 92d380c399204d327e74771f5a9b8b2a343acecc

@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

…plement_scroll_transactions); r=mbrubeck  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 _____  Source-Repo: https://github.com/servo/servo Source-Revision: 92d380c399204d327e74771f5a9b8b2a343acecc UltraBlame original commit: 5e6f51510f8927cd014e93a7c9526da8465612cb

gterzian mentioned this pull request Nov 27, 2016

Implement scroll handoff #533

Closed

gterzian changed the title ~~Keeping the currently scrolling layer~~ Keeping the currently scrolling layer, unless new gesture starts Nov 27, 2016

gterzian mentioned this pull request Nov 28, 2016

Implement scroll handoff #599

Merged

kvark suggested changes Nov 28, 2016

View reviewed changes

gterzian force-pushed the keep_scrolling_layer branch from eed1072 to ae1885f Compare December 6, 2016 15:56

gterzian mentioned this pull request Dec 6, 2016

Implement scroll transactions servo/servo#14470

Merged

5 tasks

kvark approved these changes Dec 14, 2016

View reviewed changes

keeping the currently scrolling layer

11edf22

align match elements

gterzian force-pushed the keep_scrolling_layer branch from e39a21a to 11edf22 Compare December 14, 2016 22:23

gterzian mentioned this pull request Dec 14, 2016

FIX for Implement scroll transactions servo/servo#14597

Merged

5 tasks

bors-servo merged commit 11edf22 into servo:master Dec 21, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Keeping the currently scrolling layer, unless new gesture starts #600

Keeping the currently scrolling layer, unless new gesture starts #600

Uh oh!

gterzian commented Nov 27, 2016 •

edited

Loading

Uh oh!

kvark left a comment

Uh oh!

kvark Nov 28, 2016

Uh oh!

gterzian commented Nov 30, 2016

Uh oh!

kvark left a comment

Uh oh!

gterzian commented Dec 14, 2016

Uh oh!

glennw commented Dec 14, 2016

Uh oh!

glennw commented Dec 21, 2016

Uh oh!

glennw commented Dec 21, 2016

Uh oh!

bors-servo commented Dec 21, 2016

Uh oh!

bors-servo commented Dec 21, 2016

Uh oh!

bors-servo commented Dec 21, 2016

Uh oh!

gterzian commented Dec 21, 2016

Uh oh!

Uh oh!

Keeping the currently scrolling layer, unless new gesture starts #600

Keeping the currently scrolling layer, unless new gesture starts #600

Uh oh!

Conversation

gterzian commented Nov 27, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kvark left a comment

Choose a reason for hiding this comment

Uh oh!

kvark Nov 28, 2016

Choose a reason for hiding this comment

Uh oh!

gterzian commented Nov 30, 2016

Uh oh!

kvark left a comment

Choose a reason for hiding this comment

Uh oh!

gterzian commented Dec 14, 2016

Uh oh!

glennw commented Dec 14, 2016

Uh oh!

glennw commented Dec 21, 2016

Uh oh!

glennw commented Dec 21, 2016

Uh oh!

bors-servo commented Dec 21, 2016

Uh oh!

bors-servo commented Dec 21, 2016

Uh oh!

bors-servo commented Dec 21, 2016

Uh oh!

gterzian commented Dec 21, 2016

Uh oh!

Uh oh!

gterzian commented Nov 27, 2016 •

edited

Loading