982608 - Geckoeditable data loss using Swift keyboard during Text Selection

Status: RESOLVED FIXED

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

Description

[Mark Capella [:capella]]

Follow up to bug 978731 comment #2 ...

A combination of events (notably using Swift keyboard) and involving autoSuggest/autoCorrect and Text Selection handle movements causes characters to be dropped.

See the attached STR
   https://www.dropbox.com/s/lzw6gb1npzfv7dl/bug978731c6.mp4

This seems similar to bug 969929 ... that solution was implemented in SelectionHandler code, but a bigger picture fix with IME advice and/or code seems indicated.

Comment 1

[Mark Capella [:capella]]

On further examination, TextSelection doesn't seem to be involved ...

STR:
   Enable SwiftKey as your device's Input Method
   Open testcase:   http://jsfiddle.net/H7j4g/
   Tap into <input> field to the right of the text to focus and
      open the soft keyboard
   (cursor "|" s/b blinking at end-of-field)
   Wait, observe the |X| in the text disappear

Comment 2

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

Unfortunately, manipulating the selection, when there's a composition, is not well supported. For the case of text selection, I guess we can have a "no composition" mode where we don't start compositions during text selection.

Comment 3

[Mark Capella [:capella]]

Can you explain the mechanics a little more? Do you suggest that from Gecko we could selectively clear-disable and re-enable composition processing ?

Comment 4

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

(In reply to Mark Capella [:capella] from comment #3)
> Can you explain the mechanics a little more? Do you suggest that from Gecko
> we could selectively clear-disable and re-enable composition processing ?

Pretty much. The steps would be,

1) TextSelection disables compositions on start selection
2) Compositions are disabled (i.e. no underline)
3) TextSelection enables compositions on end selection

It's not currently possible to disable compositions so we have to add that feature.

Comment 5

[Mark Capella [:capella]]

So that would help with TextSelection issues, such as described in comment #0, but leave open rare issues where content providers get "creative" such as conceived in comment #1 STR ...

This works for me :) How can I help? Is this scoped as a big change? Do we trigger via message from Gecko intercepted and acted on ... where? Java / GeckoEditable / GeckInputConnection?

Comment 6

[Mark Capella [:capella]]

notes:

   When nsWindow.cpp processes the "IME_SET_SELECTION" / setSelectionRange(12, 13), it first calls RemoveIMEComposition() which is supposed to remove the composition, and leave the text content unaffected, using AutoIME selection and text masks.

   http://mxr.mozilla.org/mozilla-central/source/widget/android/nsWindow.cpp?mark=1803-1805#1784

   However, when RemoveIMEComposition() dispatchs the NS_COMPOSITION_END event after dispatching the previous NS_TEXT_TEXT event, Gecko notices and modifies the <input> value ... ( same example where 'X' is lost in text using STR in comment #1 ).

   Brief / targeted log:

    (25532): nsWindow::RemoveIMEComposition()    Dispatchs WidgetCompositionEvent
                                                 ( NS_COMPOSITION_END ) Starting...

    (25532): nsWindow::DispatchEvent() Starts
    (25532): nsWindow::DispatchEvent()    Calls Event Handler Starts
    (25532): WidgetEvent::HasIMEEventMessage() Starts ...
    (25532): WidgetEvent::HasIMEEventMessage() Starts ...
    (25532): WidgetEvent::HasIMEEventMessage() Starts ...
    (25532): WidgetEvent::HasIMEEventMessage() Starts ...
    (25532): WidgetEvent::HasIMEEventMessage() Starts ...
    (25532): TextComposition::DispatchEvent() Starts ...
    (25532): TextComposition::DispatchEvent()     nsEventDispatcher::Dispatch() ... STARTS
       browser.js: !!!!!!!!!! HEARD INPUT EVENT !!!!!!!!!! [ input ]
       browser.js: !!!!!!!!!! HEARD INPUT EVENT !!!!!!!!!! [ abcdefghijklmnopqrstuvwxyz ]
    (25532): TextComposition::DispatchEvent()     nsEventDispatcher::Dispatch() ... FINISHES
    (25532): TextComposition::DispatchEvent()     NotityUpdateComposition() ... STARTS
    (25532): TextComposition::DispatchEvent()     NotityUpdateComposition() ... FINISHES
    (25532): TextComposition::DispatchEvent() Finishes ...
    (25532): nsWindow::DispatchEvent()    Calls Event Handler Finishes
    (25532): nsWindow::DispatchEvent()    Was: NS_COMPOSITION_END
    (25532): nsWindow::DispatchEvent() Returns ...

    (25532): nsWindow::RemoveIMEComposition()    Dispatchs WidgetCompositionEvent
                                                 ( NS_COMPOSITION_END ) Finishing...

Comment 7

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

(In reply to Mark Capella [:capella] from comment #6)
> 
>    However, when RemoveIMEComposition() dispatchs the NS_COMPOSITION_END
> event after dispatching the previous NS_TEXT_TEXT event, Gecko notices and
> modifies the <input> value ... ( same example where 'X' is lost in text
> using STR in comment #1 ).

At that point, the selection was already changed during the composition, and Gecko gets confused when you try to remove the composition.

To disable having compositions, we need to not handle IME_UPDATE_COMPOSITION in nsWindow.cpp [1] or not handle icUpdateGecko in GeckoEditable [2]. Shouldn't be too big of a change.

[1] http://mxr.mozilla.org/mozilla-central/source/widget/android/nsWindow.cpp#1992
[2] http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/GeckoEditable.java#397

Comment 8

[Mark Capella [:capella]]

Well, I'm wondering about something more basic as the cause. I don't see any reason to be removing composition here: http://mxr.mozilla.org/mozilla-central/source/widget/android/nsWindow.cpp?mark=1940-1940#1931

Simply chaning / setting selection, shouldn't affect composition aiui (?)

"The composing region and the selection are completely independent of each other"

Quick testing this solution solves the problems I've detailed in comment #0 and comment #1 ...

Can you comment / fill in any background I might overlook?

Comment 9

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

(In reply to Mark Capella [:capella] from comment #8)
> Well, I'm wondering about something more basic as the cause. I don't see any
> reason to be removing composition here:
> http://mxr.mozilla.org/mozilla-central/source/widget/android/nsWindow.
> cpp?mark=1940-1940#1931
> 
> Simply chaning / setting selection, shouldn't affect composition aiui (?)

We only send IME_SET_SELECTION when there's no composition on the Java side, http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/GeckoEditable.java#428. In that case, we shouldn't have composition on the Gecko side either.

> "The composing region and the selection are completely independent of each
> other"

That's true for Android composing regions and selection. That's not quite true for Gecko composition and selection.

Comment 10

[Mark Capella [:capella]]

re: |In that case, we shouldn't have composition on the Gecko side either.|

In this example, nsWindow.cpp gets two NOTIFY_IME_OF_SELECTION_CHANGE messages from Gecko, (first for 12,12 and the second for 12,13) each of which it flushes to GeckoEditable.java for action.

The second one in GeckoEditable triggers an IME_SET_SELECTION back to nsWindow which then tries to remove a composition string it sees.

(visually underlined from left of text @ 0 to start of cursor @ 12).
(mIMEComposing is true, the composing text string is |abcdefghijkl|).

nsWindow tries to RemoveIMEComposition() which is where Gecko loses it.

RemoveIMEComposition() first dispatches a masked NS_TEXT_TEXT, which causes TextComposition.cpp & IMEContentObserver.cpp to try to back out two text compositions:

   First: [ start , aOldEnd , aNewEnd ] : [ 12 , 13 , 12 ]
             -- which isn't really part of the composition, and ends up deleted
    then: [ start , aOldEnd , aNewEnd ] : [ 0 , 12 , 12 ]
             -- which is the part that actually was composing / underlined

RemoveIMEComposition() then dispatches a final NS_COMPOSITION_END which forces TextComposition to commit (?) the DOM change to the <input> element and deletes the data.


re: |That's not quite true for Gecko composition and selection.|

I suppose what you refer to here is potential interaction between Gecko/DOM selection and composition changes such as auto-completes / auto-suggestions provided by search <inputs>, or auto-completes provided by <select> boxes associated with email, telephone numbers, etc.


I don't yet understand how you see implementing "disabling compositions" as soluble in this case, so I'm still digging through the current mechanics.

Comment 11

[Mark Capella [:capella]]

also fyi, GeckoEditable makes the decision that there isn't a composition and generates the IME_SET_SELECTION because it doesn't see a span of type SPAN_COMPOSING.

There actually are two spans, (start/end) of 12,12 and 13,13. The first one has flag SPAN_INTERMEDIATE set instead.

Checking for both flags solves the problems I'm researching, but regresses temporary composition and it's attendant underlining in other ways.

Comment 12

[Mark Capella [:capella]]

Attachment: bug982608.diff (v0)

This works (as discussed) to fix the common problem with data loss due to text selection changes generated during Android SelectionHandler use conflicting with the IME composition code, as pointed out in comment #0.

This will not correct the more generic case where data loss is incurred as a result of programmatic selection changes initiated in Gecko, as pointed out in comment #1. I'll file a followup bug for that issue.

I still think there's merit to the thought that we can avoid calling removeIMEComposition() during selection only event IME_SET_SELECTION.

That one line was added here: https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=842013&attachment=730239, but doesn't indicate why it specifically was required.

Comment 13

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

Comment on attachment 8395191 [details] [diff] [review]
bug982608.diff (v0)

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

f+ for changes in GeckoEditable. With this fix, can we take out the "blur()/focus()" fix that was added last time?

::: mobile/android/base/GeckoEditable.java
@@ +103,5 @@
>      private int mIcUpdateSeqno;
>      private int mLastIcUpdateSeqno;
>      private boolean mUpdateGecko;
>      private boolean mFocused;
> +    private boolean mSuppressCompositions;

Needs to be volatile because mSuppressCompositions is used by two threads (Gecko and InputConnection).

@@ +360,5 @@
>  
>          mIcRunHandler = mIcPostHandler = ThreadUtils.getUiHandler();
> +
> +        mSuppressCompositions = false;
> +        GeckoAppShell.getEventDispatcher().registerEventListener("TextSelection:IMECompositions", this);

Right now we only register but don't unregister this event. Can we switch to registerEventListener when focusing and unregisterEventListener when blurring? (e.g. within this block [1])

[1] http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/GeckoEditable.java?rev=9788a3f558e6&mark=739-745#739

Also nit: indentation and use two lines.

@@ +403,5 @@
>  
>      private void icUpdateGecko(boolean force) {
>  
> +        // Skip compositoins if duplicate sequence that's not overriden, or if suppressing
> +        // during text selection changes created by SelectionHandler UI.

Maybe "Skip if receiving repeated request or if suppressing compositions during text selection."

@@ +1213,5 @@
> +    // GeckoEventListener implementation
> +
> +    @Override
> +    public void handleMessage(String event, JSONObject message) {
> +        if (!event.equals("TextSelection:IMECompositions")) {

Nit: !"TextSelection:IMECompositions".equals(event)

Comment 14

[Mark Capella [:capella]]

Attachment: bug982608.diff

Thanks! The only thing I can't do is something about the previous blur()/focus() solution for bug 978731.

For this patch, the idea is to avoid generating composed text ranges during successive code. For bug 978731, we want to clear any previously created ranges, prior to successive code.

However, I do want to get rid of that hack. I can follow-up with a patch that would (say) generate:

{ type: "TextSelection:IMECompositions", clear: true }
--- or remove: true or something ---

Then catch the AndroidGeckoEvent in nsWindow.cpp and trigger RemoveIMEComposition() ?

Comment 15

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

For removing the composition, you have a couple of options.
* You can send an IME_REMOVE_COMPOSITION event from the input connection thread [1]
* You can use nsIEditorIMESupport.forceCompositionEnd() from JS [2]
You may want to try both.

[1] Example at http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/GeckoEditable.java?rev=c69c55582faa#433
[2] Example at http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js?rev=c69c55582faa#5211

Comment 16

[Mark Capella [:capella]]

Filed: bug 987941

Comment 17

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

Comment on attachment 8396621 [details] [diff] [review]
bug982608.diff

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

r+ for changes in GeckoEditable.java. You should still ask Margaret or someone else to review the changes in SelectionHandler.js.

::: mobile/android/base/GeckoEditable.java
@@ +399,5 @@
>      }
>  
>      private void icUpdateGecko(boolean force) {
>  
> +        // Skip if receiving a repeated request, or 

Nit: trailing space

Comment 18

[Mark Capella [:capella]]

Comment on attachment 8396621 [details] [diff] [review]
bug982608.diff

Oh sure :) 

(I held off there to avoid review-flooding if I needed another iteration through you).

Comment 19

[:Margaret Leibovic]

Comment on attachment 8396621 [details] [diff] [review]
bug982608.diff

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

I'm not familiar with GeckoEditable, so I trust jchen has that covered, but I have some questions about the SelectionHandler changes.

::: mobile/android/chrome/content/SelectionHandler.js
@@ +193,5 @@
> +  // Act on selectionChange notifications when not dragging handles, allow dynamic
> +  // IME text compositions (autoSuggest, autoCorrect, etc)
> +  _stopDraggingHandles: function sh_stopDraggingHandles() {
> +    this._draggingHandles = false;
> +    sendMessageToJava({ type: "TextSelection:IMECompositions", suppress: false });

Should we also have a check in here to avoid sending this message if this._draggingHandles is already false, similar to the check in _startDraggingHandles?

@@ +601,5 @@
>        // Ensure targetElement is now focused normally
>        aElement.focus();
>      }
>  
> +    this._stopDraggingHandles();

Why is this new call needed? Shouldn't this._draggingHandles already be false if we're initializing a new target?

Comment 20

[Mark Capella [:capella]]

I avoid sending extra messages in _startDragging() as we respond to a stream of "TextSelection:Move" messages during handle dragging, and only need to disable dynamic compositions @ the first one.

I didn't check similarly in _stopDragging() as that should only happen once after we _startDragging(), though double checking it wouldn't hurt anything.

The extra _stopDragging() in _initTarget() was precautionary.

Comment 21

[:Margaret Leibovic]

(In reply to Mark Capella [:capella] from comment #20)
> I avoid sending extra messages in _startDragging() as we respond to a stream
> of "TextSelection:Move" messages during handle dragging, and only need to
> disable dynamic compositions @ the first one.
> 
> I didn't check similarly in _stopDragging() as that should only happen once
> after we _startDragging(), though double checking it wouldn't hurt anything.
> 
> The extra _stopDragging() in _initTarget() was precautionary.

Thanks for the responses. Since the _stopDragging call is precautionary, that would be a case where we would send an unnecessary message, so I would be happier if we had a similar check in _stopDragging.

Comment 22

[:Margaret Leibovic]

Comment on attachment 8396621 [details] [diff] [review]
bug982608.diff

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

I'm not familiar with GeckoEditable, so I trust jchen has that covered, but I have some questions about the SelectionHandler changes.

::: mobile/android/chrome/content/SelectionHandler.js
@@ +193,5 @@
> +  // Act on selectionChange notifications when not dragging handles, allow dynamic
> +  // IME text compositions (autoSuggest, autoCorrect, etc)
> +  _stopDraggingHandles: function sh_stopDraggingHandles() {
> +    this._draggingHandles = false;
> +    sendMessageToJava({ type: "TextSelection:IMECompositions", suppress: false });

Should we also have a check in here to avoid sending this message if this._draggingHandles is already false, similar to the check in _startDraggingHandles?

@@ +601,5 @@
>        // Ensure targetElement is now focused normally
>        aElement.focus();
>      }
>  
> +    this._stopDraggingHandles();

Why is this new call needed? Shouldn't this._draggingHandles already be false if we're initializing a new target?

Comment 23

[:Margaret Leibovic]

Oops, ignore that last comment, splinter re-posted my previous comments with my r+ change.

Comment 24

[Mark Capella [:capella]]

Cool responses ... nice additional polish :-D

https://tbpl.mozilla.org/?tree=Try&rev=cc44302ee796

Comment 25

[Mark Capella [:capella]]

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

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7f89526040a4