Closed Bug 816514 Opened 12 years ago Closed 11 years ago

[B2G] When using marionette in the Keyboard frame the frame loses focus

Categories

(Remote Protocol :: Marionette, defect)

ARM
Gonk (Firefox OS)
defect
Not set
major

Tracking

(blocking-basecamp:+, firefox19 fixed, firefox20 fixed, b2g18 fixed)

RESOLVED FIXED
mozilla20
blocking-basecamp +
Tracking Status
firefox19 --- fixed
firefox20 --- fixed
b2g18 --- fixed

People

(Reporter: zcampbell, Assigned: wachen)

References

Details

Attachments

(3 files, 2 obsolete files)

Attached file 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.
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.
Blocks: 801898
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.
Assignee: nobody → wachen
Would ask for marionette server/client review tomorrow for this.
Attached patch Patch for review v1 (obsolete) — Splinter Review
I attached a patch for reviewing (v1)
Please help to review
(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
Attachment #689530 - Flags: review?(jgriffin)
Attachment #689530 - Attachment is patch: true
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.
Attachment #689530 - Flags: review?(jgriffin) → review-
Ok, I finished the modification.
However, I don't know where should I branch from to ask for marionette client review.
Any ideas?
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.
Flags: needinfo?(dburns)
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
Flags: needinfo?(dburns)
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
Attached patch Patch for review v2.1 (obsolete) — Splinter Review
This is a new patch for this fix. plz review
Attachment #690682 - Flags: review?(jgriffin)
OS: Windows 7 → Gonk (Firefox OS)
Hardware: x86_64 → ARM
Attachment #690682 - Attachment is patch: true
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!
Attachment #690682 - Flags: review?(jgriffin) → review-
Attached patch v3Splinter Review
Attachment #689530 - Attachment is obsolete: true
Attachment #690682 - Attachment is obsolete: true
Attachment #691639 - Flags: review?(jgriffin)
Attachment #691639 - Attachment is patch: true
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) {
Attachment #691639 - Flags: review?(jgriffin) → review+
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Walter,

Did you land this? If so can you please put the commit URL in the bug please.
Flags: needinfo?(wachen)
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.
I can't do the commit to inbound.
Gina Yeh would be helping me to do so.

Thanks
Flags: needinfo?(wachen)
QA Contact: wachen
blocking-basecamp: --- → ?
Blocks automated smoke tests so this is a blocker.
blocking-basecamp: ? → +
Whiteboard: [automation-needed-in-aurora][automation-needed-in-b2g18]
https://hg.mozilla.org/mozilla-central/rev/d42d3987f8a4
Status: REOPENED → RESOLVED
Closed: 11 years ago11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
Finally, it's landed. Learned a lot from this patch.
Thanks for everyone's help.
Product: Testing → Remote Protocol
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: