Closed Bug 927882 Opened 8 years ago Closed 7 years ago

Standardize TextSelection caret positioning logic for soft keyboards

Categories

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

All
Android
defect
Not set
normal

Tracking

(firefox24 unaffected, firefox25 unaffected, firefox26 affected, firefox27 affected, fennec+)

RESOLVED FIXED
Firefox 30
Tracking Status
firefox24 --- unaffected
firefox25 --- unaffected
firefox26 --- affected
firefox27 --- affected
fennec + ---

People

(Reporter: pretzer, Assigned: capella)

References

Details

(Keywords: regression)

Attachments

(1 file, 4 obsolete files)

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
What's the behaviour in Beta/Aurora?
Sorry, I don't have these channels installed on my device... not enough storage. Can you test?
Firefox for Android 25 Beta 8 : not reproducible
Firefox for Android 26.0a2 (2013-10-20) : reproducible
tracking-fennec: --- → ?
Capella, could you have look at this one?
Assignee: nobody → markcapella
tracking-fennec: ? → 26+
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?
Flags: needinfo?(peter.retzer)
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 ...
Attached patch bug927882 (obsolete) — Splinter Review
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.
Attachment #826613 - Flags: review?(margaret.leibovic)
(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! :-)
Flags: needinfo?(peter.retzer)
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.
Attachment #826613 - Flags: feedback?(nchen)
Attachment #826613 - Flags: feedback?(bnicholson)
Comment on attachment 826613 [details] [diff] [review]
bug927882

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

LGTM
Attachment #826613 - Flags: feedback?(nchen) → feedback+
Comment on attachment 826613 [details] [diff] [review]
bug927882

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

LGTM too.
Attachment #826613 - Flags: feedback?(bnicholson) → feedback+
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.
Attachment #826613 - Flags: feedback+
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.
oh ouch
Duplicate of this bug: 934470
Duplicate of this bug: 934470
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 :(
Attachment #826613 - Flags: review?(margaret.leibovic)
This still being worked on for 26?
Status: NEW → ASSIGNED
It's my followup now that bug 934470 looks like it's clearing.
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.
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?
Flags: needinfo?(markcapella)
fyi bug 946030 should correct being unable to receive a caret on tapping into a textarea.
Flags: needinfo?(markcapella)
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.
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...
Attached patch BLUR (obsolete) — Splinter Review
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.
Attachment #826613 - Attachment is obsolete: true
Attachment #8349878 - Flags: review?(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 :)
Attachment #8349878 - Flags: review?(margaret.leibovic) → review+
Assignee: markcapella → nobody
Status: ASSIGNED → NEW
Assignee: nobody → markcapella
Status: NEW → ASSIGNED
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.
Flags: needinfo?(markcapella)
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.
Attachment #8349878 - Flags: review+
Flags: needinfo?(markcapella)
Blocks: 950085
tracking-fennec: 26+ → +
Assignee: markcapella → nobody
Status: ASSIGNED → NEW
If we no longer this._deactivate(); on composition:foo events, we should at least update the caret position in that event.
Assignee: nobody → markcapella
Attached patch bug927882.diff (obsolete) — Splinter Review
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.
Attachment #8349878 - Attachment is obsolete: true
Attachment #8371723 - Flags: review?(nchen)
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?
Attachment #8371723 - Flags: review?(nchen) → feedback+
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.
Attached patch bug927882.diff (v2) (obsolete) — Splinter Review
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
Attachment #8371723 - Attachment is obsolete: true
Attachment #8372695 - Flags: review?(wjohnston)
Attachment #8372695 - Flags: review?(nchen)
Attachment #8372695 - Flags: review?(margaret.leibovic)
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)
Attachment #8372695 - Flags: review?(nchen) → feedback+
|_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.
Status: NEW → ASSIGNED
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?
Attachment #8372695 - Flags: review?(wjohnston)
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
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!)
Attachment #8372695 - Attachment is obsolete: true
Attachment #8372695 - Flags: review?(margaret.leibovic)
Attachment #8375082 - Flags: review?(wjohnston)
Attachment #8375082 - Flags: review?(nchen)
Attachment #8375082 - Flags: review?(margaret.leibovic)
Summary: Regression: SwiftKey's jump-to-end feature causes thumb handle to disappear (which prevents panning through text) → Standardize TextSelection caret positioning logic for soft keyboards
quick look-ahead push to try: https://tbpl.mozilla.org/?tree=Try&rev=be10f1cbf881
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.
Attachment #8375082 - Flags: review?(nchen) → review+
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!
Attachment #8375082 - Flags: review?(margaret.leibovic) → feedback+
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?
Attachment #8375082 - Flags: review?(wjohnston) → review+
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...
https://hg.mozilla.org/mozilla-central/rev/a10bb6a9da93
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 30
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.