758427 - Galaxy Note's stylus deletes characters when trying to select a word in a text form

Status: VERIFIED FIXED

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

Description

[Chris Peterson [:cpeterson]]

STR:
1. On a Galaxy Note, load https://support.mozilla.org/en-US/mobile
2. Type some words in the text form
3. Try to select a word using the Galaxy Note's stylus

AR:
Some or all of the word's characters will be deleted.

ER:
The word should be selected.


This bug may be related to bug 750511 and/or bug 755599.

Comment 1

[Chris Peterson [:cpeterson]]

This bug only happens with the stylus.

Comment 2

[Chris Peterson [:cpeterson]]

Attachment: ignore-spurious-stylus-events.patch

Ignore spurious stylus events on Gingerbread Galaxy Note.

Comment 3

[Chris Peterson [:cpeterson]]

This problem is also mentioned on Stack Overflow:

* "Samsung Galaxy Note - Why KeyEvent.KEYCODE_FORWARD_DEL events are generated when its stylus touches/releases the screen?"
http://stackoverflow.com/questions/9882789/samsung-galaxy-note-why-keyevent-keycode-forward-del-events-are-generated-when

Comment 4

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

perhaps we should ignore all events (on all devices) where (event.getMetaState() & KeyEvent.FLAG_TRACKING) is true. If I read the docs right, that means the system is still tracking those key events for a potential long press which we'd get as a different event.

Comment 5

[Chris Peterson [:cpeterson]]

(In reply to Brad Lassey [:blassey] from comment #4)
> perhaps we should ignore all events (on all devices) where
> (event.getMetaState() & KeyEvent.FLAG_TRACKING) is true. If I read the docs
> right, that means the system is still tracking those key events for a
> potential long press which we'd get as a different event.

Filtering all FLAG_TRACKING events make sense, but it makes me a little nervous.

What if we filter all FLAG_TRACKING events in Fennec 16, but keep the Gingerbread Galaxy Note check for Fennec 14[.1]? For Fennec 15, I don't have strong feelings one way or the other.

Comment 6

[Chris Peterson [:cpeterson]]

blassey, I was mistaken. FLAG_TRACKING is a bit mask for KeyEvent.getFlag(), NOT KeyEvent.getMetaState(). The spurious stylus events on the Gingerbread Galaxy Note return getMetaState() == 512 (META_ALT_LOCKED), which is totally bogus value. KeyEvent.metaStateHasModifiers() says so here:

https://github.com/android/platform_frameworks_base/blob/6651a638348c15e89e265b0a53c775cac9beafa2/core/java/android/view/KeyEvent.java#L2040

Perhaps the safest and most generic fix would be to ignore KeyEvents whose getMetaState() has unexpected bits set.

Comment 7

[Chris Peterson [:cpeterson]]

Attachment: part-1-ignore-spurious-stylus-events.patch

Part 1: Ignore spurious key events with invalid metaState flags.

Comment 8

[Chris Peterson [:cpeterson]]

Attachment: part-2-but-only-on-Gingerbread-Galaxy-Note.patch

Part 2: Only ignore spurious key events on Gingerbread Galaxy Note.

If we want to be conservative, we can restrict Fennec 14.1 (and maybe 15) to only filter bogus KeyEvents on Gingerbread Galaxy Note.

Comment 9

[Chris Peterson [:cpeterson]]

@blassey, can you take a look at the attached patches? I would like to get this fix into Fennec 14.1 because this bug deletes text the user has already typed, which is pretty frustrating.

Comment 10

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

Comment on attachment 635829 [details] [diff] [review]
part-1-ignore-spurious-stylus-events.patch

talked to chris on irc, new patch incoming

Comment 11

[Chris Peterson [:cpeterson]]

Attachment: ignore-unknown-keycodes.patch

Ignore KeyEvents with keyCodes greater than getMaxKeyCode(). This is a more generic fix than the one we discussed in IRC, but I think it's hard to argue that we should process keyCodes > MaxKeyCode. :)

Gingerbread Galaxy Note's getMaxKeyCode() returns 110. The stylus hover events have keycode 112 (KEYCODE_FORWARD_DEL) and the stylus button events have keycode 114 (KEYCODE_CTRL_RIGHT). Those keycodes were introduced in Honeycomb, so a Gingerbread device should not be generating them.

ICS Galaxy Note's getMaxKeyCode() returns 225, which is strange because Android's ICS source code says getMaxKeyCode() is 210. After the Galaxy Note has been updated to ICS, the stylus no longer generates the problematic hover events.

https://github.com/android/platform_frameworks_base/blob/android-4.0.4_r2.1/core/java/android/view/KeyEvent.java#L595

Comment 12

[Chris Peterson [:cpeterson]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d70b14d6df33

Comment 13

[Chris Peterson [:cpeterson]]

Comment on attachment 636855 [details] [diff] [review]
ignore-unknown-keycodes.patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): OS bug on Galaxy Note
User impact if declined: Galaxy Note users can inadvertently delete typed text with stylus. This is very frustrating when typing with stylus, a feature Samsung promotes.
Testing completed (on m-c, etc.):
Risk to taking this patch (and alternatives if risky): Low risk. We are ignoring key events with invalid key codes, which would only be generated by an OS bug or undocumented hardware events (like Galaxy Note's stylus). I'd like to get this fix into Fennec 14.0.1 and 15.
String or UUID changes made by this patch:

Comment 14

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/d70b14d6df33

Comment 15

[Chris Peterson [:cpeterson]]

Brian, I'll be on PTO next week. Do you mind uplifting this patch to Aurora and Beta after its approved? I'd like to get this fix into 14.0.1.

Comment 16

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 636855 [details] [diff] [review]
ignore-unknown-keycodes.patch

[Triage comment]
Low risk fix with a high impact on usability - approving for aurora and mozilla-beta (tip of repo) so it can go out in 14.0.1

Comment 17

[Brian Nicholson (:bnicholson)]

http://hg.mozilla.org/releases/mozilla-aurora/rev/d54a44b1f5af
http://hg.mozilla.org/releases/mozilla-beta/rev/1297847a16ed