Closed Bug 796080 Opened 8 years ago Closed 8 years ago

MozKeyboard must send notification when user moves cursor

Categories

(Firefox OS Graveyard :: Gaia::Keyboard, defect)

defect
Not set
normal

Tracking

(blocking-basecamp:+, b2g18 fixed)

VERIFIED FIXED
blocking-basecamp +
Tracking Status
b2g18 --- fixed

People

(Reporter: djf, Assigned: djf)

References

Details

Attachments

(1 file, 2 obsolete files)

b2g/components/MozKeyboard.js currently fires an event when the user focuses an input field they takes keyboard input. This event causes the Gaia keyboard app to open up. This event is enough for a simple virtual keyboard. But now that we're doing predictions, there is a serious bug: the virtual keyboard needs to know when the user moves the cursor so that it can throw out its current predictions. 

To fix this, I plan to modify b2g/chrome/content/forms.js to send an event when the cursor moves as well as when an input element is focused.

I'm nominating this as blocking basecamp because the alternative to fixing this is to remove predictive text input.
blocking-basecamp: --- → ?
This patch modifies forms.js so that it monitors mousedown and mouseup events on the currently focused element. If it detects a change to the cursor position of the element it sends a Form:Input message just as it would when the element was first focused.  This will allow the prediction engine in the keyboard app to hide its current set of predictions since they are no longer valid at the new cursor location. 

In addition, it adds the cursor/selection position to the message it sends, which will allow the prediction engine to figure out its new context and offer predictions there, if it makes sense to do so.

The patch also changes the property name 'previousTarget' to 'focusedElement', which is more descriptive and less confusing.

Finally, the patch adds a call to removeMessageListener where it had been missing.
Attachment #666867 - Flags: review?(fabrice)
Blocks: 797155
Comment on attachment 666867 [details] [diff] [review]
patch to b2g/chrome/content/forms.js

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

::: b2g/chrome/content/forms.js
@@ +30,5 @@
> +    // multiple copies of the event listeners registered, so try not to 
> +    // run this method more than once.  
> +    // See https://bugzilla.mozilla.org/show_bug.cgi?id=796698
> +    // Even with this workaround, however, we're getting multiple mouseup
> +    // events and calling tryShowIme() more than once.

Please, fix that in bug 796698.
OS: Mac OS X → All
Hardware: x86 → All
Mounir: Fabrice has already mentioned that he doesn't like that part of the patch.  I'll remove it. But I assume that his (or your?) review comments will require other changes, so I'm waiting for the rest of the review before I change anything.  Seem reasonable?
(In reply to David Flanagan [:djf] from comment #3)
> Seem reasonable?

Yes, completely.
Blocks: 796544
I've removed the hacky workaround code for the forms.js loading too many times bug, and have added a new new lines to pass the value of the inputmode property to mozkeyboard along with the cursor/selection information.
Attachment #666867 - Attachment is obsolete: true
Attachment #666867 - Flags: review?(fabrice)
Attachment #668249 - Flags: review?(fabrice)
Comment on attachment 668249 [details] [diff] [review]
removed the hack, added inputmode

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

That looks ok for me apart from nits, but I'd like to have vingtetun check this patch also.

::: b2g/chrome/content/forms.js
@@ -4,4 @@
>   * License, v. 2.0. If a copy of the MPL was not distributed with this file,
>   * You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> -dump("###################################### forms.js loaded\n");

Please don''t remove this.

@@ +84,5 @@
>                     evt.target.parentNode instanceof HTMLSelectElement) {
>            content.setTimeout(function showIMEForSelect() {
>              sendAsyncMessage("Forms:Input", getJSON(evt.target.parentNode));
>            });
> +          this.setFocusedElement(evt.target.parentNode);

We use evt.target.parentNode 3 times here. That could be saved to a local var.

@@ +108,5 @@
> +        break;
> +
> +      case 'mouseup':
> +        // We only listen for this event on the currently focused element.
> +        // When the mouse goes up, see if the cursor has moved (or the 

nit: trailing whitespace

@@ +260,5 @@
> +  // to lowercase
> +  let inputmode = element.inputmode || element.getAttribute('inputmode');
> +  if (inputmode) 
> +    inputmode = inputmode.toLowerCase();
> +

Nit: Use braces even for single line ifs.
Attachment #668249 - Flags: review?(fabrice)
Attachment #668249 - Flags: review?(21)
Attachment #668249 - Flags: feedback+
Vivien,

Fabrice reviewed this but wanted your review as well.  I've addressed Fabrice's nits with this most recent patch.

Predictive text (and inputmode support) can't work well until this patch lands, so I'd appreciate a quick review.  (Though I know you probably have a million things to review.)
Attachment #668249 - Attachment is obsolete: true
Attachment #668249 - Flags: review?(21)
Attachment #668340 - Flags: review?(21)
Comment on attachment 668340 [details] [diff] [review]
addressed nits from fabrice's review

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

This patch sounds nice.
Attachment #668340 - Flags: review?(21) → review+
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/f1968aacad96

Please make sure that your future patches have commit messages that describe what the patch as a whole is doing, rather than what changed in the latest revision. Thanks!
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/f1968aacad96
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Reopening and requesting approval to land in aurora.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Whiteboard: approval-mozilla-aurora?
The bug resolution is for tracking when a patch lands on mozilla-central. Use the branch-specific tracking flags to indicate when it lands there. Also, you need to request aurora approval on the patch itself, not via the whiteboard.
Status: REOPENED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
Whiteboard: approval-mozilla-aurora?
Comment on attachment 668340 [details] [diff] [review]
addressed nits from fabrice's review

...which I now see that b2g bugs apparently don't have. Should probably file a bug on that...
Whiteboard: approval-mozilla-aurora?
blocking-basecamp: ? → +
Adding needs-checking-aurora to the whiteboard.

This really needs to be marked blocking+, since it blocks two blocking bugs.
blocking-basecamp: + → ?
Whiteboard: approval-mozilla-aurora? → approval-mozilla-aurora? needs-checking-aurora
blocking-basecamp: ? → +
changing the whiteboard to spell "checkin" correctly.
Whiteboard: approval-mozilla-aurora? needs-checking-aurora → approval-mozilla-aurora? needs-checkin-aurora
https://hg.mozilla.org/releases/mozilla-aurora/rev/31ecdc024188
Whiteboard: approval-mozilla-aurora? needs-checkin-aurora
Tested this fix on the following build

Otoro daily: 10-10-2012
* gaia: 2667536e3b06e46dd193aa6d76ba08dad2d867be
* gonk: 6ec34aa3d66331054de37eabc594ad923654b27c

Steps to verify this fix:
1.  launch browser
2. in url bar enter three urls, and fully load each site:
 - www.cnn.com
 - www.cnet.org
 - www.google.com

3. start to type www.cracked.com into urlbar

Expected & actual result
all previously loaded urls were shown in drop down list until I got as far as "www.c" at which point only cnn and cnet were shown.   After I got as far as "www.cr" then no URLs showed up in dropdown, except default bing listing.

4.  Repeated everything with typing www.gnu.org
 - until I was at "www." I could see all urls
 - when I got to "www.g" then I could only see google in dd list
 - when I got to "www.gn" then no urls, other than default bing were shown.
Status: RESOLVED → VERIFIED
Component: Gaia → Gaia::Keyboard
You need to log in before you can comment on or make changes to this bug.