David Flanagan [:djf] Assignee
	Description • 12 years ago

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.

	David Flanagan [:djf] Assignee
	Comment 1 • 12 years ago

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.

	Mounir Lamouri (:mounir)
	Comment 2 • 12 years ago

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.

	David Flanagan [:djf] Assignee
	Comment 3 • 12 years ago

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?

	Mounir Lamouri (:mounir)
	Comment 4 • 12 years ago

(In reply to David Flanagan [:djf] from comment #3)
> Seem reasonable?

Yes, completely.

	David Flanagan [:djf] Assignee
	Comment 5 • 12 years ago

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.

	[:fabrice] Fabrice Desré
	Comment 6 • 12 years ago

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.

	David Flanagan [:djf] Assignee
	Comment 7 • 12 years ago

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.)

	Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)
	Comment 8 • 12 years ago

Comment on attachment 668340 [details] [diff] [review]
addressed nits from fabrice's review

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

This patch sounds nice.

	Ryan VanderMeulen [:RyanVM]
	Comment 9 • 12 years ago

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!

	Ed Morley [:emorley]
	Comment 10 • 12 years ago

https://hg.mozilla.org/mozilla-central/rev/f1968aacad96

	David Flanagan [:djf] Assignee
	Comment 11 • 12 years ago

Reopening and requesting approval to land in aurora.

	Ryan VanderMeulen [:RyanVM]
	Comment 12 • 12 years ago

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.

	Ryan VanderMeulen [:RyanVM]
	Comment 13 • 12 years ago

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...

	David Flanagan [:djf] Assignee
	Comment 14 • 12 years ago

Adding needs-checking-aurora to the whiteboard.

This really needs to be marked blocking+, since it blocks two blocking bugs.

	David Flanagan [:djf] Assignee
	Comment 15 • 12 years ago

changing the whiteboard to spell "checkin" correctly.

	Gregor Wagner [:gwagner]
	Comment 16 • 12 years ago

https://hg.mozilla.org/releases/mozilla-aurora/rev/31ecdc024188

	John Hammink
	Comment 17 • 12 years ago

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.