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

forcetouch events #9811

Merged
merged 2 commits into from Apr 5, 2016
Merged

forcetouch events #9811

merged 2 commits into from Apr 5, 2016

Conversation

@paulrouget
Copy link
Contributor

paulrouget commented Feb 29, 2016

https://developer.apple.com/library/mac/documentation/AppleApplications/Conceptual/SafariJSProgTopics/RespondingtoForceTouchEventsfromJavaScript.html

Not sure how we want to land that yet. Maybe reproduce the webkit events (as in this PR), or as touch/mousemouse events.

Review on Reviewable

@highfive
Copy link

highfive commented Feb 29, 2016

warning Warning warning

  • These commits modify script code, but no tests are modified. Please consider adding a test!
@paulrouget
Copy link
Contributor Author

paulrouget commented Feb 29, 2016

/cc @mbrubeck

@Ms2ger
Copy link
Contributor

Ms2ger commented Mar 1, 2016

No spec at all? Seems like a definite no.

@metajack
Copy link
Contributor

metajack commented Mar 1, 2016

Matthew thought this might be handled in part by touch events 2, but that appears not to be the case. We're still kind of digging around to see where it belongs.

I would like to get it in servo since it is really nice and Safari already has it. It makes for amazing demos.

@paulrouget
Copy link
Contributor Author

paulrouget commented Mar 3, 2016

I'll add a pref.

@paulrouget paulrouget force-pushed the paulrouget:forceTouch branch 4 times, most recently from a98e42f to 656f7a8 Mar 16, 2016
@paulrouget paulrouget changed the title [WIP] forcetouch events forcetouch events Mar 18, 2016
@paulrouget
Copy link
Contributor Author

paulrouget commented Mar 18, 2016

The events only fire if the pref dom.forcetouch.enabled is set.
Not sure how I should test that.

Can someone look at this and tell what needs to be done to land that?

@glennw glennw assigned mbrubeck and unassigned glennw Mar 20, 2016
@paulrouget
Copy link
Contributor Author

paulrouget commented Mar 21, 2016

We probably want this too: #10081

@mbrubeck
Copy link
Contributor

mbrubeck commented Mar 21, 2016

The code looks good to me, with some minor comments (below).

Before I r+ this, any objections to landing this non-standard feature behind a pref and prefix? Do we need to block on #10081? Do we need a way to make "chrome-only" features?

-S-awaiting-review +S-needs-code-changes +S-needs-decision


Reviewed 1 of 10 files at r1, 10 of 10 files at r2.
Review status: all files reviewed at latest revision, 2 unresolved discussions.


components/script/dom/document.rs, line 218 [r2] (raw file):
Since we expect stage to be 0, 1, or 2, it would be nice to use an enum instead of an i64, though maybe that should go in glutin.


components/script/dom/document.rs, line 798 [r2] (raw file):
From the Safari docs, mouseforcechanged should be dispatched only after mousedown and before mouseup. (I think this means it should be dispatched only if stage > 1).


Comments from the review on Reviewable.io

@paulrouget
Copy link
Contributor Author

paulrouget commented Mar 21, 2016

Do we need a way to make "chrome-only" features?

Just FYI, so far, "chrome" features are: Mozbrowser API and cross-origin xhr (WIP #10068). Both are only available if 1) the mozbrowser pref is turned on 2) the API is used in the root pipeline.

@paulrouget paulrouget force-pushed the paulrouget:forceTouch branch from 656f7a8 to 35e3ae2 Mar 22, 2016
@paulrouget
Copy link
Contributor Author

paulrouget commented Mar 22, 2016

Addressed comments.

I remove webkitmouseforcewillbegin for now. It's only useful if the preventDefault() works, which I don't think it is yet. I will implement it in a follow-up bug (and make sure browserhtml actually follow this behavior).

@mbrubeck
Copy link
Contributor

mbrubeck commented Mar 22, 2016

@bors-servo r+


Reviewed 10 of 10 files at r3.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from the review on Reviewable.io

@bors-servo
Copy link
Contributor

bors-servo commented Mar 22, 2016

📌 Commit 35e3ae2 has been approved by mbrubeck

@paulrouget paulrouget force-pushed the paulrouget:forceTouch branch from 9c6e88e to ca0a3d7 Apr 1, 2016
@paulrouget
Copy link
Contributor Author

paulrouget commented Apr 1, 2016

(rebased and added pref to prefs.json)

@paulrouget paulrouget force-pushed the paulrouget:forceTouch branch from ca0a3d7 to 0d52927 Apr 5, 2016
@paulrouget
Copy link
Contributor Author

paulrouget commented Apr 5, 2016

I'm not sure when #10081 will land. So I've updated my PR and added ForceTouchEvent to interfaces.html. This test will fail with #10081.

@paulrouget
Copy link
Contributor Author

paulrouget commented Apr 5, 2016

@bors-servo retry

infra

@jdm
Copy link
Member

jdm commented Apr 5, 2016

@bors-servo: r=mbrubeck

@bors-servo
Copy link
Contributor

bors-servo commented Apr 5, 2016

📌 Commit 0d52927 has been approved by mbrubeck

@bors-servo
Copy link
Contributor

bors-servo commented Apr 5, 2016

Testing commit 0d52927 with merge c0d03e2...

bors-servo added a commit that referenced this pull request Apr 5, 2016
forcetouch events

https://developer.apple.com/library/mac/documentation/AppleApplications/Conceptual/SafariJSProgTopics/RespondingtoForceTouchEventsfromJavaScript.html

Not sure how we want to land that yet. Maybe reproduce the webkit events (as in this PR), or as touch/mousemouse events.

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.svg" height="40" alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/9811)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Apr 5, 2016

💔 Test failed - mac-rel-wpt

@larsbergstrom
Copy link
Contributor

larsbergstrom commented Apr 5, 2016

Tests with unexpected results:
  ▶ TIMEOUT [expected OK] /cssom-view/scrollingElement.html
  │ 
  │ ERROR:js::rust: Error at http://web-platform.test:8000/cssom-view/scrollingElement.html:14:4: URL.createObjectURL is not a function
  │ 
  │ ERROR:js::rust: Error at http://web-platform.test:8000/cssom-view/scrollingElement.html:11:6: URL.createObjectURL is not a function
  └ 

  ▶ Unexpected subtest result in /cssom-view/scrollingElement.html:
  └ NOTRUN [expected FAIL] Tests for scrollingElement

@jdm
Copy link
Member

jdm commented Apr 5, 2016

@bors-servo
Copy link
Contributor

bors-servo commented Apr 5, 2016

Testing commit 0d52927 with merge 0ff8adb...

bors-servo added a commit that referenced this pull request Apr 5, 2016
forcetouch events

https://developer.apple.com/library/mac/documentation/AppleApplications/Conceptual/SafariJSProgTopics/RespondingtoForceTouchEventsfromJavaScript.html

Not sure how we want to land that yet. Maybe reproduce the webkit events (as in this PR), or as touch/mousemouse events.

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.svg" height="40" alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/9811)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Apr 5, 2016

@bors-servo bors-servo merged commit 0d52927 into servo:master Apr 5, 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
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.

None yet

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