Closed Bug 1285070 Opened 3 years ago Closed 3 years ago

Real touch events will generate corresponding pointer events twice

Categories

(Core :: DOM: Events, defect)

Unspecified
Windows
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: stone, Assigned: stone)

References

Details

(Whiteboard: btpp-active)

Attachments

(1 file, 3 obsolete files)

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: nobody → sshih
Whiteboard: btpp-active
Blocks: 1166347
Attachment #8769643 - Flags: review?(bugmail)
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-
Followed reviewer's comments to refine the patch.
Attachment #8771254 - Attachment is obsolete: true
Attachment #8771256 - Flags: review?(bugmail)
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+
Updated the patch summary and followed reviewer's comments to refine the patch
Attachment #8771256 - Attachment is obsolete: true
Attachment #8776838 - Flags: review+
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
https://hg.mozilla.org/mozilla-central/rev/79415ce5af65
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Duplicate of this bug: 1292041
You need to log in before you can comment on or make changes to this bug.