Closed Bug 751513 Opened 8 years ago Closed 6 years ago

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

Categories

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

16 Branch
ARM
Android
defect
Not set

Tracking

()

RESOLVED WORKSFORME
Tracking Status
blocking-fennec1.0 --- -
fennec + ---

People

(Reporter: martijn.martijn, Unassigned)

References

(Depends on 1 open bug, )

Details

(Keywords: regression, testcase, Whiteboard: VKB)

Attachments

(3 files, 1 obsolete file)

Attached file 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
I was able to repro on Galaxy Nexus, but not Droid Pro with HKB.
Whiteboard: VKB
Thanks, regression from bug 743468 perhaps?
mw22, I think you are correct. This looks like a regression from bug 743468.
Candidate for blocking-fennec1.0. This regression affects contenteditable div, not all text input.
Assignee: nobody → cpeterson
blocking-fennec1.0: --- → ?
Depends on: 743468
Chris, we'll need you to decide if backing out bug 743468 is right or there is a good fix for this.
blocking-fennec1.0: ? → +
bug 743468 has been backed out of aurora, but this bug needs to be fixed soon if we want bug 743468 to make Fx14
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.
Attachment #624315 - Flags: review?(blassey.bugs)
Status: NEW → ASSIGNED
http://hg.mozilla.org/mozilla-central/rev/43aa1f392da4

Should this have a test?
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → Firefox 15
oops, I mistakenly listed THIS bug number in my commit message for bug 751864. THAT bug has been fixed.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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
Attachment #624315 - Flags: review?(blassey.bugs) → review-
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.
Depends on: 612128, 688438
I relanded the fix to bug 688438 on the inbound branch.  Please let me know if I can help more.
QA - does comment 13 mean that we've fixed this?
Keywords: qawanted
Blocks: 743468
No longer depends on: 743468
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.
Keywords: qawanted
Still investigating. This bug only affects VKB when predictive text is enabled, so the bug is related to text selection around composition strings.
Target Milestone: Firefox 15 → ---
Version: Trunk → Firefox 16
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>
Do you also get assertions in a debug build when this happens, like bug 757771?
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().
Hmm, OK, then I'm not sure what's going on here.  :/
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).
blocking-fennec1.0: + → ?
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?
tracking-fennec: --- → 15+
blocking-fennec1.0: ? → -
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).
Attached file contenteditable.html (obsolete) —
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.
How should it tell Java?
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.
Can't we just report the text as "A B" from the beginning?
Attached file contenteditable.html
oops: previous contenteditable.html test was saved as plaintext, not HTML.
Attachment #630668 - Attachment is obsolete: true
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
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?
Bug 681626. Aryeh updated the spec partly based on our discussion.
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.
Depends on: 681626
(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.
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?
(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.
Depends on: 764025
Depends on: 769520
Fixed by my fix for bug 769520.
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 16
Fixed in Aurora 15 because bug 769520 was uplifted to m-a.
Reopening because of the backout of bug 769250.
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
Target Milestone: Firefox 16 → ---
Even though bug 769520 was backed out, this testcase still passes with Nightly 18.0a1 2012-09-10).
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
Is it fixed in 15.0.1, 16.0 Beta 3 and Aurora 17?
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
tracking-fennec: 15+ → +
Assignee: cpeterson → nobody
Status: REOPENED → NEW
qawanted to retest
Keywords: qawanted
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.
Status: NEW → RESOLVED
Closed: 7 years ago6 years ago
Keywords: qawanted
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.