Implement scroll handoff by gterzian · Pull Request #533 · servo/webrender

gterzian · 2016-11-06T14:38:35Z

WIP for servo/servo#13249

To summarize what was discussed in the issue:

"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" Scrolling to end of element should start scrolling the parent element. servo#13249 (comment)
"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." Scrolling to end of element should start scrolling the parent element. servo#13249 (comment)

This change is

gterzian · 2016-11-06T14:42:14Z

@glennw @mrobinson Any input appreciated, pls tell me more about how this would fit in the approach chosen in #517

If we all agree it's 'good enough', we could ask @pcwalton for a review as originally planned?

glennw · 2016-11-07T20:41:34Z

@mrobinson is probably best to comment on this stuff.

mrobinson · 2016-11-09T07:39:32Z

@gterzian You'll need to scroll all the layers that have the same scroll root. Otherwise the contents of scrollable divs will be out of sync. I'll be on #servo if you want to discuss in real-time.

gterzian · 2016-11-09T10:35:01Z

@glennw OK I have taken @mrobinson feedback into account, should we ask for a review?

glennw · 2016-11-10T00:24:15Z

r? @pcwalton (I think this looks OK, but I don't know the overscroll stuff super well).

pcwalton · 2016-11-10T00:26:13Z

webrender/src/frame.rs

+        let scroll_root_id = match (switch_layer, scroll_layer_id.info, root_scroll_layer_id.info) {
+            (true, _, ScrollLayerInfo::Scrollable(_, scroll_root_id)) => scroll_root_id,
+            (true, ScrollLayerInfo::Scrollable(_, scroll_root_id), ScrollLayerInfo::Fixed) => scroll_root_id,
+            (false, ScrollLayerInfo::Scrollable(_, scroll_root_id), _) => scroll_root_id,


Combine this with the first in an or-pattern, perhaps?

thanks, it's done...

gterzian · 2016-11-10T08:18:47Z

@pcwalton I have noticed, that when I 'switch layer', the overscrolling layer that I am switching out 'jumps' up and comes down only when the scroll has ended, perhaps because I am interfering with some of the overscroll behavior? And this seems to be intermittent by the way...

glennw · 2016-11-18T02:32:11Z

@gterzian Sorry this has languished for a bit - Unfortunately the overscrolling is on mac only (and the only mac I have available is a mac mini without trackpad). Do you know if that intermittent behaviour you mention above also happens without your patch? If it does, it's probably fine to merge this.

gterzian · 2016-11-20T13:43:57Z

@glennw @pcwalton I finally found what caused that intermittent 'jumping' behaviour, it was the mutation of delta that was spilling over from one loop iteration to the other, so I now switched to copying the delta for each iteration and it has been fixed. (I think it was intermittent previously because the order in which the layers would come up in the iteration would vary).

Since the scroll function is called only once with a given delta, and it is mutated to scroll a layer, and we now have a loop that can scroll different layers, I think it makes sense to not have those mutations spill over from one layer to the other. It also certainly fixes my problem here...

glennw

Looks good! A few minor things to fix then this should be good to go.

glennw · 2016-11-21T05:19:31Z

webrender/src/frame.rs

+            },
+            (ScrollEventPhase::Start, None, _) => return false,
+            (_, _, Some(scroll_layer_id)) => scroll_layer_id,
+            (_, _, None) => return false,


We should probably have a condition here that sets the current scroll layer back to None at the end of the scroll event?

I have tried this, and on Mac it makes the scroll stop right in it's track when you stop scrolling, as opposed to the default 'natural continuous scroll'. I think we can consider leaving things as they are, to retain the default behavior on Mac.

glennw · 2016-11-21T05:21:06Z

webrender/src/frame.rs


        let mut scrolled_a_layer = false;
        for (layer_id, layer) in self.layers.iter_mut() {
+            let mut inner_delta = delta;


Perhaps call this layer_delta? And we should be able to remove the mut from the function parameter for delta, I think?

good one, done.

glennw · 2016-11-21T05:21:19Z

webrender/src/frame.rs

                layer.stretch_overscroll_spring();
            }
-
+            


nit: extra space

glennw · 2016-11-21T05:22:14Z

webrender/src/frame.rs

-        let scroll_root_id = match scroll_layer_id.info {
-            ScrollLayerInfo::Scrollable(_, scroll_root_id) => scroll_root_id,
-            ScrollLayerInfo::Fixed => unreachable!("Tried to scroll a fixed position layer."),
+        let switch_layer = match phase {


I don't completely understand what the non_root_overscroll is doing - could you add some comments explaining?

I have added a few comments, please let me know what you think.

gterzian · 2016-11-21T10:22:45Z

webrender/src/layer.rs

    pub spring: Spring,
    pub started_bouncing_back: bool,
    pub bouncing_back: bool,
+    pub should_handoff_scroll: bool


@glennw I renamed the field on layer, so that it simply states it's purpose from the layer's point of view...

glennw · 2016-11-22T01:29:08Z

I was just doing some testing with this, but it seems to break scrolling on Linux altogether.

I'm not sure if that's because of the Linux scroll implementation, or because I rebased this on top of master before testing.

Are you able to test this on a Linux machine? (You may be able to repro it on a mac by switching the CAN_OVERSCROLL constant, which is set based on mac/linux).

gterzian · 2016-11-22T08:47:13Z

@glennw thanks for testing. I don't have a Linux machine readily available, however I might be able to run it into a VM of some sorts, given more time.

I did already follow your suggestion of setting CAN_OVERSCROLL to false on Mac, results:

Overall scrolling seemed fine.
There is one small issue, which is that since there is no bounce back of an ovescrolling layer, the child layer that we switch away from remains in 'should_handoff_scroll' mode, since it is still scrolled to the end, and it takes at least one extra gesture and some scrolling with the parent layer to start scrolling that child layer again. I've fixed that by only switching layer if current_layer.scrolling.should_handoff_scroll && non_root_overscroll.
I also noticed that sometimes the last phase would be Move(false), as opposed to End, so I added a case to the match for proper cleanup of layer.should_handoff_scroll.

Can I rebase/edit and push the above mentioned changes? I guess it should not create any problems unless you were to work off the branch that you pulled for testing...

gterzian · 2016-11-22T09:19:09Z

UPDATE: I have now also tested after a rebase off master (using servo/servo#14286 on the servo side), and it seems to work on Mac, even with overscrolling turned off.

@glennw could you describe the Linux problems in more details? Does the scrolling simply not work at all?

glennw · 2016-11-22T22:02:34Z

@gterzian Yep - the scrolling was completely broken. Even on a really simple page with a single scroll layer (like https://news.ycombinator.com) I was unable to scroll at all. But when I reverted the commits in this branch and ran, I could scroll again. That's quite strange that changing CAN_OVERSCROLL doesn't reproduce it - I'm not aware of any other obvious differences between the linux/mac scrolling implementations...

gterzian · 2016-11-23T03:03:42Z

@glennw thanks for the update! Ok, I guess I will need to debug that using linux on virtualbox...

bors-servo · 2016-11-25T09:17:05Z

☔ The latest upstream changes (presumably #590) made this pull request unmergeable. Please resolve the merge conflicts.

…he parent

gterzian · 2016-11-27T14:23:14Z

@glennw Due to resource constraints on my current machine, I'm having some troubles building and running Servo in a Linux VM for testing.

In an attempt to try to move things forward while I find a solution, I've broken up this PR into two ones, since we're actually doing two separate things here, and I have a feeling it's only one of the two that has the potential to completely break scrolling on Linux.

Could I ask you to please test this one on your machine: #599 ?

I am hopeful it will work on Linux, while I will have to further debug this one later: #600

glennw · 2016-11-27T23:26:41Z

@gterzian Sure, I'll test those patches on Linux today and report back the results.

gterzian mentioned this pull request Nov 6, 2016

allow scrolling past child layer, when a new scroll starts at the end of child layer #465

Closed

gterzian force-pushed the implement_scroll_handoff branch 2 times, most recently from 1abd21e to deddeb5 Compare November 9, 2016 10:26

gterzian force-pushed the implement_scroll_handoff branch from deddeb5 to 624aff8 Compare November 9, 2016 16:31

pcwalton reviewed Nov 10, 2016

View reviewed changes

gterzian force-pushed the implement_scroll_handoff branch from 624aff8 to ecf7e16 Compare November 10, 2016 08:14

gterzian mentioned this pull request Nov 13, 2016

Implement Home end key scroll #540

Merged

gterzian mentioned this pull request Nov 19, 2016

Update to webrender 0.10.0 servo/servo#14286

Merged

5 tasks

gterzian force-pushed the implement_scroll_handoff branch 2 times, most recently from b58fe56 to 20b500a Compare November 20, 2016 13:39

glennw reviewed Nov 21, 2016

View reviewed changes

gterzian force-pushed the implement_scroll_handoff branch from 20b500a to 4a79187 Compare November 21, 2016 10:19

gterzian commented Nov 21, 2016

View reviewed changes

gterzian force-pushed the implement_scroll_handoff branch 4 times, most recently from 7426272 to 6fbc6f9 Compare November 21, 2016 10:52

when a non_root layer overscrolls, and a new gesture starts, scroll t…

63b9708

…he parent

gterzian force-pushed the implement_scroll_handoff branch from 0f15516 to ecaec50 Compare November 27, 2016 13:57

keeping the currently scrolling layer

a68baf3

gterzian force-pushed the implement_scroll_handoff branch from ecaec50 to 26a3b05 Compare November 27, 2016 13:59

fixes to switch layer - to be squashed later

26a3b05

gterzian closed this Nov 27, 2016

Conversation

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

glennw commented Nov 7, 2016

Uh oh!

mrobinson commented Nov 9, 2016

Uh oh!

gterzian commented Nov 9, 2016

Uh oh!

glennw commented Nov 10, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

glennw commented Nov 18, 2016

Uh oh!

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

Uh oh!

Uh oh!

glennw left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

glennw commented Nov 22, 2016

Uh oh!

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

Uh oh!

Uh oh!

gterzian commented Nov 22, 2016

Uh oh!

glennw commented Nov 22, 2016

Uh oh!

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

Uh oh!

Uh oh!

bors-servo commented Nov 25, 2016

Uh oh!

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

Uh oh!

Uh oh!

glennw commented Nov 27, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

gterzian commented Nov 6, 2016 •

edited

Loading

gterzian commented Nov 6, 2016 •

edited

Loading

gterzian commented Nov 10, 2016 •

edited

Loading

gterzian commented Nov 20, 2016 •

edited

Loading

gterzian commented Nov 22, 2016 •

edited

Loading

gterzian commented Nov 23, 2016 •

edited

Loading

gterzian commented Nov 27, 2016 •

edited

Loading