848489 - need to send 'contextmenu' event if we are long pressing

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect)

Description

[Malini Das [:mdas] - Away, not checking bugmail]

In press/release and in the action chains, we'll need to make sure that 'contextmenu' is fired if we are holding down on the screen and the "ui.click_hold_context_menus.delay" time period is exceeded.

Comment 1

[Ann(Yiming)]

Attachment: longPress

Comment 2

[Ann(Yiming)]

This bug has to wait until Bug 841101(multitouch), Bug 847758(touch_cancel), and Bug 848932(returnFix) get granted.

Comment 3

[Malini Das [:mdas] - Away, not checking bugmail]

Comment on attachment 723658 [details] [diff] [review]
longPress

Review of attachment 723658 [details] [diff] [review]:
-----------------------------------------------------------------

Comments in-line. Also, this code has to work for multi action chains too!

As an example, if we do:

actions1.press(element1).wait(10).release()
actions2.press(element2).wait().release()
multi.add(actions1).add(actions2)
multi.perform()

here, we will have to dispatch 'contextmenu' to both element 1 and element2, since element2 ends up being held for the same period of time as element1, which exceeds the "ui.click_hold_context_menus.delay" timeout

::: testing/marionette/marionette-listener.js
@@ +960,5 @@
> +      let standard = Services.prefs.getIntPref("ui.click_hold_context_menus.delay");
> +      if (finger[i][0] == 'wait' && finger[i][1] != null && finger[i][1]*1000 >= standard) {
> +        finger[i][1] = finger[i][1] - standard/1000;
> +        finger.splice(i, 0, ['wait', standard/1000], ['longPress']);
> +      }

This doesn't work if we do something like

actions.press(element).move(element2).wait(10).release().

You should move the 'contextmenu' firing code into the wait() command, and instead of creating this 'longPress' action, you can just use two timers. One to dispatch the contextmenu (if needed), and the other to progress along the action chain.

Comment 4

[Malini Das [:mdas] - Away, not checking bugmail]

We won't be doing multitouch support since it's unclear when or if contextmenu should be fired with multiple fingers on the screen. Will punt on that until clear specs/requests are given.

Comment 5

[Ann(Yiming)]

Attachment: longPress

Comment 6

[Malini Das [:mdas] - Away, not checking bugmail]

Comment on attachment 727397 [details] [diff] [review]
longPress

Review of attachment 727397 [details] [diff] [review]:
-----------------------------------------------------------------

Can you submit a test for this?

Comment 7

[Ann(Yiming)]

Attachment: addTest

Comment 8

[Malini Das [:mdas] - Away, not checking bugmail]

Comment on attachment 727825 [details] [diff] [review]
addTest

Review of attachment 727825 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/marionette/client/marionette/tests/unit/test_long_press.py
@@ +13,5 @@
> +        button = self.marionette.find_element("id", "mozLinkCopy")
> +        action = Actions(self.marionette)
> +        action.press(button).wait(5).perform()
> +        time.sleep(15)
> +        self.assertEqual("Context", self.marionette.execute_script("return document.getElementById('mozLinkCopy').innerHTML;"))

this is a good start, but we want to be able to verify that the contextmenu event was sent after the expected time, right? What you should do is figure out when the contextmenu event is fired after the touchstart event, and compare it with the expected delay. To get the delay in your test, you can do:

#switch to chrome space so you have access to Components.utils
self.marionette.set_context("chrome")
delay = self.marionette.execute_script("Components.utils.import('resource://gre/modules/Services.jsm'); return Services.prefs.getIntPref('ui.click_hold_context_menus.delay');")
self.marionette.set_context("content")

You can then compare the actual delay and make sure it exceeds or meets this delay. This will provide a much more robust test.

Comment 9

[Malini Das [:mdas] - Away, not checking bugmail]

Ann has looked into this and since nsITimer may call its callback before the given time, then writing a test like this may be error prone, since we'll have to code in some threshold of acceptable delay times. We can drop this for now.

Comment 10

[Malini Das [:mdas] - Away, not checking bugmail]

landed in m-i:

https://hg.mozilla.org/integration/mozilla-inbound/rev/48d2538788ec

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Backed out for Marionette failures.
https://hg.mozilla.org/integration/mozilla-inbound/rev/16091b9202a8

https://tbpl.mozilla.org/php/getParsedLog.php?id=21121325&tree=Mozilla-Inbound

Comment 12

[Malini Das [:mdas] - Away, not checking bugmail]

This was backed out due to an error:

======================================================================
12:41:21     INFO -  ERROR: test_chain (test_single_finger.testSingleFinger)
12:41:21     INFO -  ----------------------------------------------------------------------
12:41:21     INFO -  Traceback (most recent call last):
12:41:21     INFO -    File "/builds/slave/test/build/tests/marionette/tests/testing/marionette/client/marionette/tests/unit/test_single_finger.py", line 49, in test_chain
12:41:21     INFO -      action.press(button1).perform()
12:41:21     INFO -    File "/builds/slave/test/build/tests/marionette/marionette/marionette.py", line 143, in perform
12:41:21     INFO -      self.current_id = self.marionette._send_message('actionChain', 'value', chain=self.action_chain, nextId=self.current_id)
12:41:21     INFO -    File "/builds/slave/test/build/tests/marionette/marionette/marionette.py", line 310, in _send_message
12:41:21     INFO -      self._handle_error(response)
12:41:21     INFO -    File "/builds/slave/test/build/tests/marionette/marionette/marionette.py", line 367, in _handle_error
12:41:21     INFO -      raise MarionetteException(message=message, status=status, stacktrace=stacktrace)
12:41:21     INFO -  MarionetteException: finger[i] is undefined
12:41:21     INFO -  stacktrace:
12:41:21     INFO -  	actions@chrome://marionette/content/marionette-listener.js:972
12:41:21     INFO -  TEST-UNEXPECTED-FAIL | test_single_finger.py testSingleFinger.test_chain | 	actionChain@chrome://marionette/content/marionette-listener.js:1062

full stack trace is here: https://tbpl.mozilla.org/php/getParsedLog.php?id=21121325&tree=Mozilla-Inbound&full=1#error0

Can you reapply this patch on top of latest m-c and fix the error? looks like "MarionetteException: finger[i] is undefined" is the best hint here, so it's in the new marionette-listener code

Comment 13

[Ann(Yiming)]

Attachment: longPress

Comment 14

[Ann(Yiming)]

Attachment: longPress

Comment 15

[Malini Das [:mdas] - Away, not checking bugmail]

Comment on attachment 732093 [details] [diff] [review]
longPress

Great, and thanks for moving the test into the existing test file.

I tested this out these changes and they pass locally

Comment 16

[Malini Das [:mdas] - Away, not checking bugmail]

m-i is busted, will land when I can

Comment 17

[Malini Das [:mdas] - Away, not checking bugmail]

pushed to inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/da3acb67bb23

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/da3acb67bb23

Comment 19

[Malini Das [:mdas] - Away, not checking bugmail]

uplifted to mozilla-b2g18:
https://hg.mozilla.org/releases/mozilla-b2g18/rev/6691a027dbec

Comment 20

[Jonathan Griffin (:jgriffin)]

https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/8d13adf2d153