If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

backspace deletes text not where the cursor is

VERIFIED FIXED in Firefox 45

Status

()

Firefox for Android
Keyboards and IME
VERIFIED FIXED
2 years ago
a year ago

People

(Reporter: blassey, Assigned: jchen)

Tracking

({regression})

45 Branch
Firefox 47
regression
Points:
---

Firefox Tracking Flags

(firefox45 verified, firefox46 fixed, firefox47 verified, fennec45+)

Details

(URL)

Attachments

(5 attachments, 4 obsolete attachments)

STR:
1) On nightly, click the "Report Site Issue" menu item (which will open webcompat.com)
2) In the "give more details" text box, touch next to the "2)" and type some text
3) touch at the bottom of the text field (after "Actual Behavor:") to place cursor there
4) hit backspace

ER: "Actual Behavior" text should be deleted
AR: The text you typed in step 2 gets deleted
dup of bug 1235632 ?
Mark can you see if this is related to the text selection handles?
tracking-fennec: ? → 47+
Flags: needinfo?(markcapella)
snorp: sure.

I came in here with bug 1235632, which was (most likely mis-)labelled a Text Selection component, and is most likely a dup here.

I believe this will wind up properly being an IME-keyboard component / issue. The selection handles (afaik) only reflect selection state, vs text content state.

I started poking earlier down a fruitless train of thought centered on clearing the composition event state at point of single-tap re-positioning of the cursor, and haven't looped back yet.

I'll cc: jchen to be sure it's on his radar, though I've been assuming he monitors this component.
cc: jchen as promised / promptly-forgot   :-/
Flags: needinfo?(markcapella)
Created attachment 8710985 [details] [diff] [review]
bug1241558.diff

This seems to correct this bug, as well as bug 1235632.
(see https://bugzilla.mozilla.org/show_bug.cgi?id=1235632#c2 for more description).

The common issue here is |single:tap (step 3: touch at the bottom)| conflict with ongoing compositions.

Initially I'd tried issuing a REQUEST_TO_COMMIT_COMPOSITION event at the appropriate place, a solution we employed in Bug 1082425, but that has no effect in this case.

The (personally abhorred) fallback blur/focus solution works fine.

request review from kats for code location, from jchen for correctness / alternative thoughts.
Attachment #8710985 - Flags: review?(nchen)
Attachment #8710985 - Flags: review?(bugmail.mozilla)
Comment on attachment 8710985 [details] [diff] [review]
bug1241558.diff

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

::: widget/android/AndroidContentController.cpp
@@ +63,5 @@
> +
> +            // Commit the composition string of the old editable focus element
> +            // (if there is any) before tapping (via mouse events) changes the focus.
> +            // Note: REQUEST_TO_COMMIT_COMPOSITION alternative is not useful here.
> +            nsIDOMWindow* win = shell->GetDocument()->GetWindow();

This should go outside the (shell && shell->ScaleToResolution()) condition. You probably want a new condition with just if (shell) {...}
Attachment #8710985 - Flags: review?(bugmail.mozilla)
Comment on attachment 8710985 [details] [diff] [review]
bug1241558.diff

Cancelling r? pending kats' tweak ... & digging a little deeper
Attachment #8710985 - Flags: review?(nchen)
Duplicate of this bug: 1235632
workaround: For Google and stock Android (AOSP) keyboards, under "Text correction", disable "Show correction suggestions".
Created attachment 8714469 [details] [diff] [review]
bug1241558.diff

Actually, using [0] as the testcase, Google Keyboard with "Show correction suggestions", and reduced STR:

  ) tap into text field
  ) observe composition underline / styling
  ) tap backspace
  ) observe char delete and styling removal

We see baseline Chrome behaviour [1] matches correct behaviour in release-v44 [2]. Regression behaviour starts in beta-v45 [3] with incorrect caret positioning after backspace, and seemingly gets worse in nightly-v47 [4].

Still digging, but my ugly-hack seems to fix the bit that regresses between v45-v47 (gets us back to beta), but not the basic problem that occurred from v44-v45. :-/


[0] https://www.dropbox.com/s/mzzgw1i4uav925y/test_bug1241558_backspace.html?dl=0

[1] https://www.dropbox.com/s/oku5c1cay2pnvmp/bug1241558_backSpace_CHROME.mp4?dl=0
[2] https://www.dropbox.com/s/vwfjh21xxgpyifq/bug1241558_backSpace_v44.mp4?dl=0

[3] https://www.dropbox.com/s/inhws4f0b2t13pw/bug1241558_backSpace_v45.mp4?dl=0
[4] https://www.dropbox.com/s/k2i03f6mil6hzvy/bug1241558_backSpace_v47.mp4?dl=0
Attachment #8710985 - Attachment is obsolete: true
regression (local builds before/after) points here:

https://hg.mozilla.org/mozilla-central/rev/8ed62b03205f
https://hg.mozilla.org/releases/mozilla-aurora/rev/3a919bac5d72
Flags: needinfo?(nchen)
status-firefox45: --- → affected
status-firefox46: --- → affected
status-firefox47: --- → affected
(Assignee)

Updated

2 years ago
Assignee: nobody → nchen
Status: NEW → ASSIGNED
Flags: needinfo?(nchen)
(Assignee)

Updated

2 years ago
Blocks: 1236705
Keywords: regression
(Assignee)

Comment 12

2 years ago
Created attachment 8715525 [details] [diff] [review]
Don't ignore Gecko selection when Gecko commits composition (v1)

When Java is changing the composition, we should ignore the Gecko
selection. However, when Gecko is committing its composition, we should
not be ignoring the corresponding Gecko selection change. In other
words, we should only ignore selection changes when we know the change
is from Java.
Attachment #8715525 - Flags: review?(esawin)
(Assignee)

Comment 13

2 years ago
Created attachment 8715526 [details] [diff] [review]
Add test to testInputConnection (v1)
Attachment #8715526 - Flags: review?(esawin)
drive by: I've applied and tested the patch in comment #12 and don't see corrected behaviour that I expected. Is that the right patch, or have I mis-understood something?
Flags: needinfo?(nchen)
(Assignee)

Comment 15

2 years ago
It corrects a subset of the problem (the case added to the testcase), but I see there are other cases to consider.
Flags: needinfo?(nchen)
Comment on attachment 8715525 [details] [diff] [review]
Don't ignore Gecko selection when Gecko commits composition (v1)

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

Also, moar tweaks like AutoIMESynchronize and TYPE_UPDATE_COMPOSITION ftw \o/

::: mobile/android/base/java/org/mozilla/gecko/GeckoEditable.java
@@ +1097,5 @@
>              }
>  
>              // Ignore the next selection change because the selection change is a
>              // side-effect of the replace-text event we sent.
>              mIgnoreSelectionChange = true;

Right, your provisional ignore of "next-selection" as provided doesn't address where the text length changes on backspace. Ok, thought I was losing it :-)
(Assignee)

Comment 17

2 years ago
Created attachment 8717081 [details] [diff] [review]
Don't ignore Gecko selection when Gecko commits composition (v2)

This should make backspacing better. :capella, can you try it out?
Attachment #8717081 - Flags: review?(esawin)
Attachment #8717081 - Flags: feedback?(markcapella)
(Assignee)

Updated

2 years ago
Attachment #8715525 - Attachment is obsolete: true
Attachment #8715525 - Flags: review?(esawin)
(Assignee)

Comment 18

2 years ago
Created attachment 8717082 [details] [diff] [review]
Add test to testInputConnection (v2)

Updated test with an additional test case.
Attachment #8717082 - Flags: review?(esawin)
(Assignee)

Updated

2 years ago
Attachment #8715526 - Attachment is obsolete: true
Attachment #8715526 - Flags: review?(esawin)
(Assignee)

Comment 19

2 years ago
Created attachment 8717125 [details] [diff] [review]
Re-enable testInputConnection on 4.3 (v1)

Geoff, do you remember why testInputConnection was disabled on 4.3? It's not
bug 1025968 because AFAICT that's only for 2.3. The test doesn't get run at all
when we disable it for both 2.3 and 4.3, and I'd rather have it running on 4.3.
I did some try runs and it seems to be pretty reliable.
Attachment #8717125 - Flags: review?(gbrown)
Comment on attachment 8717125 [details] [diff] [review]
Re-enable testInputConnection on 4.3 (v1)

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

I don't recall and did not update the comment properly - sorry. 

As long as it's working now...
Attachment #8717125 - Flags: review?(gbrown) → review+
Comment on attachment 8717081 [details] [diff] [review]
Don't ignore Gecko selection when Gecko commits composition (v2)

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

Nice! I pecked around with Google and SwiftKey kbds, autoCorrect on/off, autoInsertPredication, tapping various places, backspacing, typing, etc. Everything looks great!
Attachment #8717081 - Flags: feedback?(markcapella) → feedback+
Comment on attachment 8717081 [details] [diff] [review]
Don't ignore Gecko selection when Gecko commits composition (v2)

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

::: mobile/android/base/java/org/mozilla/gecko/GeckoEditable.java
@@ +1116,5 @@
> +            // Nothing to do because the text is the same. This could happen when
> +            // the composition is updated for example, in which case we want to keep the
> +            // Java selection.
> +            mIgnoreSelectionChange |= (action != null &&
> +                    action.mType == Action.TYPE_UPDATE_COMPOSITION);

Although, this is correct, it's misleading (using bitwise OR instead of logical OR), can you write it out instead?

::: widget/android/nsWindow.cpp
@@ +355,5 @@
> +    public:
> +        AutoIMESynchronize(GeckoViewSupport* gvs) : mGVS(gvs) {}
> +        ~AutoIMESynchronize() { mGVS->OnImeSynchronize(); }
> +    };
> +

This is nice. Can you add a comment describing why we need it?
Attachment #8717081 - Flags: review?(esawin) → review+

Updated

2 years ago
Attachment #8717082 - Flags: review?(esawin) → review+

Comment 23

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/5ff7ebae71b2
https://hg.mozilla.org/integration/mozilla-inbound/rev/1456f80ef613
https://hg.mozilla.org/integration/mozilla-inbound/rev/f2dfb3c8fd45

Comment 24

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/5ff7ebae71b2
https://hg.mozilla.org/mozilla-central/rev/1456f80ef613
https://hg.mozilla.org/mozilla-central/rev/f2dfb3c8fd45
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox47: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
jchen, looks like we're on-track to ship this regression in 45 and 46.  Is the fix suitable for uplifting?

Or if not, should we back out the regressing bug 1236705 (identified in comment 11 here)?

This bug makes text-editing unpredictable & really annoying... and backspacing to fix typos is a common use-case on mobile I expect.  (it is for me at least).  So, it'd be great if we could avoid shipping this regression in a release...
Flags: needinfo?(nchen)
(Assignee)

Comment 26

2 years ago
I guess I had assumed this only affected builds with APZ and/or accessible carets enabled (i.e. nightly builds). We can uplift.
Flags: needinfo?(nchen)
(Assignee)

Comment 27

2 years ago
Created attachment 8724149 [details] [diff] [review]
Patch for Beta

Approval Request Comment
[Feature/regressing bug #]: Bug 1236705
[User impact if declined]: Bad experience when inputting text
[Describe test coverage new/current, TreeHerder]: Locally, m-c
[Risks and why]: Some risk. Patch introduces new code but the code is tested.
[String/UUID change made/needed]: None
Attachment #8724149 - Flags: review+
Attachment #8724149 - Flags: approval-mozilla-beta?
(Assignee)

Comment 28

2 years ago
Comment on attachment 8717081 [details] [diff] [review]
Don't ignore Gecko selection when Gecko commits composition (v2)

Aurora request for this patch only.

Approval Request Comment
[Feature/regressing bug #]: Bug 1236705
[User impact if declined]: Bad experience when inputting text
[Describe test coverage new/current, TreeHerder]: Locally, m-c
[Risks and why]: Some risk. Patch introduces new code but the code is tested.
[String/UUID change made/needed]: None
Attachment #8717081 - Flags: approval-mozilla-aurora?
Comment on attachment 8724149 [details] [diff] [review]
Patch for Beta

Sorry but this is too late for 45.
Attachment #8724149 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
status-firefox45: affected → wontfix
This bug is sufficiently-annoying from a usability perspective that I'm worried it might push users away. Any way we could reconsider (or take it in a Fennec dot release)?  With our new concept of empowering ourselves to press a button to stop or delay a release, I think this bug is a case where we should strongly consider doing that.

This manifests pretty much any time someone tries to edit text that they've just typed into a web form (e.g. a search engine's textfield, or a username/password field, or a comment box). If they make the fatal mistake of pressing the backspace key, their text-entry field becomes unpredictable & unusable from that point on.
Flags: needinfo?(sledru)
(jchen: if we get beta backport approval, are you comfortable with this bug's patch going pretty much directly to release users with very little time for testing/additional-fixes, given that current-beta is shipping as release next week?)
Flags: needinfo?(nchen)
Here's a screencast of this bug in action, as a demonstration of how easy it is to hit & how annoying it is.
https://www.youtube.com/watch?v=ScKvDSILxiI
Duplicate of this bug: 1251509
(Another screencast, showing that the same bug affects <textarea>:
 https://www.youtube.com/watch?v=-KS3L9G3rYU
Both this screencast & the earlier one are taken with Firefox Beta on Android.)
Comment on attachment 8724149 [details] [diff] [review]
Patch for Beta

After discussing with a bunch of people, clearly, we need it for fennec beta 11
Flags: needinfo?(sledru)
Attachment #8724149 - Flags: approval-mozilla-beta- → approval-mozilla-beta+
https://hg.mozilla.org/releases/mozilla-beta/rev/81807bf0d732
https://hg.mozilla.org/releases/mozilla-release/rev/88e99ea9f356
status-firefox45: wontfix → fixed
(Assignee)

Comment 38

2 years ago
(In reply to Daniel Holbert [:dholbert] from comment #31)
> (jchen: if we get beta backport approval, are you comfortable with this
> bug's patch going pretty much directly to release users with very little
> time for testing/additional-fixes, given that current-beta is shipping as
> release next week?)

I would say so based on the couple weeks it spent on m-c.
(Assignee)

Updated

2 years ago
Flags: needinfo?(nchen)
This caused failures on beta and release: https://treeherder.mozilla.org/logviewer.html#?job_id=858366&repo=mozilla-beta

I'm going to back it out for now on both so the RC candidate can get built.

https://hg.mozilla.org/releases/mozilla-beta/rev/29a9c826963a
https://hg.mozilla.org/releases/mozilla-release/rev/d57d26c516f1
status-firefox45: fixed → affected
Flags: needinfo?(nchen)
(Assignee)

Comment 40

2 years ago
Created attachment 8725101 [details] [diff] [review]
Patch for Beta (v2)

Sorry, here's an updated patch that builds.
Attachment #8724149 - Attachment is obsolete: true
Flags: needinfo?(nchen)
Attachment #8725101 - Flags: review+
https://hg.mozilla.org/releases/mozilla-beta/rev/906fc87ab81a 
https://hg.mozilla.org/releases/mozilla-release/rev/ed274004fe1f
status-firefox45: affected → fixed

Comment 42

2 years ago
Verified as fixed on Firefox 45 Beta 11 and on latest Nightly
Status: RESOLVED → VERIFIED
status-firefox45: fixed → verified
status-firefox47: fixed → verified

Comment 43

2 years ago
Verified as fixed on Firefox 45 RC
tracking-fennec: 47+ → 45+
Comment on attachment 8717081 [details] [diff] [review]
Don't ignore Gecko selection when Gecko commits composition (v2)

Approved for aurora uplift. But is this already on 46?
Attachment #8717081 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Comment 45

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/c2b983ebe137
status-firefox46: affected → fixed
Version: unspecified → 45 Branch
You need to log in before you can comment on or make changes to this bug.