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)
Tracking
(blocking-basecamp:+, firefox19 fixed, firefox20 fixed, b2g18 fixed)
People
(Reporter: zcampbell, Assigned: wachen)
References
Details
Attachments
(3 files, 2 obsolete files)
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•12 years ago
|
||
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•12 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•12 years ago
|
||
Would ask for marionette server/client review tomorrow for this.
Assignee | ||
Comment 4•12 years ago
|
||
I attached a patch for reviewing (v1) Please help to review
Comment 5•12 years ago
|
||
(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
Updated•12 years ago
|
Attachment #689530 -
Flags: review?(jgriffin)
Updated•12 years ago
|
Attachment #689530 -
Attachment is patch: true
Comment 6•12 years ago
|
||
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•12 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•12 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)
Comment 9•12 years ago
|
||
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)
Comment 10•12 years ago
|
||
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•12 years ago
|
||
This is a new patch for this fix. plz review
Attachment #690682 -
Flags: review?(jgriffin)
Assignee | ||
Updated•12 years ago
|
OS: Windows 7 → Gonk (Firefox OS)
Hardware: x86_64 → ARM
Updated•12 years ago
|
Attachment #690682 -
Attachment is patch: true
Comment 12•12 years ago
|
||
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•11 years ago
|
||
Attachment #689530 -
Attachment is obsolete: true
Attachment #690682 -
Attachment is obsolete: true
Attachment #691639 -
Flags: review?(jgriffin)
Updated•11 years ago
|
Attachment #691639 -
Attachment is patch: true
Comment 14•11 years ago
|
||
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•11 years ago
|
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Updated•11 years ago
|
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 15•11 years ago
|
||
Walter, Did you land this? If so can you please put the commit URL in the bug please.
Flags: needinfo?(wachen)
Comment 16•11 years ago
|
||
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•11 years ago
|
||
I can't do the commit to inbound. Gina Yeh would be helping me to do so. Thanks
Flags: needinfo?(wachen)
Assignee | ||
Updated•11 years ago
|
QA Contact: wachen
Assignee | ||
Updated•11 years ago
|
blocking-basecamp: --- → ?
Comment 18•11 years ago
|
||
try: https://tbpl.mozilla.org/?tree=Try&rev=0e8ff7903fd9 https://tbpl.mozilla.org/?tree=Try&rev=06257b6ca239
Comment 20•11 years ago
|
||
Blocks automated smoke tests so this is a blocker.
blocking-basecamp: ? → +
Updated•11 years ago
|
Whiteboard: [automation-needed-in-aurora][automation-needed-in-b2g18]
Comment 21•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/d42d3987f8a4
Status: REOPENED → RESOLVED
Closed: 11 years ago → 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
Comment 22•11 years ago
|
||
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•11 years ago
|
||
Finally, it's landed. Learned a lot from this patch. Thanks for everyone's help.
Updated•1 year ago
|
Product: Testing → Remote Protocol
You need to log in
before you can comment on or make changes to this bug.
Description
•