1020261 - [Touch Caret] Enable touch caret sanity test on B2G

Status: RESOLVED FIXED

(Core :: DOM: Selection, defect)

Description

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Marionette sends synthesized touch event with unique id started from 1000. 
http://hg.mozilla.org/mozilla-central/file/668f29cd71b3/testing/marionette/marionette-listener.js#l67

However, in TouchCaret::HandleTouchDownEvent(), only touch event with id 0 is recognized as a valid one.
http://hg.mozilla.org/mozilla-central/file/668f29cd71b3/layout/base/TouchCaret.cpp#l750

We need to find a way to resolve this in order to enable touch caret sanity tests on B2G.

Comment 1

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

(In reply to Ting-Yu Lin [:TYLin] from comment #0)
> However, in TouchCaret::HandleTouchDownEvent(), only touch event with id 0
> is recognized as a valid one.
> http://hg.mozilla.org/mozilla-central/file/668f29cd71b3/layout/base/
> TouchCaret.cpp#l750

Interesting, I'm guessing the reason they assume touchid 0 is because they do not want a multifinger gesture to trigger a caret? If that is the case, then it seems that touchids increment AND decrement based on the touchstart and touchend/cancel, which was not taken into account when writing Marionette. In Marionette, we based our code on the touch specification, and the only requirement about touchids were that they had to be unique, so we just enforced uniqueness by just incrementing a very high base value.

We can update Marionette if we can confirm this is the case.

Comment 2

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

AFAIK, the reason they assume touchid 0 is because they do not want a multifinger gesture to trigger zoom in/out while moving the touch caret. You are correct that touchid is unique and it will increment and decrement based on touchstart and touchend.

On device, if you put one finger on the screen, the touch event will contain a touch with id 0. Then you put second finger on the screen, the touch event will contain two touches with id 0 and 1. You then leave all your fingers, and put one finger on the screen. The touchid will be 0 again.

It will be a great if you could make the touchid of marionette's touch events consistent with the real touch events so that we could utilize marionette on touch caret tests on B2G.

Comment 3

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

After discussing with colleagues who are familiar with touch caret implementation, we conclude that it is  better to modify touch caret code to remove the assumption of touchid 0, and make touch caret recognizes the touchid of the first finger. Therefore, it should be OK that marionette stays as it is.

Comment 4

[u459114]

(In reply to Ting-Yu Lin [:TYLin] from comment #3)
> After discussing with colleagues who are familiar with touch caret
> implementation, we conclude that it is  better to modify touch caret code to
> remove the assumption of touchid 0, and make touch caret recognizes the
> touchid of the first finger. Therefore, it should be OK that marionette
> stays as it is.

I think it's a right decision. Assume "0" is the ID of the first stoke is not robust.

Comment 5

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

(In reply to Ting-Yu Lin [:TYLin] from comment #3)
> After discussing with colleagues who are familiar with touch caret
> implementation, we conclude that it is  better to modify touch caret code to
> remove the assumption of touchid 0, and make touch caret recognizes the
> touchid of the first finger. Therefore, it should be OK that marionette
> stays as it is.

Great, thanks for the quick response!

Comment 6

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Hi Malini,

I got a "MarionetteException: Element has not been pressed" when executing this line on b2g emulator.

self.actions.wait(timeout).flick(el, src_x, src_y, dest_x, dest_y).perform()

http://dxr.mozilla.org/mozilla-central/source/layout/base/tests/marionette/test_touchcaret.py?from=test_touchcaret.py&case=true#214

Can Action.wait() be called without calling a Action.press()? It seems wait() before press() is blocked by this check.
http://dxr.mozilla.org/mozilla-central/source/testing/marionette/marionette-listener.js?from=marionette-listener.js#969

If this is by design, should I use Python's time.sleep() instead if I would like to pause the script?

Comment 7

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Attachment: part 1 - Fix touch event handling for touch caret

For touch events generated by marionette, the touch id is not started
from 0. Therefore, instead of getting the event position of touch id 0
directly in HandleTouchDownEvent(), we should loop over all the touch
ids to get their positions.

Comment 8

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Attachment: part 2 - Enable touch caret sanity test on B2G (WIP)

Comment 9

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Part 1 is ready for review while part 2 needs clarification. 

opt (Mnw, Gu): https://tbpl.mozilla.org/?tree=Try&rev=25e9fe277412
debug (Mn): https://tbpl.mozilla.org/?tree=Try&rev=6495ff17f6f8

Comment 10

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

(In reply to Ting-Yu Lin [:TYLin] from comment #6)
> Hi Malini,
> 
> I got a "MarionetteException: Element has not been pressed" when executing
> this line on b2g emulator.
> 
> self.actions.wait(timeout).flick(el, src_x, src_y, dest_x, dest_y).perform()
> 
> http://dxr.mozilla.org/mozilla-central/source/layout/base/tests/marionette/
> test_touchcaret.py?from=test_touchcaret.py&case=true#214
> 
> Can Action.wait() be called without calling a Action.press()? It seems
> wait() before press() is blocked by this check.
> http://dxr.mozilla.org/mozilla-central/source/testing/marionette/marionette-
> listener.js?from=marionette-listener.js#969
> 
> If this is by design, should I use Python's time.sleep() instead if I would
> like to pause the script?

Thanks, you've found a bug! I've filed Bug 1023968 about it.

Comment 11

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Attachment: part 2 - Enable touch caret sanity test on B2G (v1)

* Rebase and make this patch depands on bug 1019441.
* Workaroud exception raised when wait() before press() in bug 962645.

Try opt (Mnw, Gu): https://tbpl.mozilla.org/?tree=Try&rev=6b76f4195705
Try debug (Mn): https://tbpl.mozilla.org/?tree=Try&rev=6ab0dbec3c3d

Comment 12

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

Comment on attachment 8439052 [details] [diff] [review]
part 2 - Enable touch caret sanity test on B2G (v1)

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

lgtm

::: layout/base/tests/marionette/test_touchcaret.py
@@ +143,5 @@
>          src_x, src_y = sel.touch_caret_location()
>          dest_x, dest_y = 0, 0
> +
> +        # TODO: After bug 962645 is resolved, the following line could be
> +        # changed to: self.actions.wait(timeout)

I've r+'d your patch and marked it for checkin, so you should be able to change this to a wait() if you like

Comment 13

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Attachment: part 1 - Fix touch event handling for touch caret. r=roc (v2, carry r+)

Rebased to latest m-c.

Comment 14

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Attachment: part 2 - Enable touch caret sanity test on B2G. r=mdas (v2, carry r+)

Remove workaround to pause on server-side due to bug 962645 is resolved.

Try opt (Mnw, Gu, Mn): https://tbpl.mozilla.org/?tree=Try&rev=e2bd578e5a3b
Try debug (Mn): https://tbpl.mozilla.org/?tree=Try&rev=8d28d431f616

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4d4c6b90198f
https://hg.mozilla.org/integration/mozilla-inbound/rev/0e4257f03e72

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/4d4c6b90198f
https://hg.mozilla.org/mozilla-central/rev/0e4257f03e72