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)
Tracking
()
RESOLVED
FIXED
mozilla33
People
(Reporter: TYLin, Assigned: TYLin)
References
Details
Attachments
(2 files, 3 obsolete files)
(deleted),
patch
|
TYLin
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
TYLin
:
review+
|
Details | Diff | Splinter Review |
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•10 years ago
|
||
(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.
Assignee | ||
Comment 2•10 years ago
|
||
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.
Assignee | ||
Comment 3•10 years ago
|
||
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.
Comment 5•10 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.
Great, thanks for the quick response!
Assignee | ||
Comment 6•10 years ago
|
||
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)
Assignee | ||
Comment 7•10 years ago
|
||
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)
Assignee | ||
Comment 8•10 years ago
|
||
Assignee | ||
Comment 9•10 years ago
|
||
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
Attachment #8437525 -
Flags: review?(roc) → review+
Blocks: CopyPasteLegacy
Comment 10•10 years ago
|
||
(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 | ||
Comment 11•10 years ago
|
||
* 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 12•10 years ago
|
||
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+
Assignee | ||
Comment 13•10 years ago
|
||
Rebased to latest m-c.
Attachment #8437525 -
Attachment is obsolete: true
Attachment #8439711 -
Flags: review+
Assignee | ||
Comment 14•10 years ago
|
||
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+
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 15•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/4d4c6b90198f
https://hg.mozilla.org/integration/mozilla-inbound/rev/0e4257f03e72
Flags: in-testsuite+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/4d4c6b90198f
https://hg.mozilla.org/mozilla-central/rev/0e4257f03e72
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in
before you can comment on or make changes to this bug.
Description
•