Closed
Bug 1285070
Opened 9 years ago
Closed 9 years ago
Real touch events will generate corresponding pointer events twice
Categories
(Core :: DOM: Events, defect)
Tracking
()
RESOLVED
FIXED
mozilla51
| Tracking | Status | |
|---|---|---|
| firefox51 | --- | fixed |
People
(Reporter: stone, Assigned: stone)
References
Details
(Whiteboard: btpp-active)
Attachments
(1 file, 3 obsolete files)
|
5.66 KB,
patch
|
stone
:
review+
|
Details | Diff | Splinter Review |
Test environment
1) device with touch screen
2) windows10
3) firefox with browser.tabs.remote.force-enable=true, dom.w3c_pointer_events.enabled=true
|
Assignee | |
Updated•9 years ago
|
Assignee: nobody → sshih
|
||
Updated•9 years ago
|
Whiteboard: btpp-active
|
|
Assignee | |
Comment 1•9 years ago
|
||
|
|
Assignee | |
Updated•9 years ago
|
Attachment #8769643 -
Flags: review?(bugmail)
|
||
Comment 2•9 years ago
|
||
Comment on attachment 8769643 [details] [diff] [review]
Real touch events will generate corresponding pointer events twice.
Review of attachment 8769643 [details] [diff] [review]:
-----------------------------------------------------------------
The fix is fine, but the test needs a little cleanup. See comments below.
::: gfx/layers/apz/test/mochitest/mochitest.ini
@@ +61,5 @@
> +[test_bug1285070.html]
> +# Windows touch injection doesn't work in automation, but this test can be run locally on a windows touch device.
> +# On OS X we don't support touch events at all.
> +# Bug 1178701 - Issue on 'B2G ICS Emulator' and 'Android'
> +skip-if = (toolkit == 'windows') || (toolkit == 'cocoa') || (toolkit == 'gonk') || (os == 'android')
\ No newline at end of file
Bug 1178701 is closed, please remove that comment, and change the skip-if line to just the windows and cocoa clauses. (i.e. enable the test on gonk and android)
::: gfx/layers/apz/test/mochitest/test_bug1285070.html
@@ +23,5 @@
> + testElement.addEventListener(elem, (event) => {
> + ok(true, "receiving event " + event.type);
> + ++pointerEventsCount[event.type];
> + if (event.type == "pointerleave") {
> + window.postMessage("finish", "http://mochi.test:8888");
Why are you doing this in a postMessage? It seems unnecessarily complicated, you could just do it as a setTimeout(..., 0);
@@ +38,5 @@
> + window.addEventListener("message", (event) => {
> + if (event.data == "finish") {
> + for (var key in pointerEventsCount) {
> + if (pointerEventsCount.hasOwnProperty(key)) {
> + is(pointerEventsCount[key], 1, "event" + key + " should be generated once");
Do you need this hasOwnProperty check? You initialize everything to zero anyways so this seems unnecessary, just do the is(...) check.
@@ +64,5 @@
> + }
> + SimpleTest.waitForExplicitFinish();
> + </script>
> +</head>
> +<body onload="setup()">
For APZ tests, onload is not sufficient to ensure the APZ state is properly set up, as we need to wait until the APZ structures are initialized properly as well. Remove the setup() function and just do this:
if (isApzEnabled()) {
SimpleTest.waitForExplicitFinish();
pushPrefs([["dom.w3c_pointer_events.enabled", true]])
.then(waitUntilApzStable())
.then(test);
}
Note that in the test environment window.TouchEvent is always defined, because of http://searchfox.org/mozilla-central/rev/bc94fc8653d9821caf87f5af0e2cd830a09ca8d3/testing/profiles/prefs_general.js#50 so there's actually not much point in checking that. I know a few other tests do it but you don't really need it. isApzEnabled() already has some stuff built-in for the APZ-disabled case so you don't need to explicitly handle that either.
Attachment #8769643 -
Flags: review?(bugmail) → review-
|
|
Assignee | |
Comment 3•9 years ago
|
||
Attachment #8769643 -
Attachment is obsolete: true
|
|
Assignee | |
Comment 4•9 years ago
|
||
Followed reviewer's comments to refine the patch.
Attachment #8771254 -
Attachment is obsolete: true
|
|
Assignee | |
Updated•9 years ago
|
Attachment #8771256 -
Flags: review?(bugmail)
|
|
||
Comment 5•9 years ago
|
||
Comment on attachment 8771256 [details] [diff] [review]
Real touch events will generate corresponding pointer events twice. r?kats
Review of attachment 8771256 [details] [diff] [review]:
-----------------------------------------------------------------
This looks good, thanks. Please do a try run to verify the test passes on Linux and Android before landing.
::: gfx/layers/apz/test/mochitest/test_bug1285070.html
@@ +12,5 @@
> + <script type="application/javascript">
> +
> + var subtests = [
> + {'file': 'helper_bug1285070.html', 'prefs': [["dom.w3c_pointer_events.enabled", true]]}
> + ]
nit: add a semicolon after the ]
Attachment #8771256 -
Flags: review?(bugmail) → review+
|
|
Assignee | |
Comment 6•9 years ago
|
||
Updated the patch summary and followed reviewer's comments to refine the patch
Attachment #8771256 -
Attachment is obsolete: true
Attachment #8776838 -
Flags: review+
|
|
Assignee | |
Comment 7•9 years ago
|
||
|
|
Assignee | |
Updated•9 years ago
|
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/79415ce5af65
Real touch events will generate corresponding pointer events twice. r=kats
Keywords: checkin-needed
|
||
Comment 9•9 years ago
|
||
| bugherder | ||
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox51:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in
before you can comment on or make changes to this bug.
Description
•