Closed Bug 1020261 Opened 10 years ago Closed 10 years ago

[Touch Caret] Enable touch caret sanity test on B2G

Categories

(Core :: DOM: Selection, defect)

All
Gonk (Firefox OS)
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: TYLin, Assigned: TYLin)

References

Details

Attachments

(2 files, 3 obsolete files)

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.
(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)
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)
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
(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
* 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+
Depends on: 962645
Rebased to latest m-c.
Attachment #8437525 - Attachment is obsolete: true
Attachment #8439711 - Flags: review+
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
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: