Closed Bug 758427 Opened 8 years ago Closed 8 years ago

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

Categories

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

ARM
Android
defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 16
Tracking Status
firefox14 --- fixed
firefox15 --- verified
firefox16 --- verified
firefox17 --- verified
blocking-fennec1.0 --- soft

People

(Reporter: cpeterson, Assigned: bnicholson)

References

()

Details

Attachments

(1 file, 3 obsolete files)

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.
blocking-fennec1.0: --- → ?
blocking-fennec1.0: ? → soft
This bug only happens with the stylus.
Ignore spurious stylus events on Gingerbread Galaxy Note.
Assignee: nobody → cpeterson
Status: NEW → ASSIGNED
Attachment #635106 - Flags: review?(blassey.bugs)
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
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.
(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.
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.
Part 1: Ignore spurious key events with invalid metaState flags.
Attachment #635106 - Attachment is obsolete: true
Attachment #635106 - Flags: review?(blassey.bugs)
Attachment #635829 - Flags: review?(blassey.bugs)
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.
Attachment #635830 - Flags: review?(blassey.bugs)
@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 on attachment 635829 [details] [diff] [review]
part-1-ignore-spurious-stylus-events.patch

talked to chris on irc, new patch incoming
Attachment #635829 - Flags: review?(blassey.bugs) → review-
Attachment #635830 - Flags: review?(blassey.bugs) → review-
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
Attachment #635829 - Attachment is obsolete: true
Attachment #635830 - Attachment is obsolete: true
Attachment #636855 - Flags: review?(blassey.bugs)
Attachment #636855 - Flags: review?(blassey.bugs) → review+
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:
Attachment #636855 - Flags: approval-mozilla-beta?
Attachment #636855 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/d70b14d6df33
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 16
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.
Assignee: cpeterson → bnicholson
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
Attachment #636855 - Flags: approval-mozilla-beta?
Attachment #636855 - Flags: approval-mozilla-beta+
Attachment #636855 - Flags: approval-mozilla-aurora?
Attachment #636855 - Flags: approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.