[Touch Caret] Enable touch caret sanity test on B2G

RESOLVED FIXED in mozilla33

Status

()

Core
Selection
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: TYLin, Assigned: TYLin)

Tracking

unspecified
mozilla33
All
Gonk (Firefox OS)
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

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.
(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.
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.
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

4 years ago
(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.
(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!
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?
Flags: needinfo?(mdas)
Created attachment 8437525 [details] [diff] [review]
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.
Attachment #8437525 - Flags: review?(roc)
Created attachment 8437526 [details] [diff] [review]
part 2 - Enable touch caret sanity test on B2G (WIP)
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

Updated

4 years ago
Blocks: 1023688
(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.
Flags: needinfo?(mdas)
Assignee: nobody → tlin
Depends on: 1019441
Created attachment 8439052 [details] [diff] [review]
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
Attachment #8437526 - Attachment is obsolete: true
Attachment #8439052 - Flags: review?(mdas)
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
Attachment #8439052 - Flags: review?(mdas) → review+
Created attachment 8439711 [details] [diff] [review]
part 1 - Fix touch event handling for touch caret. r=roc (v2, carry r+)

Rebased to latest m-c.
Attachment #8437525 - Attachment is obsolete: true
Attachment #8439711 - Flags: review+
Created attachment 8439713 [details] [diff] [review]
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
Attachment #8439052 - Attachment is obsolete: true
Attachment #8439713 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/4d4c6b90198f
https://hg.mozilla.org/mozilla-central/rev/0e4257f03e72
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in before you can comment on or make changes to this bug.