Last Comment Bug 816514 - [B2G] When using marionette in the Keyboard frame the frame loses focus
: [B2G] When using marionette in the Keyboard frame the frame loses focus
Status: RESOLVED FIXED
:
Product: Testing
Classification: Components
Component: Marionette (show other bugs)
: unspecified
: ARM Gonk (Firefox OS)
: -- major (vote)
: mozilla20
Assigned To: Walter Chen[:ypwalter][:wachen]
: Walter Chen[:ypwalter][:wachen]
Mentors:
Depends on:
Blocks: 801898
  Show dependency treegraph
 
Reported: 2012-11-29 07:39 PST by Zac C (:zac)
Modified: 2012-12-19 09:16 PST (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
+
fixed
fixed
fixed


Attachments
Keyboard test case (2.00 KB, text/plain)
2012-11-29 07:39 PST, Zac C (:zac)
no flags Details
Patch for review v1 (12.99 KB, patch)
2012-12-06 19:04 PST, Walter Chen[:ypwalter][:wachen]
jgriffin: review-
Details | Diff | Splinter Review
Patch for review v2.1 (1.83 KB, patch)
2012-12-10 19:12 PST, Walter Chen[:ypwalter][:wachen]
jgriffin: review-
Details | Diff | Splinter Review
v3 (5.58 KB, patch)
2012-12-12 18:19 PST, Walter Chen[:ypwalter][:wachen]
jgriffin: review+
Details | Diff | Splinter Review
This is revised according to the reviewer's feedback (5.77 KB, text/plain)
2012-12-16 19:13 PST, Walter Chen[:ypwalter][:wachen]
no flags Details

Description Zac C (:zac) 2012-11-29 07:39:20 PST
Created attachment 686591 [details]
Keyboard test case

This test loads the UI tests app and goes to a basic <input> field. Then activates the field which loads the Keyboard.

It then switches to the keyboard iframe. When marionette switches into the keyboard frame the keyboard hides again and the cursor in the UI Tests app disappears. The focus or something is being broken.

The find/click calls work but no text ends up anywhere visible in the UI.
Comment 1 Stephen Donner [:stephend] 2012-11-29 07:44:35 PST
I'm going to go out on a limb and say we need actual keyboard tests as part of the smoketests, so tentatively marking as blocking bug 801898.
Comment 2 Walter Chen[:ypwalter][:wachen] 2012-12-05 01:20:44 PST
I will take this

I think I found the solution. 
But, I need a little bit more time since I am going to take off from office soon (+8 timezone)

I would have to ask for a code review tmr for both marionette client and server. Maybe it would come with a unit test code so that other people can use it.
Comment 3 Walter Chen[:ypwalter][:wachen] 2012-12-06 02:08:15 PST
Would ask for marionette server/client review tomorrow for this.
Comment 4 Walter Chen[:ypwalter][:wachen] 2012-12-06 19:04:43 PST
Created attachment 689530 [details] [diff] [review]
Patch for review v1

I attached a patch for reviewing (v1)
Please help to review
Comment 5 Stephen Donner [:stephend] 2012-12-06 19:08:17 PST
(In reply to Walter Chen from comment #4)
> Created attachment 689530 [details] [diff] [review]
> Patch for review v1
> 
> I attached a patch for reviewing (v1)
> Please help to review

You'll want to set reviews within Bugzilla, then: https://wiki.mozilla.org/Bugzilla:Review
Comment 6 Jonathan Griffin (:jgriffin) 2012-12-07 13:39:08 PST
Comment on attachment 689530 [details] [diff] [review]
Patch for review v1

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

Thanks for the patch!  I think the essence of what this does is correct, but we can do it with a lot less code duplication.

Instead of defining a new switch_to_keyboard comment in Marionette, we can just add a new parameter to switch_to_frame, e.g., 

     def switch_to_frame(self, frame=None, focus=True):
         if isinstance(frame, HTMLElement):
             response = self._send_message('switchToFrame', 'ok', element=frame.id, focus=focus)
         else:
             response = self._send_message('switchToFrame', 'ok', value=frame, focus=focus)
         return response

and then modify switchToFrame in marionette-actors.js to use this new focus field, similar to what you've already done in marionette-listener.js.  Then, when someone wants to switch to the keyboard frame, they can just use switch_to_frame(foo, focus=False).

Let me know if any of that isn't clear.
Comment 7 Walter Chen[:ypwalter][:wachen] 2012-12-09 22:17:13 PST
Ok, I finished the modification.
However, I don't know where should I branch from to ask for marionette client review.
Any ideas?
Comment 8 Walter Chen[:ypwalter][:wachen] 2012-12-10 01:42:34 PST
Hi, AutomatedTester, 
I need your help so that I can find the trunk of marionette client code.
After that, I can branch it, get a patch, and get reviewed

Please help me. I appreciate it.
Comment 9 Dave Hunt (:davehunt) 2012-12-10 04:14:43 PST
Walter: the canonical location for the Marionette client code is mozilla-central in /testing/marionette/client it's also sync'd to github. You could provide a patch based on either.

http://hg.mozilla.org/mozilla-central/file/ee311a1282ef/testing/marionette/client
https://github.com/mozilla/marionette_client
Comment 10 David Burns :automatedtester 2012-12-10 08:11:31 PST
Walter, as :davehunt says, please use our canonical repo of mozilla-central and then someone in the ateam can merge the python parts across to github
Comment 11 Walter Chen[:ypwalter][:wachen] 2012-12-10 19:12:36 PST
Created attachment 690682 [details] [diff] [review]
Patch for review v2.1

This is a new patch for this fix. plz review
Comment 12 Jonathan Griffin (:jgriffin) 2012-12-11 12:45:04 PST
Comment on attachment 690682 [details] [diff] [review]
Patch for review v2.1

This patch looks like it's a diff between v1 and v2, and looks like it is missing any JS files.  Can you provide a single patch that includes the diff between your code and mozilla-central?

You don't need to do any github pull requests; we'll handle the mirroring after all changes have been committed to mozilla-central.

Thanks!
Comment 13 Walter Chen[:ypwalter][:wachen] 2012-12-12 18:19:29 PST
Created attachment 691639 [details] [diff] [review]
v3
Comment 14 Jonathan Griffin (:jgriffin) 2012-12-13 17:13:49 PST
Comment on attachment 691639 [details] [diff] [review]
v3

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

Looks great!  Ready to land after addressing a couple of minor issues.   Thanks for the work on this one Walter.

::: testing/marionette/client/marionette/marionette.py
@@ +364,2 @@
>          if isinstance(frame, HTMLElement):
>              response = self._send_message('switchToFrame', 'ok', element=frame.id)

We should add focus=focus to this call as well.

::: testing/marionette/marionette-actors.js
@@ +1104,5 @@
>      if (this.context == "chrome") {
>        let foundFrame = null;
>        if ((aRequest.value == null) && (aRequest.element == null)) {
>          this.curFrame = null;
> +        if (aRequest.focus == true) {

nit: or more simply, if (aRequest.focus) {
Comment 15 David Burns :automatedtester 2012-12-14 07:11:12 PST
Walter,

Did you land this? If so can you please put the commit URL in the bug please.
Comment 16 Jonathan Griffin (:jgriffin) 2012-12-14 10:04:27 PST
Walter, the normal procedure for landing patches in gecko is to push them https://hg.mozilla.org/integration/mozilla-inbound and use a commit message that includes the bug number.

See bug 821091 for an example.

If the tests for this commit show up OK at https://tbpl.mozilla.org/?tree=Mozilla-Inbound, a sheriff will land your patch for you at mozilla-central, and he will mark the bug as closed.

If you don't have sufficient privileges to commit to mozilla-inbound, let me know and I can do it for you.
Comment 17 Walter Chen[:ypwalter][:wachen] 2012-12-16 19:13:31 PST
Created attachment 692815 [details]
This is revised according to the reviewer's feedback

I can't do the commit to inbound.
Gina Yeh would be helping me to do so.

Thanks
Comment 19 Gina Yeh [:gyeh] [:ginayeh] 2012-12-17 01:46:11 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/d42d3987f8a4
Comment 20 Andrew Overholt [:overholt] 2012-12-17 10:15:32 PST
Blocks automated smoke tests so this is a blocker.
Comment 21 Ryan VanderMeulen [:RyanVM] 2012-12-17 16:41:45 PST
https://hg.mozilla.org/mozilla-central/rev/d42d3987f8a4
Comment 23 Walter Chen[:ypwalter][:wachen] 2012-12-19 09:16:43 PST
Finally, it's landed. Learned a lot from this patch.
Thanks for everyone's help.

Note You need to log in before you can comment on or make changes to this bug.