Closed Bug 792928 Opened 7 years ago Closed 7 years ago

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

Categories

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

16 Branch
ARM
Android
defect
Not set
major

Tracking

()

VERIFIED FIXED
Firefox 19
Tracking Status
firefox18 --- verified
firefox19 --- verified
fennec 18+ ---

People

(Reporter: krudnitski, Assigned: jchen)

References

Details

(Whiteboard: [SUMO])

Attachments

(5 files)

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
Attached image 2 - back spacing
Attached image 3 - deleted text
Component: General → Keyboards and IME
tracking-fennec: --- → ?
Assignee: nobody → gpascutto
tracking-fennec: ? → 18+
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.
Status: NEW → ASSIGNED
Target Milestone: Firefox 17 → ---
Duplicate of this bug: 799206
Whiteboard: [SUMO]
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.
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.
Assignee: gpascutto → nchen
Attachment #672511 - Flags: review?(masayuki)
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/nchen@mozilla.com-deb49bc0d7e9
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.
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/nchen@mozilla.com-deb49bc0d7e9
Attached image 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 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);
   }
Attachment #672511 - Flags: review?(masayuki) → review+
https://hg.mozilla.org/mozilla-central/rev/c395a0974676
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 19
Can this be uplifted as it's a very annoying bug that affects every version.
(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.
Duplicate of this bug: 806179
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
Attachment #672511 - Flags: approval-mozilla-aurora?
Duplicate of this bug: 806331
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.
Keywords: qawanted, verifyme
QA Contact: aaron.train
Attachment #672511 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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.
Status: RESOLVED → VERIFIED
Keywords: qawanted, verifyme
Verified on:
Build: Aurora 18.0a2 (2012-11-19)
Device: Galaxy R (2.3.4)
Duplicate of this bug: 689096
You need to log in before you can comment on or make changes to this bug.