Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement WheelEvent interface #23154

Merged
merged 2 commits into from Jun 9, 2019
Merged

Conversation

@robert-snakard
Copy link
Contributor

robert-snakard commented Apr 2, 2019

Created a new dom interface: "WheelEvent" and added WPT tests to confirm the interface works. To do this I had to do the following:

  • Create a new WheelEvent dom interface. It can be found in script/dom/wheelevent.rs and dom/webidls/WheelEvent.webidl
  • Add a new WheelEvent option to the compositor's CompositorEvent enum
  • Add a new Wheel option to the compositor's WindowEvent enum
  • Add a new WheelDelta type to the script_traits module
  • Modify the scroll_event logic. Now we send a WheelEvent before scrolling. Repeat: we send the WheelEvent notification BEFORE we send the scroll delta.
  • Add two manual wpt tests to the uievents/order-of-events test collection

  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #22843 (GitHub issue number if applicable)
  • There are tests for these changes OR
  • These changes do not require tests because ___

This change is Reviewable

@highfive
Copy link

highfive commented Apr 2, 2019

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

@highfive
Copy link

highfive commented Apr 2, 2019

Heads up! This PR modifies the following files:

  • @asajeffrey: components/script/dom/mod.rs, components/script/dom/mouseevent.rs, components/script/dom/window.rs, components/script/dom/document.rs, components/script/script_thread.rs and 3 more
  • @wafflespeanut: python/servo/build_commands.py
  • @cbrewster: components/constellation/constellation.rs
  • @paulrouget: ports/servo/glutin_app/window.rs, components/servo/lib.rs, ports/servo/browser.rs, components/compositing/compositor.rs, components/compositing/windowing.rs and 1 more
  • @KiChjang: components/script/dom/mod.rs, components/script/dom/mouseevent.rs, components/script/dom/window.rs, components/script_traits/lib.rs, components/script/dom/document.rs and 3 more
@highfive
Copy link

highfive commented Apr 2, 2019

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!
@robert-snakard
Copy link
Contributor Author

robert-snakard commented Apr 2, 2019

@servo-wpt-sync
Copy link
Collaborator

servo-wpt-sync commented Apr 2, 2019

Error syncing changes upstream. Logs saved in error-snapshot-1554165902717.

@jdm
Copy link
Member

jdm commented Apr 2, 2019

@robert-snakard Looks like this needs to be rebased in a way that doesn't include commits which are already present on master.

@jdm jdm added the S-needs-rebase label Apr 2, 2019
@robert-snakard
Copy link
Contributor Author

robert-snakard commented Apr 2, 2019

This should just be master with 2 commits on top...

@jdm
Copy link
Member

jdm commented Apr 2, 2019

It's possible that because the @bors-servo merge commits are missing it is not actually equivalent to master. I recommend doing a hard reset to master and cherry-picking your two commits, then force pushing.

@servo-wpt-sync
Copy link
Collaborator

servo-wpt-sync commented Apr 2, 2019

Opened new PR for upstreamable changes.

Completed upstream sync of web-platform-test changes at web-platform-tests/wpt#16199.

@jdm jdm changed the title Issue 22843 Implement WheelEvent interface Apr 2, 2019
@servo-wpt-sync
Copy link
Collaborator

servo-wpt-sync commented Apr 2, 2019

PR title changed; changed existing PR.

Completed upstream sync of web-platform-test changes at web-platform-tests/wpt#16199.

@robert-snakard
Copy link
Contributor Author

robert-snakard commented Apr 3, 2019

@jdm Updated the branch version and tested on my machine, the patch still works. Do I need to do anything else?

@jdm
Copy link
Member

jdm commented Apr 8, 2019

No, we just need to review these changes now.

@jdm
Copy link
Member

jdm commented Apr 8, 2019

@paulrouget Review ping?

@jdm
Copy link
Member

jdm commented Apr 9, 2019

@bors-servo try=wpt

bors-servo added a commit that referenced this pull request Apr 9, 2019
Implement WheelEvent interface

Created a new dom interface: "WheelEvent" and added WPT tests to confirm the interface works. To do this I had to do the following:

- Create a new `WheelEvent` dom interface. It can be found in `script/dom/wheelevent.rs` and `dom/webidls/WheelEvent.webidl`
- Add a new `WheelEvent` option to the compositor's `CompositorEvent` enum
- Add a new `Wheel` option to the compositor's `WindowEvent` enum
- Add a new `WheelDelta` type to the `script_traits` module
- Modify the `scroll_event` logic. Now we send a `WheelEvent` before scrolling. Repeat: we send the WheelEvent notification BEFORE we send the scroll delta.
- Add two manual wpt tests to the `uievents/order-of-events` test collection

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #22843 (GitHub issue number if applicable)

<!-- Either: -->
- [X] There are tests for these changes OR
- [ ] These changes do not require tests because ___

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/23154)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Apr 9, 2019

Trying commit 9e01a45 with merge cdd4cb0...

@bors-servo
Copy link
Contributor

bors-servo commented Apr 9, 2019

💔 Test failed - linux-rel-css

@robert-snakard
Copy link
Contributor Author

robert-snakard commented Apr 9, 2019

This failed because tests that were supposed to fail are now passing, looking for the idlharness now to update it

@robert-snakard robert-snakard force-pushed the robert-snakard:issue_22843 branch from 9e01a45 to 99e99b8 Apr 9, 2019
@highfive highfive removed the S-tests-failed label Apr 9, 2019
@robert-snakard
Copy link
Contributor Author

robert-snakard commented Apr 9, 2019

@bors-servo try=wpt

@bors-servo
Copy link
Contributor

bors-servo commented Apr 9, 2019

@robert-snakard: 🔑 Insufficient privileges: not in try users

@jdm
Copy link
Member

jdm commented Jun 7, 2019


  ▶ Unexpected subtest result in /uievents/idlharness.window.html:
  └ PASS [expected FAIL] MouseEvent interface: new WheelEvent("event") must inherit property "buttons" with the proper type
@robert-snakard
Copy link
Contributor Author

robert-snakard commented Jun 7, 2019

@jdm
Copy link
Member

jdm commented Jun 7, 2019

You can run individual tests or directories with ./mach test-wpt /uievents/idlharness.window.html or ./mach test-wpt /uievents/ (for example), but running all of the tests locally is a mostly futile effort because there are so many of them.

@jdm
Copy link
Member

jdm commented Jun 7, 2019

Looks like these tests as well:

  ▶ Unexpected subtest result in /dom/events/Event-subclasses-constructors.html:
  └ PASS [expected FAIL] WheelEvent constructor (no argument)

  ▶ Unexpected subtest result in /dom/events/Event-subclasses-constructors.html:
  └ PASS [expected FAIL] WheelEvent constructor (undefined argument)

  ▶ Unexpected subtest result in /dom/events/Event-subclasses-constructors.html:
  └ PASS [expected FAIL] WheelEvent constructor (null argument)

  ▶ Unexpected subtest result in /dom/events/Event-subclasses-constructors.html:
  └ PASS [expected FAIL] WheelEvent constructor (empty argument)

  ▶ Unexpected subtest result in /dom/events/Event-subclasses-constructors.html:
  └ PASS [expected FAIL] WheelEvent constructor (argument with default values)

Otherwise it should be ready :)

@robert-snakard robert-snakard force-pushed the robert-snakard:issue_22843 branch from c44a49c to cc16f43 Jun 8, 2019
@robert-snakard robert-snakard force-pushed the robert-snakard:issue_22843 branch from cc16f43 to 219df9b Jun 8, 2019
@servo-wpt-sync
Copy link
Collaborator

servo-wpt-sync commented Jun 8, 2019

Error syncing changes upstream. Logs saved in error-snapshot-1559972947848.

@servo-wpt-sync
Copy link
Collaborator

servo-wpt-sync commented Jun 8, 2019

Error syncing changes upstream. Logs saved in error-snapshot-1559972971985.

Note: The WheelEvent interface supports rotation in all 3 spatial
dimensions. This implementation only supports two due to limitations
in the Glutin compositor.

The wheelevent interface is a dom interface that triggers for any
attached device that can rotate in one or more spatial dimensions.
Traditionally this is the mouse wheel though other devices could be
used as well. E.g. the trackball on a trackball mouse.
The wheelevent sends a signal when a supported device is rotated in the
x, y, or z rotational dimensions. These tests only check whether this
signal is received for any rotation at all.
@jdm jdm force-pushed the robert-snakard:issue_22843 branch from 219df9b to 52a8f5a Jun 9, 2019
@servo-wpt-sync
Copy link
Collaborator

servo-wpt-sync commented Jun 9, 2019

Transplanted upstreamable changes to existing PR.

Completed upstream sync of web-platform-test changes at web-platform-tests/wpt#16199.

@jdm
Copy link
Member

jdm commented Jun 9, 2019

@bors-servo r=paulrouget

@bors-servo
Copy link
Contributor

bors-servo commented Jun 9, 2019

📌 Commit 52a8f5a has been approved by paulrouget

@bors-servo
Copy link
Contributor

bors-servo commented Jun 9, 2019

Testing commit 52a8f5a with merge 973a344...

bors-servo added a commit that referenced this pull request Jun 9, 2019
Implement WheelEvent interface

Created a new dom interface: "WheelEvent" and added WPT tests to confirm the interface works. To do this I had to do the following:

- Create a new `WheelEvent` dom interface. It can be found in `script/dom/wheelevent.rs` and `dom/webidls/WheelEvent.webidl`
- Add a new `WheelEvent` option to the compositor's `CompositorEvent` enum
- Add a new `Wheel` option to the compositor's `WindowEvent` enum
- Add a new `WheelDelta` type to the `script_traits` module
- Modify the `scroll_event` logic. Now we send a `WheelEvent` before scrolling. Repeat: we send the WheelEvent notification BEFORE we send the scroll delta.
- Add two manual wpt tests to the `uievents/order-of-events` test collection

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #22843 (GitHub issue number if applicable)

<!-- Either: -->
- [X] There are tests for these changes OR
- [ ] These changes do not require tests because ___

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/23154)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Jun 9, 2019

☀️ Test successful - arm64, linux-rel-css, linux-rel-wpt, status-taskcluster
Approved by: paulrouget
Pushing 973a344 to master...

@bors-servo bors-servo merged commit 52a8f5a into servo:master Jun 9, 2019
3 of 4 checks passed
3 of 4 checks passed
Travis CI - Pull Request Build Failed
Details
Taskcluster (pull_request) TaskGroup: success
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details
@robert-snakard
Copy link
Contributor Author

robert-snakard commented Jul 17, 2019

Thanks for finishing the merge up for me @jdm / @paulrouget

@jdm
Copy link
Member

jdm commented Jul 17, 2019

Thanks for doing the hard work :)

@robert-snakard
Copy link
Contributor Author

robert-snakard commented Jul 17, 2019

Haha, we must have different definitions of hard work, that merge took forever!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

7 participants
You can’t perform that action at this time.