1241558 - backspace deletes text not where the cursor is

Status: VERIFIED FIXED

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

Description

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

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

Comment 1

[Mark Capella [:capella]]

dup of bug 1235632 ?

Comment 2

[James Willcox (:snorp) ([e-mail]) (he/him)]

Mark can you see if this is related to the text selection handles?

Comment 3

[Mark Capella [:capella]]

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.

Comment 4

[Mark Capella [:capella]]

cc: jchen as promised / promptly-forgot   :-/

Comment 5

[Mark Capella [:capella]]

Attachment: 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.

Comment 6

[Kartikaya Gupta (email:[e-mail])]

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) {...}

Comment 7

[Mark Capella [:capella]]

Comment on attachment 8710985 [details] [diff] [review]
bug1241558.diff

Cancelling r? pending kats' tweak ... & digging a little deeper

Comment 9

[Mark Capella [:capella]]

workaround: For Google and stock Android (AOSP) keyboards, under "Text correction", disable "Show correction suggestions".

Comment 10

[Mark Capella [:capella]]

Attachment: 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

Comment 11

[Mark Capella [:capella]]

regression (local builds before/after) points here:

https://hg.mozilla.org/mozilla-central/rev/8ed62b03205f
https://hg.mozilla.org/releases/mozilla-aurora/rev/3a919bac5d72

Comment 12

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

Attachment: 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.

Comment 13

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

Attachment: Add test to testInputConnection (v1)

Comment 14

[Mark Capella [:capella]]

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?

Comment 15

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

It corrects a subset of the problem (the case added to the testcase), but I see there are other cases to consider.

Comment 16

[Mark Capella [:capella]]

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 :-)

Comment 17

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

Attachment: Don't ignore Gecko selection when Gecko commits composition (v2)

This should make backspacing better. :capella, can you try it out?

Comment 18

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

Attachment: Add test to testInputConnection (v2)

Updated test with an additional test case.

Comment 19

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

Attachment: 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.

Comment 20

[Geoff Brown [: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...

Comment 21

[Mark Capella [:capella]]

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!

Comment 22

[Eugen Sawin [:esawin]]

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?

Comment 23

[Pulsebot]

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

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5ff7ebae71b2
https://hg.mozilla.org/mozilla-central/rev/1456f80ef613
https://hg.mozilla.org/mozilla-central/rev/f2dfb3c8fd45

Comment 25

[Daniel Holbert [:dholbert]]

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...

Comment 26

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

I guess I had assumed this only affected builds with APZ and/or accessible carets enabled (i.e. nightly builds). We can uplift.

Comment 27

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

Attachment: 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

Comment 28

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

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

Comment 29

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8724149 [details] [diff] [review]
Patch for Beta

Sorry but this is too late for 45.

Comment 30

[Daniel Holbert [:dholbert]]

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.

Comment 31

[Daniel Holbert [:dholbert]]

(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?)

Comment 32

[Daniel Holbert [:dholbert]]

Here's a screencast of this bug in action, as a demonstration of how easy it is to hit & how annoying it is.

Comment 33

[Daniel Holbert [:dholbert]]

https://www.youtube.com/watch?v=ScKvDSILxiI

Comment 35

[Daniel Holbert [:dholbert]]

(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 36

[Sylvestre Ledru [:Sylvestre]]

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

Comment 37

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-beta/rev/81807bf0d732
https://hg.mozilla.org/releases/mozilla-release/rev/88e99ea9f356

Comment 38

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

(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.

Comment 39

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 40

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

Attachment: Patch for Beta (v2)

Sorry, here's an updated patch that builds.

Comment 41

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/906fc87ab81a 
https://hg.mozilla.org/releases/mozilla-release/rev/ed274004fe1f

Comment 42

[u421692]

Verified as fixed on Firefox 45 Beta 11 and on latest Nightly

Comment 43

[u421692]

Verified as fixed on Firefox 45 RC

Comment 44

[Liz Henry (:lizzard) (relman/hg->git project)]

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?

Comment 45

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c2b983ebe137