751513 - Typing characters in the contenteditable div causes the whole line to be deleted

Status: RESOLVED WORKSFORME

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

Description

[Martijn Wargers (dead)]

Attachment: testcase

I'm seeing this on the HTC Desire HD and on the Samsung Galaxy Nexus. I don't see it on the Galaxy SII, but I don't get an IME help there (the suggestion that appear while typing above the vkb, right?).

I don't see the problem occuring on an Auror build of 2012-04-02, so this looks like a regression.

Steps to reproduce:
- With the testcase, focus at the end of the text
- Type 2 characters on the vkb

Expected result:
- 2 characters added

Actual result:
- Whole line deleted, only the 2 typed characters remain

Comment 1

[Chris Peterson [:cpeterson]]

I was able to repro on Galaxy Nexus, but not Droid Pro with HKB.

Comment 2

[Catalin Suciu [:csuciu]]

This issue doesn't occur on:
Nightly 15.0a1 (2012-04-27)
http://hg.mozilla.org/mozilla-central/rev/450d8cd16316

but it occurs on:
Nightly 15.0a1 (2012-04-28)
http://hg.mozilla.org/mozilla-central/rev/b5c0d6abf69b

Possible range:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=450d8cd16316&tochange=b5c0d6abf69b

Comment 3

[Martijn Wargers (dead)]

Thanks, regression from bug 743468 perhaps?

Comment 4

[Chris Peterson [:cpeterson]]

mw22, I think you are correct. This looks like a regression from bug 743468.

Comment 5

[Chris Peterson [:cpeterson]]

Candidate for blocking-fennec1.0. This regression affects contenteditable div, not all text input.

Comment 6

[JP Rosevear [:jpr]]

Chris, we'll need you to decide if backing out bug 743468 is right or there is a good fix for this.

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

bug 743468 has been backed out of aurora, but this bug needs to be fixed soon if we want bug 743468 to make Fx14

Comment 8

[Chris Peterson [:cpeterson]]

Attachment: bug-751513-query-selection.patch

Query Gecko selection asynchronously (like XUL Fennec). The Android widget code alerts us BEFORE the selection has changed, so we must ask Gecko for the new selection range AFTER it has been changed.

This change also fixes some problems with "Select All" text not getting the correct selection range.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

http://hg.mozilla.org/mozilla-central/rev/43aa1f392da4

Should this have a test?

Comment 10

[Chris Peterson [:cpeterson]]

oops, I mistakenly listed THIS bug number in my commit message for bug 751864. THAT bug has been fixed.

Comment 11

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 624315 [details] [diff] [review]
bug-751513-query-selection.patch

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

::: mobile/android/base/GeckoInputConnection.java
@@ +502,1 @@
>          }

remove the curly braces

@@ +527,5 @@
> +                updateSelectionFromGecko(mGeckoSelectionStart, mGeckoSelectionEnd);
> +            }}, 200);
> +    }
> +
> +    private void updateSelectionFromGecko(int start, int end) {

this needs a little more explanation. Are we just hoping that this timer fires after the selection has been changed? I'd rather "just" fire a new event from gecko instead.

@@ +1092,5 @@
>      private boolean hasCompositionString() {
>          return mCompositionStart != NO_COMPOSITION_STRING;
>      }
> +
> +    private void queryGeckoSelection() {

which thread does this run on? Should be in a comment

@@ +1094,5 @@
>      }
> +
> +    private void queryGeckoSelection() {
> +        if (mQueryResult == null) {
> +            mQueryResult = new SynchronousQueue<String>();

do this in the class constructor

@@ +1107,5 @@
> +            Log.e(LOGTAG, "IME: queryGeckoSelection interrupted?!", e);
> +        }
> +    }
> +
> +    public void putQueryResult(String result, int selectionStart, int selectionEnd) {

which thread does this run on? Should be in a comment

Comment 12

[Chris Peterson [:cpeterson]]

The asynchronous queryGeckoSelection was a hack, but it is no longer necessary because I now understand the root cause.

Android is not getting the proper selection change notifications because Gecko is not calling Android widget's OnIMEFocusChange(). This is a regression caused by the fix for bug 612128. Fortunately, Ehsan has a fix in bug 688438.

Comment 13

[(no longer active)]

I relanded the fix to bug 688438 on the inbound branch.  Please let me know if I can help more.

Comment 14

[Johnathan Nightingale [:johnath]]

QA - does comment 13 mean that we've fixed this?

Comment 15

[Chris Peterson [:cpeterson]]

With Ehsan's fix for bug 688438, the contenteditable div's text is no longer deleted, BUT we are inserting new text in the wrong position. The first new word is correctly appended to the div, but subsequent words are inserted at the beginning of the div.

Comment 16

[Chris Peterson [:cpeterson]]

Still investigating. This bug only affects VKB when predictive text is enabled, so the bug is related to text selection around composition strings.

Comment 17

[Chris Peterson [:cpeterson]]

This bug has something to do with selection offsets being out-of-bounds when contenteditable divs contain plaintext and whitespace immediately before the </div> closing tag. Unfortunately, contenteditable div text is often followed by a newline before the </div> closing tag (including Mozilla's MDN example code).

<div contenteditable=true>TEXT WORKS</div>

<div contenteditable=true>TEXT AND SPACE DOES NOT WORK </div>

<div contenteditable=true>TEXT AND NEWLINE DOES NOT WORK
</div>

<div contenteditable=true><p>ELEMENT WORKS</p></div>

<div contenteditable=true><p>ELEMENT AND SPACE WORKS</p> </div>

<div contenteditable=true><p>ELEMENT AND NEWLINE WORKS</p>
</div>

<div contenteditable=true><p>ELEMENT AND TEXT WORKS</p>TEXT</div>

<div contenteditable=true><p>ELEMENT, SPACE, AND TEXT WORKS</p> TEXT</div>

<div contenteditable=true><p>ELEMENT, TEXT, AND SPACE DOES NOT WORK</p>TEXT </div>

Comment 18

[(no longer active)]

Do you also get assertions in a debug build when this happens, like bug 757771?

Comment 19

[Chris Peterson [:cpeterson]]

Ehsan, I don't see the assertions list in bug 757771, but nsContentEventHandler::OnSelectionEvent() logs an error message that nsContentEventHandler::SetRangeFromFlatTextOffset() failed because SetRangeFromFlatTextOffset's computed nativeOffset 32 was less than aNativeOffset 33:

https://hg.mozilla.org/mozilla-central/file/e96e0eaa6d85/content/events/src/nsContentEventHandler.cpp#l453


For comparison, a debug build of Mac Firefox logs these messages when entering Katakana characters using OS X's IME for the same test:

WARNING: NS_ENSURE_SUCCESS(res, res) failed with result 0x80530001: file ../../../../editor/libeditor/html/nsHTMLEditRules.cpp, line 8327
WARNING: NS_ENSURE_SUCCESS(res, res) failed with result 0x80530001: file ../../../../editor/libeditor/html/nsHTMLEditRules.cpp, line 8327
...

Even though Mac IME input seems to behave (more) correctly compared to Android, the problems seem somewhat related because NS_ERROR_DOM_INDEX_SIZE_ERR (0x80530001) is being returned from SetEnd().

Comment 20

[(no longer active)]

Hmm, OK, then I'm not sure what's going on here.  :/

Comment 21

[Chris Peterson [:cpeterson]]

We should consider bumping this bug from 1.0 to .N+ blocker. I am getting closer to a fix, but the fix would be risky. Some of my other 1.0 blockers look easier to fix and probably affect more users (e.g. bug 755517).

Comment 22

[Brad Lassey [:blassey] (use needinfo?)]

I'm under the impression that this is only a problem when there is autocomplete suggestions. Can we bandaid this for 14 (and perhaps 15) by turning off autocomplete suggestions when editing a content editable?

Comment 23

[Chris Peterson [:cpeterson]]

Android IME has a "no suggestions" hint (InputType.TYPE_TEXT_FLAG_NO_SUGGESTIONS) we can toggle at runtime, but contenteditable would still break for IMEs that must use composition strings (like Swype or Simeji).

Comment 24

[Chris Peterson [:cpeterson]]

Attachment: contenteditable.html

Added simpler test case demonstrating contenteditables with and without trailing whitespace.

nsPlaintextEditor is collapsing the contenteditable's trailing whitespace (in nsWSRunObject), but not notifying Java.

Comment 25

[(no longer active)]

How should it tell Java?

Comment 26

[Chris Peterson [:cpeterson]]

I see that the editor is notifying the Android widget (and Java) of the trailing whitespace changes with nsWindow::OnIMETextChange(). The crux is that Gecko trims trailing whitespace _in response_ to Java inserting new text and then the OnIMETextChange() callback reenters the Java code:

1. Java asks Gecko to insert "C" at position 4 of "A B ".
2. Gecko notifies Java that the text is being trimmed to "A B". Unfortunately, this callback reenters Java when Android IME (Google's code, not ours) does not permit text changes!
3. Gecko tries to insert "C" at position 4, but "A B" is now only three characters long.
4. Gecko has "A BC", but Java thinks the text is "A B C".

I think I will need to detect these callbacks and reset the Java IME text to the Gecko text when feasible.

Comment 27

[(no longer active)]

Can't we just report the text as "A B" from the beginning?

Comment 28

[Chris Peterson [:cpeterson]]

Attachment: contenteditable.html

oops: previous contenteditable.html test was saved as plaintext, not HTML.

Comment 29

[Chris Peterson [:cpeterson]]

Ehsan, are you suggesting Gecko's libeditor report "A B" for ALL platforms? Or that Android's widget code could auto-trim whitespace before reporting it to Android's system IME?

Note that Chrome and Safari trim the trailing whitespace BEFORE populating the contenteditable. Desktop Firefox populates the contenteditable with trailing whitespace and later trims it when text is appended.

You can see this using this bug's contenteditable.html test. Just select-all "TRAILING SPACE " or "TRAILING NEWLINE " and paste into another text editor:

https://bug751513.bugzilla.mozilla.org/attachment.cgi?id=630745

Comment 30

[(no longer active)]

I'm suggesting doing this on all platforms.  I believe we discussed it at some point (roc do you remember) but I can't find the bug right now.  Basically, our behavior is weird, since such trailing spaces are collapsed for non-editable content, and it doesn't make a lot of sense for us to expose them for editable content.

Note that I'm not suggesting just lying to the IME layer (report "A B" when the thing you're editing is really "A B ").  I'm suggesting trimming the trailing space the same way that WebKit does, and then report the real value (which would be "A B") to the IME layer.

Aryeh, what does the spec say about this, if anything?

Comment 31

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Bug 681626. Aryeh updated the spec partly based on our discussion.

Comment 32

[Chris Peterson [:cpeterson]]

If I understand bug 681626 correctly, the conclusion was that trailing whitespace should be trimmed *when* text is deleted or inserted. The trailing whitespace is there and you can position the caret there. That is what I see Gecko doing today.

In contrast, Chrome and Safari trim the whitespace *before* displaying the text. You cannot position or move the caret beyond the contenteditable div's last non-whitespace character.

Comment 33

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

(In reply to Ehsan Akhgari [:ehsan] from comment #30)
> Note that I'm not suggesting just lying to the IME layer (report "A B" when
> the thing you're editing is really "A B ").  I'm suggesting trimming the
> trailing space the same way that WebKit does, and then report the real value
> (which would be "A B") to the IME layer.
> 
> Aryeh, what does the spec say about this, if anything?

Bug 681626 comment 15 ff. contains the relevant info (thanks, roc!).  From comment 32, it sounds like Gecko is already doing that.

There's something further we could do: don't allow the cursor to be positioned after insignificant whitespace that's at the end of a line.  If the cursor would be placed there, move it back to before the whitespace.  Then nothing will try inserting after the to-be-stripped whitespace to begin with.

(In reply to Chris Peterson (:cpeterson) from comment #32)
> In contrast, Chrome and Safari trim the whitespace *before* displaying the
> text. You cannot position or move the caret beyond the contenteditable div's
> last non-whitespace character.

WebKit's selection handling is very weird.  IIUC, it's based on the visible text -- their internal selection class is called VisibleSelection.  They translate back and forth to DOM locations when the DOM APIs require it.  But for instance, if you have something like
  <b>foo</b><i>bar</i>
you'll never have the selection at (<b>, 0) or (<b>, 1) or in between the <b> and <i>.  Such a selection point can't be represented in WebKit.  If you try using addRange() or collapse() or something to get it to go there, it will go to (foo, 0) or (foo, 3) or whatever instead.  This results in all kinds of bugs.

The general idea of selection normalization is essential, though, and we don't do enough of it -- WebKit is better in that respect.  We should be deciding on a normalization routine that will make the selection points as predictable as possible, so we can't have things like <b>foo{</b><i>b]ar</i> (which looks just like <b>foo</b><i>[b]ar</i> but in Gecko behaves differently).  We should then call that routine often, such as before any command or user input.  I've been meaning to spec this for a long time.

Comment 34

[(no longer active)]

OK, so I think that our caret placement should be fixed to make sure that we can't place or move the caret past the trimmed whitespace (which is essentially the fix to bug 681626).  Unfortunately I don't have enough time to work on that at the moment (unless Aryeh wants to jump in and help :), but in the mean time, if we stop reporting the trimmed whitespace to the IME layer, that would fix this bug, right?

Comment 35

[Chris Peterson [:cpeterson]]

(In reply to Ehsan Akhgari [:ehsan] from comment #34)
> but in the mean time, if we stop reporting the trimmed whitespace to the IME
> layer, that would fix this bug, right?

I think so, because Android IME would not try to insert text within or after trailing whitespace that Gecko will trim.

I may be able to implement a less invasive fix just for Android. When Gecko's OnIMETextChange notification reenters Java when Android's IME is busy, I can save the text change and apply it later.

Comment 36

[Chris Peterson [:cpeterson]]

Fixed by my fix for bug 769520.

Comment 37

[Chris Peterson [:cpeterson]]

Fixed in Aurora 15 because bug 769520 was uplifted to m-a.

Comment 38

[Scoobidiver (away)]

Reopening because of the backout of bug 769250.

Comment 39

[Chris Peterson [:cpeterson]]

Even though bug 769520 was backed out, this testcase still passes with Nightly 18.0a1 2012-09-10).

Comment 40

[Scoobidiver (away)]

Is it fixed in 15.0.1, 16.0 Beta 3 and Aurora 17?

Comment 41

[Chris Peterson [:cpeterson]]

Good question, Scoobidiver. I just double-checked the browser versions I tested and discovered that 15.0.1 and Beta 16.0b3 have not been published yet. <:)

This testcase works for me on Nightly 18 and Aurora 17 (which both have my backout), but I will keep this bug open until I can also test 15.0.1 and Beta 16.0b3, too.

Comment 42

[Scoobidiver (away)]

15.0.1 can be downloaded from ftp://ftp.mozilla.org/pub/mobile/candidates/15.0.1-candidates/build2/android/en-US/fennec-15.0.1.en-US.android-arm.apk
I guess if it works in 15.0.1 and 17.0a2, it will work in 16.0b3 that will be built in two days.

Comment 43

[Brad Lassey [:blassey] (use needinfo?)]

qawanted to retest

Comment 44

[Teodora Vermesan (:TeoVermesan)]

Device: HTC Desire HD (Android 2.3.5) 
Build: Firefox for Android 30.0a1 (2014-02-26)
I cannot reproduce the issue. With the testcase given if I type 2 characters at the end of the text, 
the line isn't deleted, and the 2 characters are present after the existing text.