Implement Home end key scroll #540

samuknet · 2016-11-08T19:12:21Z

Addressing issue servo/servo#13082.
Supporting servo/servo#14141.

Adds ScrollLocation type in webrender_traits.
Refactor api to use new ScrollLocation
Implement home/end scrolling in webrender/src/frame.rs fn scroll(...) using new ScrollLocation struct passed in.

This change is

glennw

Nice! Left a few minor style comments, and a question for @mrobinson

glennw · 2016-11-08T21:58:59Z

webrender/src/frame.rs

@@ -282,9 +282,9 @@ impl Frame {
        result
    }

-    /// Returns true if any layers actually changed position or false otherwise.
+      /// Returns true if any layers actually changed position or false otherwise.


nit: indentation

glennw · 2016-11-08T21:58:59Z

webrender/src/frame.rs

@@ -378,7 +404,7 @@ impl Frame {
        }

        scrolled_a_layer
-    }
+}


nit: indentation

glennw · 2016-11-08T21:58:59Z

webrender/src/frame.rs

@@ -319,6 +319,32 @@ impl Frame {
                continue;
            }

+           let mut delta:Point2D<f32> = match scroll_location {


nit: indentation of this block

glennw · 2016-11-08T21:58:59Z

webrender/src/frame.rs

+            },
+            ScrollLocation::End => {
+                let end_pos = -layer.content_size.height +
+                                                 (layer.local_viewport_rect.size.height);


This is probably clearer as layer.local_viewport_rect.size.height - layer.content_size.height

glennw · 2016-11-08T21:58:59Z

webrender/src/frame.rs

+            ScrollLocation::Start => {
+                if layer.scrolling.offset.y.round() == 0.0 {
+                    // Nothing to do.
+                    return false;


I think that we don't want to return straight away here (or below) due to the recent scroll root changes that have been made. Is that correct @mrobinson ?

samuknet · 2016-11-11T18:45:12Z

@glennw thanks 😃 Just pushed those indentation fixes.

kvark

Got a few minor points and some questions.

kvark · 2016-11-11T20:52:57Z

webrender/src/frame.rs

@@ -319,6 +319,31 @@ impl Frame {
                continue;
            }

+            let mut delta:Point2D<f32> = match scroll_location {


is the explicit type required here?

kvark · 2016-11-11T20:52:57Z

webrender/src/frame.rs

+                    return true;
+                },
+                ScrollLocation::End => {
+                    let end_pos = -layer.content_size.height +


weird indentation and brackets

kvark · 2016-11-11T20:52:57Z

webrender/src/frame.rs

+                    let end_pos = -layer.content_size.height +
+                                                 (layer.local_viewport_rect.size.height);
+
+                    if layer.scrolling.offset.y.round() == end_pos {


should it be >= instead of ==?

ahh yeah I think >= makes sense, I also have done it for the ScrollLocation::End case, making replacing the == with <=.

kvark · 2016-11-11T20:52:57Z

webrender/src/frame.rs

+                    }
+
+                    layer.scrolling.offset.y = 0.0;
+                    return true;


So if it's Start or End you don't want to even proceed to the rest of the function body, even if the scroll happened?

kvark · 2016-11-11T20:52:57Z

webrender_traits/src/types.rs

@@ -439,6 +439,13 @@ pub enum ScrollPolicy {
    Fixed,
 }

+#[derive(Clone, Copy, Debug, Deserialize, Serialize)]
+pub enum ScrollLocation {
+    Delta(Point2D<f32>), // Scroll by a certain amount.


please move those comments atop of the elements, preceded by /// for some real documentation

gterzian · 2016-11-13T08:05:39Z

webrender/src/frame.rs

+                ScrollLocation::Start => {
+                    if layer.scrolling.offset.y.round() == 0.0 {
+                        // Nothing to do.
+                        return false;


from my own discussions with @mrobinson with regards to #533 I am pretty much certain that, as @glennw suggested in #540 (comment), those return statements could be turned into continue, because the loop still needs to scroll any other layer with the same scroll_root_id.

So instead of returning a boolean, have you considered doing something like scrolled_a_layer = scrolled_a_layer || your_boolean, followed by continue;?

okay :)

I was thinking something like this would work for these blocks:

ScrollLocation::Start => { if layer.scrolling.offset.y.round() == 0.0 { // Nothing to do on this layer. continue; } layer.scrolling.offset.y = 0.0; scrolled_a_layer = true; continue; },

What do you think? I have put these changes in and updated with latest WR master in the latest push

bors-servo · 2016-11-14T08:03:03Z

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

glennw

Thanks, this looks good to me! I added a couple of nits - they are extremely minor - but since this needs a rebase anyway, we might as well fix them up at the same time. Once rebased, this is ready to merge!

glennw · 2016-11-18T02:28:29Z

webrender/src/frame.rs

+                },
+                ScrollLocation::End => {
+                    let end_pos = -layer.content_size.height +
+                                  layer.local_viewport_rect.size.height;


Let's make this layer.local_viewport_rect.size.height - layer.content_size.height (just reads a bit better switching the order around).

glennw · 2016-11-18T02:28:29Z

webrender/src/frame.rs

+                    layer.scrolling.offset.y = 0.0;
+                    scrolled_a_layer = true;
+                    continue;
+                },


The comma here and in the section below aren't necessary.

glennw · 2016-11-18T02:29:49Z

@samuknet Ah, I hadn't realised this was already rebased. Let's fix those last couple of minor nits, squash in to one commit, and then r=me.

bors-servo · 2016-11-22T00:46:24Z

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

glennw · 2016-11-28T02:28:20Z

@samuknet Have you got time to rebase this and fix those last couple of nits?


        Resolve imports conflict.


        Implement home end key scrolling support.

samuknet · 2016-11-28T09:05:37Z

Sure! One issue I was having, is getting Servo to compile once the Webrender version has changed. I noticed webrender has had changes to its API/type signatures which means I can no longer compile and test Servo with the new scrolling code. Is there an easy way to check that everything still works together?


        Fix indentation.


        Add types ScrollLocation documentation. Insert continue in layers loo…

…p instead of early return.


        Fix nits, reordered calc, fixed indentation

gterzian · 2016-11-28T09:13:42Z

@samuknet Have you considered rebasing your branch of the uptream master? It seems a bunch of traits file have indeed changed, so it could help solve the problems you describe. If that isn't enough, you could also try to rebase the servo repository as well...

samuknet · 2016-11-28T09:37:52Z

I notice in webrender_traits/types.rs the previous Scroll message has now been changed to ScrollLayersWithScrollId(Point2D<f32>, PipelineId, ServoScrollRootId).

And that now this messages makes a call to:

pub fn scroll_layers(&mut self,
                         origin: Point2D<f32>,
                         pipeline_id: PipelineId,
                         scroll_root_id: ServoScrollRootId)

Is this effectively the new fn scroll_layers, i.e. should I refactor the existing home/end key scrolling code in fn scroll into fn scroll_layers? Or do we want both?

Thanks :D


        Add Scroll ApiMsg

glennw · 2016-11-28T22:33:45Z

The scroll_layers interface is mostly used for scrolling via JS, so it's fine to leave that as is.

glennw · 2016-11-28T22:33:50Z

@bors-servo r+

bors-servo · 2016-11-28T22:33:51Z

📌 Commit fa805af has been approved by glennw

glennw · 2016-11-28T22:33:54Z

Thanks!


        Auto merge of #540 - samuknet:home-end-key-scroll2, r=glennw

Implement Home end key scroll Addressing issue servo/servo#13082. Supporting servo/servo#14141. * Adds `ScrollLocation` type in `webrender_traits`. * Refactor api to use new `ScrollLocation` * Implement home/end scrolling in `webrender/src/frame.rs` `fn scroll(...)` using new `ScrollLocation` struct passed in.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/540)

bors-servo · 2016-11-28T23:00:27Z

⌛ Testing commit fa805af with merge 3a6725a...

bors-servo · 2016-11-28T23:42:34Z

☀️ Test successful - status-travis

samuknet · 2016-11-30T11:54:48Z

@glennw Thank you!


        Auto merge of #14141 - samuknet:home-end-key-scroll2, r=glennw

Implement home end key scrolling  * Refactor all scroll related code to use a new `ScrollLocation` struct which can either be a `delta` (as before) or a `Start` or `End` request, to represent the desire to scroll to the start and end of the page. Effectively, everywhere a delta was used, there is now a `ScrollLocation` struct instead. * Add key press listeners for HOME and END keys so as to cause a scroll to be queued with `ScrollLocation::Start` (in HOME case) or `ScrollLocation::End` (in END case). * These changes depend on added support for the new `ScrollLocation` in webrender and webrender_traits. See servo/webrender#540. ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [ x] These changes fix #13082 (github issue number if applicable).  - [ ] There are tests for these changes OR - [x] These changes do not require tests because scrolling I/O   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14141)


        Auto merge of #14141 - samuknet:home-end-key-scroll2, r=glennw

Implement home end key scrolling  * Refactor all scroll related code to use a new `ScrollLocation` struct which can either be a `delta` (as before) or a `Start` or `End` request, to represent the desire to scroll to the start and end of the page. Effectively, everywhere a delta was used, there is now a `ScrollLocation` struct instead. * Add key press listeners for HOME and END keys so as to cause a scroll to be queued with `ScrollLocation::Start` (in HOME case) or `ScrollLocation::End` (in END case). * These changes depend on added support for the new `ScrollLocation` in webrender and webrender_traits. See servo/webrender#540. ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [ x] These changes fix #13082 (github issue number if applicable).  - [ ] There are tests for these changes OR - [x] These changes do not require tests because scrolling I/O   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14141)


        servo: Merge #14141 - Implement home end key scrolling (from samuknet…

…:home-end-key-scroll2); r=glennw  * Refactor all scroll related code to use a new `ScrollLocation` struct which can either be a `delta` (as before) or a `Start` or `End` request, to represent the desire to scroll to the start and end of the page. Effectively, everywhere a delta was used, there is now a `ScrollLocation` struct instead. * Add key press listeners for HOME and END keys so as to cause a scroll to be queued with `ScrollLocation::Start` (in HOME case) or `ScrollLocation::End` (in END case). * These changes depend on added support for the new `ScrollLocation` in webrender and webrender_traits. See servo/webrender#540. ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [ x] These changes fix #13082 (github issue number if applicable).  - [ ] There are tests for these changes OR - [x] These changes do not require tests because scrolling I/O  Source-Repo: https://github.com/servo/servo Source-Revision: 1706ffd6e5a02f26f69970b3b41536a8a85ef6fe


        servo: Merge #14141 - Implement home end key scrolling (from samuknet…

…:home-end-key-scroll2); r=glennw  * Refactor all scroll related code to use a new `ScrollLocation` struct which can either be a `delta` (as before) or a `Start` or `End` request, to represent the desire to scroll to the start and end of the page. Effectively, everywhere a delta was used, there is now a `ScrollLocation` struct instead. * Add key press listeners for HOME and END keys so as to cause a scroll to be queued with `ScrollLocation::Start` (in HOME case) or `ScrollLocation::End` (in END case). * These changes depend on added support for the new `ScrollLocation` in webrender and webrender_traits. See servo/webrender#540. ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [ x] These changes fix #13082 (github issue number if applicable).  - [ ] There are tests for these changes OR - [x] These changes do not require tests because scrolling I/O  Source-Repo: https://github.com/servo/servo Source-Revision: 1706ffd6e5a02f26f69970b3b41536a8a85ef6fe UltraBlame original commit: 43657bf6458152f8a99e25af3bd99094669f6642


        servo: Merge #14141 - Implement home end key scrolling (from samuknet…

…:home-end-key-scroll2); r=glennw  * Refactor all scroll related code to use a new `ScrollLocation` struct which can either be a `delta` (as before) or a `Start` or `End` request, to represent the desire to scroll to the start and end of the page. Effectively, everywhere a delta was used, there is now a `ScrollLocation` struct instead. * Add key press listeners for HOME and END keys so as to cause a scroll to be queued with `ScrollLocation::Start` (in HOME case) or `ScrollLocation::End` (in END case). * These changes depend on added support for the new `ScrollLocation` in webrender and webrender_traits. See servo/webrender#540. ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [ x] These changes fix #13082 (github issue number if applicable).  - [ ] There are tests for these changes OR - [x] These changes do not require tests because scrolling I/O  Source-Repo: https://github.com/servo/servo Source-Revision: 1706ffd6e5a02f26f69970b3b41536a8a85ef6fe UltraBlame original commit: 43657bf6458152f8a99e25af3bd99094669f6642


        servo: Merge #14141 - Implement home end key scrolling (from samuknet…

…:home-end-key-scroll2); r=glennw  * Refactor all scroll related code to use a new `ScrollLocation` struct which can either be a `delta` (as before) or a `Start` or `End` request, to represent the desire to scroll to the start and end of the page. Effectively, everywhere a delta was used, there is now a `ScrollLocation` struct instead. * Add key press listeners for HOME and END keys so as to cause a scroll to be queued with `ScrollLocation::Start` (in HOME case) or `ScrollLocation::End` (in END case). * These changes depend on added support for the new `ScrollLocation` in webrender and webrender_traits. See servo/webrender#540. ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [ x] These changes fix #13082 (github issue number if applicable).  - [ ] There are tests for these changes OR - [x] These changes do not require tests because scrolling I/O  Source-Repo: https://github.com/servo/servo Source-Revision: 1706ffd6e5a02f26f69970b3b41536a8a85ef6fe UltraBlame original commit: 43657bf6458152f8a99e25af3bd99094669f6642

samuknet mentioned this pull request Nov 8, 2016

Implement home end key scrolling servo/servo#14141

Merged

3 of 4 tasks complete

glennw reviewed Nov 8, 2016

View changes

kvark reviewed Nov 11, 2016

View changes

gterzian suggested changes Nov 13, 2016

View changes

samuknet force-pushed the samuknet:home-end-key-scroll2 branch from a6ac45c to 29837e9 Nov 17, 2016

glennw reviewed Nov 18, 2016

View changes

Sam added 2 commits Nov 17, 2016

Resolve imports conflict.

22aefa1

Implement home end key scrolling support.

efdaeb1

Sam added 3 commits Nov 11, 2016

Fix indentation.

2d0c213

Add types ScrollLocation documentation. Insert continue in layers loo…

6513151

…p instead of early return.

Fix nits, reordered calc, fixed indentation

Loading status checks…

0ee01f3

samuknet force-pushed the samuknet:home-end-key-scroll2 branch from eafeeab to 0ee01f3 Nov 28, 2016

Add Scroll ApiMsg

Loading status checks…

fa805af

bors-servo merged commit fa805af into servo:master Nov 28, 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

bors-servo mentioned this pull request Nov 28, 2016

Implement scroll handoff #599

Merged

servo / webrender

Implement Home end key scroll #540

Implement Home end key scroll #540

samuknet commented Nov 8, 2016 •

edited

glennw left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

samuknet commented Nov 11, 2016

kvark left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

bors-servo commented Nov 14, 2016

glennw left a comment

This comment has been minimized.

This comment has been minimized.

glennw commented Nov 18, 2016

bors-servo commented Nov 22, 2016

glennw commented Nov 28, 2016

samuknet commented Nov 28, 2016

gterzian commented Nov 28, 2016

samuknet commented Nov 28, 2016 •

edited

glennw commented Nov 28, 2016

glennw commented Nov 28, 2016

bors-servo commented Nov 28, 2016

glennw commented Nov 28, 2016

bors-servo commented Nov 28, 2016

bors-servo commented Nov 28, 2016

samuknet commented Nov 30, 2016

servo / webrender

Join GitHub today

Implement Home end key scroll #540

Implement Home end key scroll #540

Conversation

samuknet commented Nov 8, 2016 • edited

glennw left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

samuknet commented Nov 11, 2016

kvark left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

samuknet Nov 17, 2016 • edited

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

gterzian Nov 13, 2016 • edited

This comment has been minimized.

samuknet Nov 17, 2016 • edited

bors-servo commented Nov 14, 2016

glennw left a comment

This comment has been minimized.

This comment has been minimized.

glennw commented Nov 18, 2016

bors-servo commented Nov 22, 2016

glennw commented Nov 28, 2016

samuknet commented Nov 28, 2016

gterzian commented Nov 28, 2016

samuknet commented Nov 28, 2016 • edited

glennw commented Nov 28, 2016

glennw commented Nov 28, 2016

bors-servo commented Nov 28, 2016

glennw commented Nov 28, 2016

bors-servo commented Nov 28, 2016

bors-servo commented Nov 28, 2016

samuknet commented Nov 30, 2016

samuknet commented Nov 8, 2016 •

edited

samuknet Nov 17, 2016 •

edited

gterzian Nov 13, 2016 •

edited

samuknet Nov 17, 2016 •

edited

samuknet commented Nov 28, 2016 •

edited