1344464 - locationbar/navigationbar: Tapping backspace deletes suggested part of url, second backspace deletes two characters or re-adds it [xperia keyboard]

Status: VERIFIED FIXED

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

Description

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

Firefox for Android 54.0a1 on Android 6.0.1 (Sony Xperia Z3 Compact)

Tapping backspace after having typed something in the location bar deletes the suggested part of the url (suggestion sometimes doesn't show up on first try), a second tap on backspace will add it again, so you can't fix typos.

It should delete the previous character.

This is a regression between 2017027 and 20170228. Commits: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=cc0d34f0490c9dab0fda35765683c31d3a4fc950&tochange=1bc2ad020aee2830e0a7941f10958dbec108c254

The behavior before was also erroneous: The second backspace deleted everything typed from the location bar.

Comment 1

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

Ok, the issue here seems to be that Firefox wants to delete two characters after the second backspace. If there is only character in the location bar, this fails and the autofill applies again.

Comment 2

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

Latest Firefox for Android 54.0a1 on Android 6.0.1 (Sony Xperia Z3 Compact)

Tapping backspace after having typed one letter in the location bar deletes the suggested part of the url (suggestion sometimes doesn't show up on first try), a second tap on backspace will add it again, so you can't fix typos. If there are two or more characters in the location bar, two will be deleted on that second backspace.

It should delete the previous character only that one.

This is a regression between 20161130 and 20161201. Commits: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=13736e2db6eb94b02dd28cc88f2943b8109aa374&tochange=cd4cdcc9ad6c45dad8b8d8c0d40e459db2bca8a1

Regression from bug 1320449 or bug 1320603?

Comment 3

[Jan Henning [:JanH]]

Which keyboard are you using?

Our options probably are:
- find some workaround
- blacklist the affected IME
- entirely disable input suggestions again

Comment 4

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

> Which keyboard are you using?
The default one. Translated from German: "International Keyboard - Xperia(TM) Keyboard" (the available keyboards are either Xperia ones or Voice input).

Comment 5

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

Works as expected with latest Nightly on a Nexus S with Android 4.1.2

Comment 6

[Jan Henning [:JanH]]

I've obtained the Xperia keyboard from XDA (https://forum.xda-developers.com/android/apps-games/app-sony-xperia-keyboard-phones-t3346736) and I can indeed reproduce it.

It seems that the relevant bit of autocomplete logic works by intercepting setComposingText (https://dxr.mozilla.org/mozilla-central/rev/80c06df83395314697d464f88f8daa98bf05465c/mobile/android/base/java/org/mozilla/gecko/toolbar/ToolbarEditText.java#481).

Let's assume we've typed wiki, which autocompleted to wiki<pedia.org>.
When pressing backspace, setComposingText gets called with "wik" for the text parameter. We detect that the composing text is getting shorter, remove the autocomplete and then return *false* to prevent the keyboard's changes from having any actual effect on the URL bar.
Other keyboards (okay, at least Swiftkey - I don't have that many keyboards installed and the Google keyboard doesn't offer input suggestions in URL input mode and uses deleteSurroundingText instead) handle this fine, so the next time backspace is pressed they're calling setComposingText("wik") again.

The Xperia keyboard however seems to ignore setComposingText returning false and assumes it successfully deleted that letter, so the next time the user presses backspace, it sends setComposingText("wi"), thereby apparently deleting two letters in one go.
If there was just one user-typed letter, it oscillates between "" and "w" and causes the behaviour you've observed above.

As a workaround, it seems that restarting the input before returning false from setComposingText() works fine.

Comment 7

[Jan Henning [:JanH]]

Attachment: Bug 1344464 - Part 0 - Remove unneeded code.

Review commit: https://reviewboard.mozilla.org/r/117252/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/117252/

Comment 8

[Jan Henning [:JanH]]

Attachment: Bug 1344464 - Part 1 - Restart input after removing autocomplete text for keyboards that require it.

When we detect text being deleted from the URL bar via setComposingText while we're displaying an autocomplete suggestion, we clear the autocomplete text and prevent the keyboard's delete from having any effect on the URL bar by returning false.

Some keyboards might not handle this correctly and assume that they've in fact successfully set the new text, so the next time the user presses a key can lead to weird behaviour. As a workaround, we therefore additionally restart the input for affected keyboards.

Review commit: https://reviewboard.mozilla.org/r/117254/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/117254/

Comment 9

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

Comment on attachment 8843631 [details]
Bug 1344464 - Part 0 - Remove unneeded code.

https://reviewboard.mozilla.org/r/117252/#review119308

Comment 10

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

Comment on attachment 8843632 [details]
Bug 1344464 - Part 1 - Restart input after removing autocomplete text for keyboards that require it.

https://reviewboard.mozilla.org/r/117254/#review119310

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/08e4e7a7af7d
Part 0 - Remove unneeded code. r=jchen
https://hg.mozilla.org/integration/autoland/rev/960852d0618a
Part 1 - Restart input after removing autocomplete text for keyboards that require it. r=jchen

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/08e4e7a7af7d
https://hg.mozilla.org/mozilla-central/rev/960852d0618a

Comment 13

[Jan Henning [:JanH]]

Can you verify that this is working as expected and doesn't have any other unwanted side effects?

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Please request Aurora/Beta approval on this when you're ready to do so.

Comment 15

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

Works now as expected with 55.0a1 20170308 on the Xperia Z3C.

Comment 16

[Jan Henning [:JanH]]

Comment on attachment 8843631 [details]
Bug 1344464 - Part 0 - Remove unneeded code.

see Part 1 for rest
[Is the change risky?]: No.
[Why is the change risky/not risky?]: Removed imports in ToolbarEditText.java aren't necessary in Beta codebase either and InputMethods.isGestureKeyboard() was likewise already disused at that time.

Comment 17

[Jan Henning [:JanH]]

Comment on attachment 8843632 [details]
Bug 1344464 - Part 1 - Restart input after removing autocomplete text for keyboards that require it.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1320449
[User impact if declined]: Strange behaviour when deleting text on the URL bar in conjunction with URL autocompletion.
[Is this code covered by automated tests?]: No.
[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]: Part 0 would be nice, although not absolutely necessary.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: Restarting input is already used as a workaround to force the keyboard noticing our actions in similar circumstances. Additionally, this code is restricted to a specific keyboard app only.
[String changes made/needed]: none

Comment 18

[Gerry Chang [:gchang]]

Comment on attachment 8843631 [details]
Bug 1344464 - Part 0 - Remove unneeded code.

Fix a deleting text on the URL bar issue with URL autocompletion and was verified. Aurora54+.

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/eff06436e4bb
https://hg.mozilla.org/releases/mozilla-aurora/rev/679c2e708824

Comment 20

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

Comment on attachment 8843632 [details]
Bug 1344464 - Part 1 - Restart input after removing autocomplete text for keyboards that require it.

Recent regression in 53, fix has been verified on 54, Beta53+

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/27b7ba43c5af
https://hg.mozilla.org/releases/mozilla-beta/rev/72774ba28bbb

Comment 22

[Sorina Florean [:sflorean]]

Hi, 

This bug is reproducible with Huawei MediaPad M2 (Android 5.1.1) and keyboard Swype- Huawei Swype on all branches, 52 - 55. I type wiki, one backspace deleted "pedia.org", and the second backspace deleted "ki".
Same for Samsung Galaxy Note 4 (Android 5.0.1) with SWYPE keyboard.
With Android keyboard (AOSP) this issue is not reproducible.

Comment 23

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

Please create a new bug for this issue. That is a different issue because it already affects Firefox 52.

Comment 24

[Sorina Florean [:sflorean]]

Filed Bug 1348799 - Second backspace is deleting two characters instead of one.