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

RESOLVED FIXED in Firefox 19

Status

Testing
Marionette
--
major
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: zac, Assigned: ypwalter)

Tracking

unspecified
mozilla20
ARM
Gonk (Firefox OS)
Points:
---

Firefox Tracking Flags

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

Details

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

5 years ago
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.
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
(Assignee)

Comment 2

5 years ago
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
(Assignee)

Comment 3

5 years ago
Would ask for marionette server/client review tomorrow for this.
(Assignee)

Comment 4

5 years ago
Created attachment 689530 [details] [diff] [review]
Patch for review v1

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-
(Assignee)

Comment 7

5 years ago
Ok, I finished the modification.
However, I don't know where should I branch from to ask for marionette client review.
Any ideas?
(Assignee)

Comment 8

5 years ago
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
(Assignee)

Comment 11

5 years ago
Created attachment 690682 [details] [diff] [review]
Patch for review v2.1

This is a new patch for this fix. plz review
Attachment #690682 - Flags: review?(jgriffin)
(Assignee)

Updated

5 years ago
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-
(Assignee)

Comment 13

5 years ago
Created attachment 691639 [details] [diff] [review]
v3
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+
(Assignee)

Updated

5 years ago
Status: NEW → RESOLVED
Last Resolved: 5 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.
(Assignee)

Comment 17

5 years ago
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
Flags: needinfo?(wachen)
(Assignee)

Updated

5 years ago
QA Contact: wachen
(Assignee)

Updated

5 years ago
blocking-basecamp: --- → ?
try:
https://tbpl.mozilla.org/?tree=Try&rev=0e8ff7903fd9
https://tbpl.mozilla.org/?tree=Try&rev=06257b6ca239
https://hg.mozilla.org/integration/mozilla-inbound/rev/d42d3987f8a4
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
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
https://hg.mozilla.org/releases/mozilla-aurora/rev/3250d4c5dd8f
https://hg.mozilla.org/releases/mozilla-b2g18/rev/5f39d089ad38
status-b2g18: --- → fixed
status-firefox19: --- → fixed
status-firefox20: --- → fixed
Whiteboard: [automation-needed-in-aurora][automation-needed-in-b2g18]
(Assignee)

Comment 23

5 years ago
Finally, it's landed. Learned a lot from this patch.
Thanks for everyone's help.
You need to log in before you can comment on or make changes to this bug.