927882 - Standardize TextSelection caret positioning logic for soft keyboards

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Keyboards and IME, defect)

Description

[Peter Retzer (:pretzer)]

SwiftKey has a feature which makes the text cursor in form fields jump to the end of the word when you tap on it somewhere in the middle. As a side effect our orange thumb handle disappears as soon as the cursor jumps.
This on it's own is already a problem, but the real issue starts when you try to move the cursor through text with the help of the thumb handle. The jump-to-end feature constantly kicks in which causes the handle to disappear and subsequently makes panning through form text impossible.

This seems to be a regression. I can reproduce in Nightly, but the jump-to-end feature doesn't get activated in Release channel for me.

Device: Samsung Galaxy S2, Android 4.1.2, SwiftKey 4.2.1.202

Comment 1

[Aaron Train [:aaronmt]]

What's the behaviour in Beta/Aurora?

Comment 2

[Peter Retzer (:pretzer)]

Sorry, I don't have these channels installed on my device... not enough storage. Can you test?

Comment 3

[Teodora Vermesan (:TeoVermesan)]

Firefox for Android 25 Beta 8 : not reproducible
Firefox for Android 26.0a2 (2013-10-20) : reproducible

Comment 4

[Lucas Rocha (:lucasr)]

Capella, could you have look at this one?

Comment 5

[Mark Capella [:capella]]

Ok, but new to SwiftKey keyboard, downloaded the free Beta v4.3.0.178 to my N7 and can't find a setting controlling behaviour as described in comment 0.

Then I tried SwiftKey Trial v4.2.1.202 on my GS3, and I still can't find that setting.

pretzer: Is this a paid feature? What am I missing?

Comment 6

[Mark Capella [:capella]]

Ok ... reproducable now ... cc:ing margaret since my initial fix is looks to be going into making SelectionHandler.js ... making it smarter about communication with the IME ...

Comment 7

[Mark Capella [:capella]]

Attachment: bug927882

We've enhanced the SelectionHandler a lot recently, but I can't think what may have regressed this. However I can see what's wrong with it, and the fix seems fairly simple, and works for me.

Comment 8

[Peter Retzer (:pretzer)]

(In reply to Mark Capella [:capella] from comment #5)
> pretzer: Is this a paid feature? What am I missing?

Sorry for the late response! There is no setting to control this behavior, at least none that I'm aware of... but it seems you figured that out already anyways. Thanks for the fix! :-)

Comment 9

[:Margaret Leibovic]

Comment on attachment 826613 [details] [diff] [review]
bug927882

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

This seems sensible to me, but I want to get input from bnicholson and/or jchen, since they were involved in bug 828990, which was similar.

Comment 10

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 826613 [details] [diff] [review]
bug927882

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

LGTM

Comment 11

[Brian Nicholson (:bnicholson)]

Comment on attachment 826613 [details] [diff] [review]
bug927882

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

LGTM too.

Comment 12

[Brian Nicholson (:bnicholson)]

Comment on attachment 826613 [details] [diff] [review]
bug927882

With this patch applied, trying to position the thumb within a word always makes the thumb jump to the end of the word for me. I'm using Google Keyboard on HTC One; other keyboards don't seem to have this problem.

Mark, do you see the same thing with Google Keyboard? If I remove the compositionstart/compositionupdate listeners from your patch, things work more or less as expected (with some minor jumping glitchiness). Are the compositionstart/compositionupdate listeners here necessary? If so, I think we'll need to tweak this a bit more.

Comment 13

[Mark Capella [:capella]]

The way I understand things is based on digging through the IME widget/android/nsWindow.cpp, and watching / verifying communications across the bridge during testing at a very detailed level ...comparing individual test results might be complicated but doable.

I've not played with results from Googles keyboard ... let me try that and see if my observations hold.

Comment 14

[Mark Capella [:capella]]

oh ouch

Comment 17

[:Margaret Leibovic]

Comment on attachment 826613 [details] [diff] [review]
bug927882

Clearing review for now, since it seems like there are still some more issues to work out here :(

Comment 18

[Aaron Train [:aaronmt]]

This still being worked on for 26?

Comment 19

[Mark Capella [:capella]]

It's my followup now that bug 934470 looks like it's clearing.

Comment 20

[Mark Capella [:capella]]

Ah, the thing is with this one, the issue with SwiftKey exists in v26/Beta already (v25/release is unaffected). Tap into a field, SwiftKey does something (auto-correct) causing the cursor to skip to end of field ... we erroneously leave the caret pointing to the wrong place.

Something we fixed in v27 (not sure what yet) makes the situation better, though not as optimal as the requester here might like. When SwiftKey jumps to end of field, we turn off the caret rather than leave it in the wrong place.

Comment 21

[Peter Retzer (:pretzer)]

So I tested again in Nightly(28) just now. This is what I saw:

Multi-line textarea (tested on Bugzilla's comment box):
- SwiftKey: The caret doesn't come up at all, no panning possible
- Stock Keyboard: The caret doesn't come up at all, no panning possible

Single-line input (tested on Google.com's search box):
- SwiftKey: The caret does not come up on first tap, because it gets hidden when the jump-to-end feature kicks in. It does come up on second tap though, panning works fine then 
- Stock Keyboard: The caret comes up on first tap, panning works fine

That makes at least two different issues in Nightly at the moment AFAICS: 
1. No caret at all in multiline textareas (regardless of IME used)
2. Jump-to-end feature hiding caret for SwiftKey in single-line inputs (though probably also in multi-line textareas if issue 1 wouldn't exist)


Aurora(27)/Beta(26): I did not test in these channels though it seems from your comment that at least Beta(26) shows different (worse?) behavior than described above.

The question now is, which of these issues do you plan to fix here and do other bugs need to be filed? I guess the 26-ship has sailed now anyways, right?

Comment 22

[Mark Capella [:capella]]

fyi bug 946030 should correct being unable to receive a caret on tapping into a textarea.

Comment 23

[Mark Capella [:capella]]

This will leave this bug focusing on jump-to-end on auto-complete / auto-correct / auto-suggest, etc. Currently all keyboards in this case are simply turning off the caret, rather than re-positioning it.

I have a WIP that leaves the caret visible and re-positions it correctly, but seems to trigger unacceptable side-effect behavior as a result during any subsequent user / manual dragging / repositioning of the caret that I'm looking into.

This may be why we simply turned off the caret in the first place in this situation.

Comment 24

[Peter Retzer (:pretzer)]

Okay, thanks for the update! I hope you can find a solution. 
With bug 934470 already having fixed the panning problem described in comment 0 and bug 946030 being the more pressing remaining issue I think this bug no longer need to be tracked...

Comment 25

[Mark Capella [:capella]]

Attachment: BLUR

A chat with jchen kicked loose this idea for me ... We may be reacting wrong to compositionend messages ... those I believe refer to internal and sometimes fleeting changes such as might be generated by an IME after autoCorrecting, or even autoStyle Changing (underline-a-block(?)) text in a GeckoEditable.

I think this heads in the right direction... the next trick is that everytime you do type something, we leve the caret sitting right where it was.

We still need to choose a trigger to listen to for caretChange events. I think I saw an API in a11y... or something else should be available, I haven't dug further than this status update.

Comment 26

[:Margaret Leibovic]

Comment on attachment 8349878 [details] [diff] [review]
BLUR

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

I haven't tested this myself, but this sounds like a reasonable change to me.

::: mobile/android/chrome/content/SelectionHandler.js
@@ +181,5 @@
>      this.isSelectionActive();
>  
>      switch (aEvent.type) {
>        case "pagehide":
>        // We only add keydown and blur listeners for TYPE_CURSOR

You should update this comment, since it's incorrect now :)

Comment 27

[:Margaret Leibovic]

Any reason you haven't landed this? I can help take over to wrap this up, but I just want to know if there are outstanding issues that need to be addressed.

Comment 28

[Mark Capella [:capella]]

Comment on attachment 8349878 [details] [diff] [review]
BLUR

I'm sorry! I thought I'd flagged this for f? vs r?. I'd pointed this out as informational, describing the problem behind the original behavior only, to be sure we agreed on the nature of the change going forward.

I ran out of time before making further progress.

Comment 29

[Mark Capella [:capella]]

If we no longer this._deactivate(); on composition:foo events, we should at least update the caret position in that event.

Comment 30

[Mark Capella [:capella]]

Attachment: bug927882.diff

I think logically this may be all there is to finish this patch. This version corrects a minor problem wesj would have seen in bug 950085 comment 8 ...

I've made a critical change in GeckoEditable to help us in SelectionHandler, and am asking jchen for first review ... there is probably a better way to do what I've done there, but I don't understand the current state, so my gap analysis/correction may be shortsighted.

Pending that fix, I'll need to followup with review requests to margaret and probably wesj also.

Comment 31

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8371723 [details] [diff] [review]
bug927882.diff

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

::: mobile/android/base/GeckoEditable.java
@@ +434,5 @@
>              }
>              return;
>          }
>  
> +        Integer caretPos = null;

Use int type and an invalid value like -1

@@ +530,5 @@
>                  composingStart, composingEnd));
> +
> +        // Notify SelectionHandler of final caret position
> +        if (caretPos != null) {
> +            GeckoAppShell.sendEventToGecko(GeckoEvent.createBroadcastEvent("TextSelection:UpdateCaretPos", null));

Can you explain why a separate event is needed instead of the original composition events?

Comment 32

[Mark Capella [:capella]]

The additional changes we'll be making to SelectionHandler to fix Swift keyboard (avoiding closing the selection on compositionend message) leaves the caret on display in situations where it wasn't previously.

The Google keyboard regresses as a result:
   https://www.dropbox.com/s/m7k7u5b1ta8blln/Screenshot_2014-02-06-12-43-39.png

GeckoEditable.icUpdateGecko() performs createIMERangeEvent(IME_RANGE_CARETPOSITION) early in the routine which places the caret in it's final position. This isn't observed in SelectionHandler.

Later in the same routine, we generate a GeckoEvent.createIMECompositionEvent which *is* observed in SelectionHandler as a compositionend message, and we use to update the cursor position. This is correct in most cases, but not in this one, where the two positions differ.

Comment 33

[Mark Capella [:capella]]

Attachment: bug927882.diff (v2)

For jchen: I've corrected the nit from the first patch (int caretPos == -1) as requested.

Also, I noticed a further Google keyboard issue with caret positioning during IME dismissal that required an additional patch to nsWindow.cpp ... basically requiring us to generate the same "TextSelection:UpdateCaretPos" message like we did in GeckoEditable.java. Let me know if you're not the correct person to bless that part of the change?

The rest of the changes go into SelectionHandler.js, and I'm asking margaret and wesj to both look at that. I think the comments I've added should make the changes easily digestible.

fyi, I've been testing throughout using Samsung, Google, Jellybean, and Swift keyboards, and had to absorb a deal of IME code, to where I'm finally comfortable with this patch, and it's ability to handle autoCorrect, and etc.

Happy reviewing :-D

Comment 34

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8372695 [details] [diff] [review]
bug927882.diff (v2)

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

Thanks! I have some more questions,

::: mobile/android/base/GeckoEditable.java
@@ +531,5 @@
> +
> +        // Notify SelectionHandler of final caret position, required for Google keyboard after autoCorrections, etc
> +        if (caretPos != -1) {
> +            GeckoAppShell.sendEventToGecko(GeckoEvent.createBroadcastEvent("TextSelection:UpdateCaretPos", null));
> +        }

Now that you are also sending the event in nsWindow.cpp, this here may not be necessary? I'd rather we send events in C++ than Java to avoid the extra overhead.

::: mobile/android/chrome/content/SelectionHandler.js
@@ +76,5 @@
> +      case "TextSelection:UpdateCaretPos":
> +        // GeckoEditable/IME special use case for autoCorrection / styling
> +        this._positionHandles();
> +        break;
> +

.

@@ +192,1 @@
>          }

So the only difference between the "TextSelection:UpdateCaretPos" handler and the "compositionend" handler is the check for _ignoreCompositionChanges.

That implies when SwiftKey does jump-to-end, _ignoreCompositionChanges is true and the compositionend handler is not doing anything. Otherwise, the compositionend handler would have repositioned the handles and TextSelection:UpdateCaretPos wouldn't be necessary, right? Still trying to understand all the bits :)

::: widget/android/nsWindow.cpp
@@ +1803,5 @@
>      WidgetCompositionEvent event(true, NS_COMPOSITION_END, this);
>      InitEvent(event, nullptr);
>      DispatchEvent(&event);
> +
> +    // Notify SelectionHandler of final caret position, required for Google keyboard after IME hide via 'Back' button

I think just "Notify SelectionHandler of final caret position" is fine. No need to mention the specifics of the bug, because the specifics can change in the future depending on Android version, stock keyboard version, etc.

@@ +1804,5 @@
>      InitEvent(event, nullptr);
>      DispatchEvent(&event);
> +
> +    // Notify SelectionHandler of final caret position, required for Google keyboard after IME hide via 'Back' button
> +    nsAppShell::gAppShell->PostEvent(AndroidGeckoEvent::MakeBroadcastEvent(NS_LITERAL_CSTRING("TextSelection:UpdateCaretPos"), NS_LITERAL_CSTRING("")));

Please limit one line to 80 characters (for C++; 100 for Java)

Comment 35

[Mark Capella [:capella]]

|_ignoreCompositionChanges| is used to ignore compositionFoo message that we generate in SelectionHandler during User caret positioning / movement around and through the editable ... we know in this situation that we'll wind up initiating these messages back to ourselves, so have a simple means to ignore them. (This isn't the same case timing-wise as the composition messages we receive during autoCorrection / jump-to-end, etc.)

The two new sources for "TextSelection:UpdateCaretPos" messages each will correct a different caret positioning error introduced as a result of the new logic associated with this bug / change. I guess my code comments for them don't make that distinction clear. I can dropbox a screenshot if you'd like.

I haven't worked on the C++ side to any great degree recently (bits here and there) and forgot Java / C++ styling line limits differ, so I'll fix that up.

Comment 36

[Wesley Johnston (:wesj)]

Comment on attachment 8372695 [details] [diff] [review]
bug927882.diff (v2)

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

Mostly just concerns/questions that I wanted to clarify. If they're unfounded, tell me :) Thanks for tackling this stuff Mark :)

::: mobile/android/chrome/content/SelectionHandler.js
@@ +505,4 @@
>      this._initTargetInfo(aElement);
>  
> +    // Caret-specific observer/listeners
> +    this._contentWindow.addEventListener("keyup", this, false);

Does "keyup" not propagate to the deck? I'd like to be consistent, plus, avoiding touching content is a good thing...

@@ +526,5 @@
>      this._isRTL = (this._contentWindow.getComputedStyle(aElement, "").direction == "rtl");
>  
>      this._addObservers();
>      this._contentWindow.addEventListener("pagehide", this, false);
> +    this._contentWindow.addEventListener("blur", this, true);

Same thing about the deck event listener here, for both of these listeners...

@@ +765,4 @@
>      this._contentWindow.removeEventListener("blur", this, true);
> +
> +    // Only observed for caret positioning
> +    if (this._activeType == this.TYPE_CURSOR) {

I don't really trust that deactivate will always be called (if we for instance move from CURSUR to SELECTION mode). Maybe this stuff should go in a (non-existent right now) _activeType setter?

Comment 37

[Mark Capella [:capella]]

All questions are founded :-D

The listener code you point to is legacy, but now located in a more logically consistent fashion with event flow. I could probably move those two out of _initTargetInfo(), into the shared (by handles and caret) global method this._addObservers(); and then change them over to the deck as you point out, and let you know in case of obvious failures.

Also, now might be a good time to point out that one of the things that had been complicating this patch for me, was not realizing that Google and Swift keyboards behave different in terms of how they signal to / interact with us in SelectionHandler.

Please be aware of bug 968231 comment 1.

tldr: Major difference in that Google kbd generates keyUp/Down/Press events and Swift kbd does not.  :-O

Comment 38

[Mark Capella [:capella]]

Attachment: bug927882.diff (v3)

Ok, based on suggestions from jchen re: the IME side of things, I've managed to move new code responsible for caret positioning that I had previously located in GeckoEditable.java, into nsWindow.cpp in a more appropriate place.

I also had no problems with the consistency changes wesj pointed out.

Further, I see his concern re: |don't really trust that deactivate will always be called|, as currently we don't immediately call _closeSelection() at start of attachCaret() similar to how we do it in startSelection(). I've not yet had an opportune patch to piggyback that change onto. I can offer to open a follow-up bug to handle that to the side (?).

Flagging all for another review flood :-)   (You're probably seeing these messages in either case!)

Comment 39

[Mark Capella [:capella]]

quick look-ahead push to try: https://tbpl.mozilla.org/?tree=Try&rev=be10f1cbf881

Comment 40

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8375082 [details] [diff] [review]
bug927882.diff (v3)

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

r+ for the nsWindow.cpp bits. Thanks!

::: widget/android/nsWindow.cpp
@@ +1965,3 @@
>          }
>          break;
> +

Remove extra line here.

Comment 41

[:Margaret Leibovic]

Comment on attachment 8375082 [details] [diff] [review]
bug927882.diff (v3)

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

This looks reasonable to me, but I want to let wesj do the final review, since he's more familiar with the context around these changes. Thanks for digging into this!

Comment 42

[Wesley Johnston (:wesj)]

Comment on attachment 8375082 [details] [diff] [review]
bug927882.diff (v3)

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

::: mobile/android/chrome/content/SelectionHandler.js
@@ +192,1 @@
>        case "compositionend":

This feels a lot of a shotgun approach. Listen for everything! But at this point I'm frustrated enough with this that I think its our best solution. Just want to make sure we're not doing something nuts here. i.e. do we need keyup AND compositionupdate, or will just one be enough?

Comment 43

[Mark Capella [:capella]]

Since in comment #37 we noted that Swiftkey board doesn't generate key up/down/press events, it depends on the compositionFoo stuff.

The other keyboards (Google as the base case) don't generate compositionFoo events when individual keys are pressed, so we do need to observe both and use them in combination so serve the array of keyboards.

As for "frustrating" ... yes !!! :(  It's taken some time to figure this all out. My initial approaches made sense, tested ok, then failed on different keyboards ... so then I had to dig out the difference, conceive a new simplest form, restest, etc...

Comment 44

[Mark Capella [:capella]]

Obligatory push to try
https://tbpl.mozilla.org/?tree=Try&rev=1f26cd7680c3

Comment 45

[Mark Capella [:capella]]

Swing for the outfield !!!!

https://hg.mozilla.org/integration/fx-team/rev/a10bb6a9da93

Comment 46

[Carsten Book [:Tomcat]]

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