792928 - back spacing to delete a whole word in a text field deletes all but one word in the entire text field

Status: VERIFIED FIXED

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

Description

[Karen Rudnitski [:kar]]

Attachment: 1 - typing in several words

You can see sequence of events in the 3 screenshots (numbered 1, 2, 3).

STR:
- open Facebook and put cursor in 'status' field
- begin typing three or four words
- start back-spacing (like you're correcting a word) until the entire word is deleted

ER: only the characters you have backspaced are deleted
AR: all of the text in the field is deleted save for a few letters / one word

Happens all the time on my Galaxy Nexus on Nightly, Aurora and Beta

Comment 1

[Karen Rudnitski [:kar]]

Attachment: 2 - back spacing

Comment 2

[Karen Rudnitski [:kar]]

Attachment: 3 - deleted text

Comment 3

[Chris Peterson [:cpeterson]]

gcp, this bug is likely fallout from bug 769520. There are race conditions between the Gecko thread and the IME events on the UI thread. This IME code is very fragile. Fixing the races introduces new bugs.

Comment 5

[Benoit Girard (:BenWa)]

I've seen this bug a lot typing in the bugzilla comment box making it impossible to type a non trivial bug. I though it might of been related to the spelling suggestion replacing the text box but I'm not sure.

Comment 6

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

Attachment: Clip selection to end when offsets are out of bounds

Masayuki-san, can you review this patch?

Because of Bug 769520, sometimes we send NS_SELECTION_SET with offsets that are out of bounds. The current behavior in SetRangeFromFlatTextOffset is to select the entire text when the offsets are out of bounds, but I think the proper behavior should be to clip the offsets to the end of the text. This new behavior fixes this bug.

Comment 7

[Mozilla RelEng Bot]

Try run for deb49bc0d7e9 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=deb49bc0d7e9
Results (out of 141 total builds):
    success: 135
    warnings: 6
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-deb49bc0d7e9

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Hmm, I don't understand well what happened in the screenshot.

This bug report is about back spacing the text. Why is it related to NS_SELECTION_SET? Sounds like some events are processed synchronously but others are not so?

Could you give an example of the event order of NS_COMPOSITION_*, NS_TEXT_TEXT, NS_QUERY_* and NS_SET_SELECTION? I want to know the difference between the event order which is received by the content process and expected event order if all of them are handled synchronously.

Comment 9

[Mozilla RelEng Bot]

Try run for deb49bc0d7e9 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=deb49bc0d7e9
Results (out of 142 total builds):
    success: 136
    warnings: 6
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-deb49bc0d7e9

Comment 10

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

Attachment: Bug causing steps

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #8)
> Hmm, I don't understand well what happened in the screenshot.
> 
> This bug report is about back spacing the text. Why is it related to
> NS_SELECTION_SET? Sounds like some events are processed synchronously but
> others are not so?
> 
> Could you give an example of the event order of NS_COMPOSITION_*,
> NS_TEXT_TEXT, NS_QUERY_* and NS_SET_SELECTION? I want to know the difference
> between the event order which is received by the content process and
> expected event order if all of them are handled synchronously.

Yes the basic issue is that the Gecko code and the Java code are not in sync.

I made a chart of the sequence of steps to cause the bug. Hope it'll explain it better. Please see the attached image.

The correct solution would be to properly sync the two sides, but the fix in the patch is a good bandaid fix. And I think the new behavior for NS_SELECTION_SET is more reasonable than the current behavior.

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 672511 [details] [diff] [review]
Clip selection to end when offsets are out of bounds

Okay, I don't like this patch for the solution of the problem. However, I also think that your patch fixes actual bug in the nsContentEventHandler.

Basically, this looks okay. mRootContent is probably not a text node. But if it's so, it causes odd behavior. Please add MOZ_ASSERT() before SetStart():

   NS_ASSERTION(domNode, "lastContent doesn't have nsIDOMNode!");
-  if (!content) {
-    rv = aRange->SetStart(domNode, 0);
+  if (!startSet) {
>    MOZ_ASSERT(!mRootContent->IsNodeOfType(nsINode::eTEXT));
+    rv = aRange->SetStart(domNode, int32_t(mRootContent->GetChildCount()));
     NS_ENSURE_SUCCESS(rv, rv);
   }

Comment 12

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

Addressed review comments.

https://hg.mozilla.org/integration/mozilla-inbound/rev/c395a0974676

Comment 13

[Ryan VanderMeulen [:RyanVM]]

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

Comment 14

[Demetrius S.]

Can this be uplifted as it's a very annoying bug that affects every version.

Comment 15

[Aaron Train [:aaronmt]]

(In reply to Demetrius Schwab from comment #14)
> Can this be uplifted as it's a very annoying bug that affects every version.

It needs to land on Aurora.

Comment 17

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

Comment on attachment 672511 [details] [diff] [review]
Clip selection to end when offsets are out of bounds

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 

User impact if declined: Every time the user backspaces, the entire text is deleted; very annoying and severely disrupts typing in Fennec

Testing completed (on m-c, etc.): On m-c since Oct 19, and on try (https://tbpl.mozilla.org/?tree=Try&rev=deb49bc0d7e9)

Risk to taking this patch (and alternatives if risky): Very little; the code path is used mostly on Android only

String or UUID changes made by this patch: None

Comment 19

[Alex Keybl [:akeybl]]

Similar to bug 712018, we're willing to take this change while we're still on Aurora. At the first sign of trouble, we'll back it out though. This requires verification.

Comment 20

[Paul Feher]

I have verified this on Nightly 19.a1 (2012-10-29) using HTC Desire Z(Android 2.3.3) and Samsung Galaxy R (Android 2.3.4) the cursor can't be moved in the middle of the word to correct a misspelling it is always positioned to the end of the text in the 'status' field. I can delete all the words, but only from the end of the text.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/675572e7a097

Comment 23

[Paul Feher]

Verified on:
Build: Aurora 18.0a2 (2012-11-19)
Device: Galaxy R (2.3.4)