1406247 - event.target.selectionStart reports incorrect cursor position

Status: RESOLVED FIXED

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

Description

[dwbruhn]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_6) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/61.0.3163.100 Safari/537.36

Steps to reproduce:

All the necessary information is available here:

https://github.com/dwbruhn/android-firefox-cursor

Summary: Upon firing of an keypress event, the reported cursor position is wrong in a particular situation (described in the README).

Note that this affects both Android Firefox 56.0 and 57.0b5.

I'm certainly open to this being some mobile vs. desktop difference that I've missed!


Actual results:

Cursor position (event.target.selectionStart) was reported incorrectly.


Expected results:

Cursor position (event.target.selectionStart) should have indicated the end of the input.

Comment 1

[dwbruhn]

Hi, I was able to confirm on my device (Nexus 5X) that the bug was introduced in Firefox 52.0:

51.0 (http://ftp.mozilla.org/pub/mobile/releases/51.0/android-api-15/en-US/) shows the expected behavior.

52.0 (http://ftp.mozilla.org/pub/mobile/releases/52.0/android-api-15/en-US/) shows the bug.

Interestingly, Android Firefox Focus (2.1) doesn't have the bug.

Comment 2

[Zibi Braniecki [:zbraniecki][:gandalf]]

Hi :dwbruhn! Thank you for reporting the issue.

The triage owner for this component is currently on PTO. I'll see if I can find someone to step in, but in the worst case, I'd expect :jchen to chime in when he's back on Oct 23rd.

Comment 3

[dwbruhn]

:zibi: Thanks so much!

Comment 4

[Jan Henning [:JanH]]

Thanks for the detailed test case.

A tip for the future: Since you've already narrowed it down to Firefox 52, note that with mozregression (http://mozilla.github.io/mozregression/) we have a tool that allows you to easily narrow down further the exact revision a regression was introduced. If you can run that and pinpoint the (potential) change that caused a bug this is always appreciated.

In this case, I've done a mozregression run with your test case and it seems that before bug 1307816 everything was fine, after  that bug pressing "6" crashes the browser and a few days later after bug 1123514 was fixed I'm seeing the behaviour you describe. So it was probably one or the other of those two that caused this.

Comment 5

[Zibi Braniecki [:zbraniecki][:gandalf]]

Since both patches have been authored by :jchen who's on PTO, I'm wondering if :esawin who reviewed them can chime in.

Eugen - do you think this should wait for :jchen to be back?

Comment 6

[Eugen Sawin [:esawin]]

Thank you Daniel for the extensive bug report.

It looks like the replacement range (bug 1123514) is off in this case, but Jim will know best how to fix this without regressing other cases.

Comment 7

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

I'll take a look.

Comment 8

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

Attachment: 1. Don't ignore selection change when Gecko text change is more than expected (v1)

When a Gecko text change covers more than just our expected change on
the Java side, don't ignore the subsequent selection change notification
because the Gecko selection could have moved anywhere.

Comment 9

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

Attachment: 2. Fix testInputConnection to have correct check (v1)

testInputConnection had a wrong check for selection offset, which would
otherwise have caught this bug.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/76f9a0835dd7
1. Don't ignore selection change when Gecko text change is more than expected; r=esawin
https://hg.mozilla.org/integration/mozilla-inbound/rev/8c8844a2e3ca
2. Fix testInputConnection to have correct check; r=esawin

Comment 11

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/76f9a0835dd7
https://hg.mozilla.org/mozilla-central/rev/8c8844a2e3ca

Comment 12

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

Comment on attachment 8922567 [details] [diff] [review]
1. Don't ignore selection change when Gecko text change is more than expected (v1)

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1307816
[User impact if declined]: Wrong behavior in the given test case
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: Small change in infrequent code path to correct wrong behavior.
[String changes made/needed]: None

Comment 13

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/76f9a0835dd7
https://hg.mozilla.org/mozilla-central/rev/8c8844a2e3ca

Comment 14

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8922567 [details] [diff] [review]
1. Don't ignore selection change when Gecko text change is more than expected (v1)

I fail to understand the end user impact. Could you please explain?

Comment 15

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

This is not a new regression, been a problem since 52, I'd rather let it ride the 58 train. Sorry!

Comment 16

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

Okay, that's understandable. User impact should be limited to sites with scripts that are similar to the test case, which I don't think is common.

Comment 17

[dwbruhn]

It's your call, and I'm grateful to see it fixed in any case. But this bug is actually affecting the popular react-maskedinput package (https://github.com/insin/react-maskedinput). Try the phone entry form in the PayPal signup flow (US, personal account) to see the bug in all its glory. :-D