1036075 - [L10n] Keyboard goes CAPS LOCK after typing 100 characters and selecting a suggestion

Status: RESOLVED FIXED

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

Description

[Johan Lorenzo [:jlorenzo]]

Build info
Device: Flame
Gaia      e935f4ff190b76c70d9b2af8856c542a6e4a7546
Gecko     https://hg.mozilla.org/releases/mozilla-aurora/rev/3f9d7a3a0b7b
BuildID   20140708000322
Version   32.0a2
ro.build.version.incremental=109
ro.build.date=Mon Jun 16 16:51:29 CST 2014
B1TC00011220


STR
Prerequisites: Having at least 2 keyboards from 2 different languages. (Here will be used the English (US) and French keyboards)

1. Open the Messages app
2. Create a new message
3. Type in the message field : "Firefox n'a jamais été aussi rapide. Apprenez-en davantage sur le navigateur gratuit qui s'engage pour"
4. Type "vou"
5. Select the suggestion "vous"
6. Type "votre vie privée"

Expected result
You should be able to type "votre vie privée" without typing the shift key.

Actual result
The shift key is pressed every time you type a character. You're able to deactivate the caps lock for the next character but it switches back just after.

Other notes
Also tested with a third keyboard (Spanish) and the text "Prueba el mejor Firefox. Descubre el navegador de escritorio gratuito que está comprometido contigo, con tu privacidad y con una Web abierta". Caps locks is enabled from "privacidad Y CON..."

Reproducibility rate: 100%

Comment 1

[Johan Lorenzo [:jlorenzo]]

Reproduced on 1.4, Buri:
Gaia      5c9e1e4131d3ac8915ed88b72bb66dc7d97be6a0
Gecko     https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/2d0c15450488
BuildID   20140707000200
Version   30.0
ro.build.version.incremental=eng.tclxa.20131223.163538
ro.build.date=Mon Dec 23 16:36:04 CST 2013

NOT reproduced on 1.3, Buri.
Gaia      23f55be856cef53c6604a6fe4aeb09061afbc897
Gecko     https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/874adddec55d
BuildID   20140708024005
Version   28.0
ro.build.version.incremental=eng.tclxa.20131223.163538
ro.build.date=Mon Dec 23 16:36:04 CST 2013

Comment 2

[Johan Lorenzo [:jlorenzo]]

Would this bug block a 1.4 partner certification?

Comment 3

[Johan Lorenzo [:jlorenzo]]

Regression window wanted between 1.3 and 1.4.

Comment 4

[Jason Smith [:jsmith]]

(In reply to Johan Lorenzo [:jlorenzo] from comment #2)
> Would this bug block a 1.4 partner certification?

I think you didn't ask the right person here. Wayne was the person to ask here.

Comment 5

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

I'll take a look.

Comment 6

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

Attachment: Patch V1

WIP

Comment 7

[nick]

Similar incident reproduced on B2G 2.1.1 mako without necessity of language support. 

Platform version: 33.0a1
BuildID: 20140706005844

Expected behavior: Caps lock only toggles on when double-tapping Shift key.
Actual behavior: Caps lock toggles on after autocompleting a word with a user-input capitalized first letter, and toggles to on after every character. Leaving that keyboard window and returning, e.g. to a draft of a text message, returns to expected behavior.

How to reproduce: 

In a window with a keyboard (I did it in Messenger), type out a string of more than 100 characters. 

Hit the Shift key to begin spelling out an arbitrary word with the first letter capitalized. Accept the autocomplete suggestion for that word. Put the word in the middle of a sentence. Repeat until suddenly everything after the word is in upper case.

Example:

I am completely Flummox(accept suggestion: Flummoxed) by this Bug(accept suggestion: Bugs). What is going On with autocorrect? It appears to Only take place after a certain number of Word(accept suggestion: Words) 

At this point, everything afterward is also upper case - in this case, HAVE.

I let the phone sleep. When I reopened the screen, the shift key was toggled back to off.

Duplicated again in this manner. In that window, caps lock toggles back to on after every character. Save as a draft, exit and come back, and caps lock behavior is as expected (begins toggled off, does not toggle on for no reason.)

I believe this is similar to bug 1034942.

Comment 10

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

Comment on attachment 8454336 [details] [review]
Patch V1

[Root Cause]
from Bug 1015541, we started to listen for selectionchange so that keyboard app would update the word suggestion when the user changes the cursor position.
However, in that patch, I did not realize that the textBeforeCursor we received in selectionchange event would be limited to only 100 chars, but we still keep the cursor as the selectionStart, which may exceed the boundary of 100 and caused the issue since it cached the wrong state of the input field. 

[Solution]
Keep the inputText as a "window" to the input field, and let cursor point to this "window" instead of as the real selectionStart.
--
Jan, could you please help review this?

Yuan, if possible, please help take a look at this since this is is related API behavior.
Thanks

Comment 11

[Yuan Xulei [:yxl] (Leave from May 18)]

Comment on attachment 8454336 [details] [review]
Patch V1

The patch is not quite straightfoward. I suggest not change |cursor|, but maintain |textBeforeCursor + textAfterCursor| instead of |inputText|.

Comment 12

[Wayne Chang [:wchang]]

taking on 1.4+ since this is a regression from 1.3 according to comment 1

Comment 13

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

Comment on attachment 8454336 [details] [review]
Patch V1

David,

Not sure if you're available to look at this, but seems Jan is not around to comment on this patch.
Please help if you could since this is a v1.4+ issue.

--
Yuan, thanks for your feedback, but I'd like to also see what David/Jan think about this approach, and then I'll refine this fix.

Comment 14

[David Flanagan [:djf]]

Mike Habischer noticed this bug today and found that when he sees the all caps issue he finds that if he accepts an autocorrection and then types a period, the "space period" -> "period space" transition also inserts the uncorrected form of his word.  I'm guessing that that is the same issue here where the cursor value is corrupted.  But we should verify that this fix also fixes Mike's bug.

Comment 15

[David Flanagan [:djf]]

Also, I can see that this code in updateCapitalization() is causing the all caps behavior when the cursor value is too large:

    else if (cursor >= 2 &&
             isUpperCase(inputText.substring(cursor - 2, cursor))) {
      keyboard.setUpperCase(true);
    }

If cursor is too large, then the substring call returns the empty string, and the isUpperCase() function returns true in that case.

Comment 16

[David Flanagan [:djf]]

I'm a little confused by this bug and patch. The STR do not include any instructions to tap in the input field to move the cursor. So the only selectionchange events we are receiving are in response to the user typing keys. The first if statement in handleEvent() is to return early for simple character insertion when pendingSelectionChange is non-zero.  Apparently when we accept a word suggestion, the replacement that happens then triggers selectionchange, and we do not return early, but instead run the code that Rudy's patch fixes.

Can we make the pendingSelectionChange flag work for autocorrect and word suggestions the way it does for single key insertions?  That's not enough to fix the bug, but it seems like it should maybe be part of the fix.

Comment 17

[David Flanagan [:djf]]

Comment on attachment 8454336 [details] [review]
Patch V1

It took me a while to understand this patch, since it is to code that has been added since I last looked at latin.js...  I think it is pretty clear that this is a regression caused by bug 1015541.

If I understand it correctly, the patch will fix the upper case problem, but there is another bug: if you backspace 100 times, we will end up thinking that the cursor is at position 0 and will not allow any further backspaces.

One way to fix this would be to call inputContext.getText() when we get a selectionchange event so that we have a complete copy of the text. Or perhaps we could just update the cursor position and not change the text.

But for that to work we also want to clearly distinguish the case where the cursor moves without any text changes from the case where the cursor moves and the text changes.  If we had pendingSelectionChange++ and pendingSelectionChange-- wrappers around the code that does autocorrect and suggestion insertion that might help distinguish those cases.

And maybe you could use code something like this to verify that no text has changed:

cursor = evt.target.selectionStart;
var beforeLen = evt.target.textBeforeCursor.length;
var afterLen = evt.target.textAfterCursor.length;
var newText = evt.target.textBeforeCursor + evt.target.textAfterCursor;
if (inputText.substring(cursor - beforeLen, beforeLen + afterLen) === newText) {
  // the text didn't change, only the position of the cursor
  // so just update the displayed suggestions.
}
else {
  // the text has changed, we need to call getText() to get all of it before
  // we update the displayed suggestions.
  // I don't know if this case will ever actually happen.
}

Comment 19

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

Attachment: Patch V2

David,

Thanks for the advice, patch updated to include the following changes,
 1. also not to handle selectionchange when you select a suggestion (with pendingSelectionChange flag).
 2. Do not update inputText but only the cursor value.

It is obvious that I tend to modify this part at the least level, since this change might go to v1.4 as well, so I want to minimize the risk.

Please help review it again.

Comment 20

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

Comment on attachment 8456681 [details] [review]
Patch V2

Also ask for Jan's help to review this.
David, do you mind I transferred the review to Jan if you don't have time to review this change?

Thanks.

Comment 21

[David Flanagan [:djf]]

Comment on attachment 8456681 [details] [review]
Patch V2

Rudy,

I don't mind that you've transferred the review to Jan. I've been at a media team workweek and we are all pretty distracted by media blockers.  I'm giving feedback+ on this and clearing the review request. I would like Jan to have a look since he knows this code better than I do, I think.  But if he is not available, I will do a review next week if you need it.

I'm giving feedback+ because I like this version of the patch better than the last one. You've set the pendingSelectionChange flag when the user taps a suggested word, but have not set it for autocorrect.  It seems to me that you should do both or neither.  One way would be to set the flag inside replaceBeforeCursor() so that it handles both cases and also handles the case where we revert an auto correct.

On the other hand, now that you have removed the code that changes inputText, it no longer seems to matter as much. You could probably remove the pendingSelectionChange code for word suggestions and it would still work.

One thing I'm wondering about: if pendingSelectionChange is > 0 then we return early and updateSuggestions() never gets called.  Does that break anything?  After an autocorrect or word suggestion obviously we want to clear the list of suggestions.  Does that still happen with this patch?

Comment 22

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

(In reply to David Flanagan [:djf] from comment #21)
> Comment on attachment 8456681 [details] [review]
> Patch V2
> 
> Rudy,
> 
> I don't mind that you've transferred the review to Jan. I've been at a media
> team workweek and we are all pretty distracted by media blockers.  I'm
> giving feedback+ on this and clearing the review request. I would like Jan
> to have a look since he knows this code better than I do, I think.  But if
> he is not available, I will do a review next week if you need it.
> 
> I'm giving feedback+ because I like this version of the patch better than
> the last one. You've set the pendingSelectionChange flag when the user taps
> a suggested word, but have not set it for autocorrect.  It seems to me that
> you should do both or neither.  One way would be to set the flag inside

> replaceBeforeCursor() so that it handles both cases and also handles the
> case where we revert an auto correct.

I think these 2 cases (auto-correction and revert) should have been addressed by setting pendingSelectionChange flag in click().

> 
> On the other hand, now that you have removed the code that changes
> inputText, it no longer seems to matter as much. You could probably remove
> the pendingSelectionChange code for word suggestions and it would still work.
> 
> One thing I'm wondering about: if pendingSelectionChange is > 0 then we
> return early and updateSuggestions() never gets called.  Does that break
> anything?  After an autocorrect or word suggestion obviously we want to
> clear the list of suggestions.  Does that still happen with this patch?
Yes, that is the original behavior of our code, and what we did is not to do additional 'updateSuggestions()' for selectionchange.
e.g. we would still clear the suggestions in select() function.

Comment 23

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

Comment on attachment 8456681 [details] [review]
Patch V2

It seems Jan is also too busy to take a look at my patch.
David, could you help?

Thank you.

Comment 24

[David Flanagan [:djf]]

Comment on attachment 8456681 [details] [review]
Patch V2

Rudy,

Thanks for the clarifications. I understand what is happening now. r+

Comment 25

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

Landed to Gaia master,
https://github.com/mozilla-b2g/gaia/commit/044072710058b94c034cf3ecf5ba3032333f91db


--
David, thanks for the review.

Comment 26

[Anton Borodyuk]

Will this land on 1.4?

Comment 27

[Ryan VanderMeulen [:RyanVM]]

v2.0: https://github.com/mozilla-b2g/gaia/commit/372b58dff18d1493ea7eb05ade75085545625f3e
v1.4: https://github.com/mozilla-b2g/gaia/commit/5db3d93d68c30c1cf24499b3cda3fd881d207ea6