Implement home end key scrolling #14141

samuknet · 2016-11-08T19:09:50Z

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.

./mach build -d does not report any errors
./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
These changes do not require tests because scrolling I/O

This change is

highfive · 2016-11-08T19:09:57Z

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @nox (or someone else) soon.

bors-servo · 2016-11-09T11:42:27Z

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


        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)

gterzian · 2016-12-17T14:54:09Z

@samuknet Have you considered rebasing off the latest Servo? I think it might make the build pass, since this PR is using webrender_traits 0.8.0, and the ScrollLocation you added to webrender was not part of it at version 0.8.0. Since Servo itself has been upgraded to use 0.11.0, rebasing might be just enough.

I'm waiting for this one to land to be able to further test servo/webrender#599 😄

samuknet · 2016-12-19T14:10:42Z

@gterzian Working on it, should be up soon. servo/webrender#599 looks cool :)

samuknet · 2016-12-20T05:19:05Z

@gterzian
I've done the rebase although when testing the home/end key scrolling doesn't actually happen on the webrender side. The webrender_traits repos has not yet been updated to use the new ScrollLocation instead of deltas - https://github.com/servo/webrender_traits/blob/master/src/api.rs#L184. Should I go ahead and PR the changes separately on webrender_traits?

So what's happening is the render_backend is just sending through the deltas with:
if self.frame.scroll(delta, cursor, move_phase)
which is the old way without ScrollLocation.

Although what's unusual is that fn scroll(...) in frame.rs is taking a ScrollLocation which seems different from the way render_backend is calling it? I see that webrender is using the types from the un-updated webrender_traits separate repos although I don't see how fn scroll can be used with a delta signature in render_backend.rs but defined with a ScrollLocation in frame.rs.

gterzian · 2016-12-20T11:52:56Z

@samuknet thanks for doing all this research! I think the webrender_traits repo you are looking at is not used anymore(last update seems to have been 5 months ago), and the code has been moved into the webrender repo.

You can see in the new repo that the ScrollLocation is indeed used in ApiMsg::Scroll

Also you could take a look at this PR because it looks like @glennw has done some work on the Servo side to integrate webrender_traits::ScrollLocation.

I'm not sure why "when testing the home/end key scrolling doesn't actually happen", but maybe with the info above you can get an idea why?

samuknet · 2016-12-20T13:23:41Z

@gterzian - thanks for this info. Found the issue - changes to coordinates in WR meant the home/end safety checks were stopping the scroll in the wrong when they should have been allowing it.

Waiting now on servo/webrender#656.

bors-servo · 2016-12-21T13:12:31Z

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

jdm · 2016-12-21T14:02:48Z

That PR merged, so this is ready for review.

jdm · 2016-12-21T14:03:31Z

It looks like this PR updates a bunch of other dependencies, though, which we probably want to avoid!

samuknet · 2017-01-15T11:19:34Z

@jdm any idea why the travis CI build failed? I suspect something is wrong with the Cargo.lock on this branch.

Thanks! :)

jdm

The travis error is because our script that verifies that no accidental Cargo.lock changes were committed triggered.

jdm · 2017-01-15T16:06:47Z

components/compositing/Cargo.toml

@@ -15,6 +15,7 @@ gfx_traits = {path = "../gfx_traits"}
 gleam = "0.2.8"
 image = "0.12"
 ipc-channel = "0.5"
+layers = {git = "https://github.com/servo/rust-layers", features = ["plugins"]}


This line was added and is not necessary. Probably a mistake while rebasing?

jdm · 2017-01-15T17:19:43Z

r? @glennw

samuknet · 2017-01-23T11:00:04Z

@glennw All done and squashed - ready to go hopefully

jdm · 2017-01-23T12:09:48Z

@bors-servo: r=glennw

bors-servo · 2017-01-23T12:09:49Z

📌 Commit 0c9c343 has been approved by glennw

bors-servo · 2017-01-23T12:10:17Z

⌛ Testing commit 0c9c343 with merge 3535d5b...


        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)

bors-servo · 2017-01-23T12:24:03Z

💔 Test failed - mac-dev-unit

samuknet · 2017-01-23T12:32:14Z

@jdm Looks like this could be fixed by changing the macro in browser_hosts.rs on line 474 to be:

this.downcast().send_window_event(WindowEvent::Scroll(ScrollLocation::Delta(delta),
                                                                  origin,
                                                                  TouchEventType::Move))

This compilation error doesn't occur on my machine, I'm guessing because it depends on the platform?

Ms2ger · 2017-01-23T13:22:52Z

Have you tried running ./mach build-cef?


        Implement home end key scroll.

samuknet · 2017-01-23T13:40:26Z

@Ms2ger Thanks for this - I've just pushed the nescessary changes in browser_hosts.rs.

jdm · 2017-01-23T15:25:58Z

@bors-servo: r=glennw

bors-servo · 2017-01-23T15:25:58Z

📌 Commit 7e4255e has been approved by glennw

bors-servo · 2017-01-23T15:26:02Z

⌛ Testing commit 7e4255e with merge 1706ffd...


        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)

bors-servo · 2017-01-23T16:55:40Z

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

highfive assigned nox Nov 8, 2016

highfive added the S-awaiting-review label Nov 8, 2016

samuknet mentioned this pull request Nov 8, 2016

Implement Home end key scroll servo/webrender#540

Merged

samuknet changed the title ~~Home end key scroll2~~ Implement home end key scrolling Nov 8, 2016

highfive added the S-needs-rebase label Nov 9, 2016

cbrewster added the S-blocked-on-external label Nov 10, 2016

jdm mentioned this pull request Nov 14, 2016

Support scrolling with the `Home` and `End` keys #14212

Closed

2 of 4 tasks complete

samuknet force-pushed the samuknet:home-end-key-scroll2 branch from d9f6ae8 to 5914600 Dec 21, 2016

highfive added the S-needs-rebase label Dec 21, 2016

jdm removed the S-blocked-on-external label Dec 21, 2016

samuknet force-pushed the samuknet:home-end-key-scroll2 branch from 5914600 to 21c5bf8 Dec 21, 2016

jdm removed the S-needs-rebase label Dec 21, 2016

samuknet force-pushed the samuknet:home-end-key-scroll2 branch 2 times, most recently from d1977a3 to 259d4c2 Jan 13, 2017

jdm reviewed Jan 15, 2017

View changes

jdm added the S-needs-squash label Jan 15, 2017

highfive unassigned nox Jan 15, 2017

samuknet force-pushed the samuknet:home-end-key-scroll2 branch 2 times, most recently from 1a8c5c3 to 0c9c343 Jan 21, 2017

highfive added S-awaiting-merge and removed S-awaiting-review S-needs-squash labels Jan 23, 2017

highfive added S-tests-failed and removed S-awaiting-merge labels Jan 23, 2017

Implement home end key scroll.

Loading status checks…

7e4255e

samuknet force-pushed the samuknet:home-end-key-scroll2 branch from 0c9c343 to 7e4255e Jan 23, 2017

highfive added S-awaiting-review and removed S-tests-failed labels Jan 23, 2017

highfive added S-awaiting-merge and removed S-awaiting-review labels Jan 23, 2017

bors-servo merged commit 7e4255e into servo:master Jan 23, 2017
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 Jan 23, 2017

bors-servo mentioned this pull request Jan 23, 2017

update mouse position when receiving mouse wheel events #14808

Closed

3 of 5 tasks complete

servo / servo

Implement home end key scrolling #14141

Implement home end key scrolling #14141

samuknet commented Nov 8, 2016 •

edited

highfive commented Nov 8, 2016

bors-servo commented Nov 9, 2016

gterzian commented Dec 17, 2016 •

edited

samuknet commented Dec 19, 2016

samuknet commented Dec 20, 2016

gterzian commented Dec 20, 2016

samuknet commented Dec 20, 2016

bors-servo commented Dec 21, 2016

jdm commented Dec 21, 2016

jdm commented Dec 21, 2016

samuknet commented Jan 15, 2017

jdm left a comment

This comment has been minimized.

jdm commented Jan 15, 2017

samuknet commented Jan 23, 2017

jdm commented Jan 23, 2017

bors-servo commented Jan 23, 2017

bors-servo commented Jan 23, 2017

bors-servo commented Jan 23, 2017

samuknet commented Jan 23, 2017

Ms2ger commented Jan 23, 2017

samuknet commented Jan 23, 2017

jdm commented Jan 23, 2017

bors-servo commented Jan 23, 2017

bors-servo commented Jan 23, 2017

bors-servo commented Jan 23, 2017

servo / servo

Join GitHub today

Implement home end key scrolling #14141

Implement home end key scrolling #14141

Conversation

samuknet commented Nov 8, 2016 • edited

highfive commented Nov 8, 2016

bors-servo commented Nov 9, 2016

gterzian commented Dec 17, 2016 • edited

samuknet commented Dec 19, 2016

samuknet commented Dec 20, 2016

gterzian commented Dec 20, 2016

samuknet commented Dec 20, 2016

bors-servo commented Dec 21, 2016

jdm commented Dec 21, 2016

jdm commented Dec 21, 2016

samuknet commented Jan 15, 2017

jdm left a comment

This comment has been minimized.

jdm Jan 15, 2017

jdm commented Jan 15, 2017

samuknet commented Jan 23, 2017

jdm commented Jan 23, 2017

bors-servo commented Jan 23, 2017

bors-servo commented Jan 23, 2017

bors-servo commented Jan 23, 2017

samuknet commented Jan 23, 2017

Ms2ger commented Jan 23, 2017

samuknet commented Jan 23, 2017

jdm commented Jan 23, 2017

bors-servo commented Jan 23, 2017

bors-servo commented Jan 23, 2017

bors-servo commented Jan 23, 2017

samuknet commented Nov 8, 2016 •

edited

gterzian commented Dec 17, 2016 •

edited