All users were logged out of Bugzilla on October 13th, 2018

Improve Lockscreen Inputpad's touch and visual feeback

RESOLVED FIXED in Firefox OS master

Status

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: cr, Assigned: cr)

Tracking

({feature})

unspecified
FxOS-S7 (18Sep)
ARM
Gonk (Firefox OS)
feature

Firefox Tracking Flags

(b2g-master fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

3 years ago
Inputpad looks and feels wonky compared to the slick lock and feel of
the keyboard panel used while setting the pass code in Settings. Much of
the reason comes from that it only vibrates on key release, the other
that it does not behave well when the user is sliding a finger over the
keys. There is also a considerable delay between touching a key and a
visual reaction to the extent that it's skipped when typing mildly fast.
Created attachment 8639355 [details] [review]
[gaia] cr:lockscreentouch > mozilla-b2g:master
(Assignee)

Comment 2

3 years ago
The prototype patch addresses all of the above:

- make InputPad vibrate instantly on touch
- make InputPad respect user's touchmove
- visualize key presses instantly

It currently breaks the vibration test, but it's only meant to start discussion anyway.

Let me know what you think.
Flags: needinfo?(firefoxos-ux-bugzilla)
(Assignee)

Updated

3 years ago
Summary: Improve Inputpad's touch and visual feeback → Improve Lockscreen Inputpad's touch and visual feeback
(Assignee)

Comment 3

3 years ago
The visualization delay appears to be scaling according to hardware performance. While really irritating on the Flame, it's barely noticeable on the much more powerful foxfood device.
Comment on attachment 8639355 [details] [review]
[gaia] cr:lockscreentouch > mozilla-b2g:master

Adding ui-review for Harly (interaction) and Patryk (visual).

Thanks for pinging the UX team!
Attachment #8639355 - Flags: ui-review?(padamczyk)
Attachment #8639355 - Flags: ui-review?(hhsu)
oops forgot to remove our flag.
Flags: needinfo?(firefoxos-ux-bugzilla)

Comment 6

3 years ago
Hi Christiane, 
After taking a look at the patch, I can see that you have made the change of having the phone vibrate immediately instead of after release when user presses the keypad. However, I don't really feel a difference for "make InputPad respect user's touchmove" and "visualize key presses instantly", could you be more specific as what does these 2 thing do? 

Also, it will be great if we can add a small feature which is when user long presses the back key, system will delete all the entered pass code like what we have in the pass code settings. Thanks!
Flags: needinfo?(cr)
(Assignee)

Comment 7

3 years ago
Hi Harly,

 for me it makes a difference on which hardware I test. Generally the "wonkiness" feels stronger on the Flame than on the beefier Foxfood device. The visualization delay could be detected on the Flame only. There was a ~200ms delay between touching a key and the key being visualized as "active".

For the touch-slide issue, touch your finger down on a key, slide around and release over a different key. I expect to see the "active" key move around with my finger, and that I input the key over which I release just like the normal keyboard does.

I tested this on several devices and b2g versions, so I am not able to exactly reproduce from memory, but what I observed were issues like:

- "active" key not following finger movement, stuck on the first key
- no key or wrong key being input when sliding to a different key

Does that make it clearer?

I like the idea of the "clear pin" feature and it is easy to implement. What's the regular delay for holding keys?
Flags: needinfo?(cr) → needinfo?(hhsu)

Comment 8

3 years ago
Comment on attachment 8639355 [details] [review]
[gaia] cr:lockscreentouch > mozilla-b2g:master

Thanks for clarifying, I think the patch has all of the things you've mentioned and it is working nicely. About the long press back key, could you reference the code from the passcode settings, I don't really know the time delay for long press.
Flags: needinfo?(hhsu)
Attachment #8639355 - Flags: ui-review?(hhsu) → ui-review+
Comment on attachment 8639355 [details] [review]
[gaia] cr:lockscreentouch > mozilla-b2g:master

Works well.
Attachment #8639355 - Flags: ui-review?(padamczyk) → ui-review+
(Assignee)

Comment 10

3 years ago
Comment on attachment 8639355 [details] [review]
[gaia] cr:lockscreentouch > mozilla-b2g:master

Greg, it looks like UX likes the change. Rebased to today's master, local tests passing. I think it's ready for final review.
Attachment #8639355 - Flags: review?(gweng)
(Assignee)

Comment 11

3 years ago
Finalized the code according to gweng's comments on the pull request. please r+.
Flags: needinfo?(gweng)
(Assignee)

Updated

3 years ago
Assignee: nobody → cr
It's great work and much more clear now. But would you mind to put the event handling statements into different functions, like `onTouchStart(evt)` and call that in the switch branches? I usually do that when the switch grows up like this, since to have a long switch case is bad for tracing and reading. Moreover, it makes unit tests much clear (since we only need to test each function, rather than test the same function with the switch). After that, you may also add some tests for each handler, which also can guarantee individual functions work well in the future.
Flags: needinfo?(gweng) → needinfo?(cr)
(Assignee)

Comment 13

3 years ago
Greg, I started out with creating separate event sub-handlers, but then was too appalled by blatant code duplication and lack state compartmentalization. I ended up with a major overhaul of InputPad and the accompanying test suite. I hope it now has the clarity as well the as code coverage that it deserves. 

Please re-review and r+ if you agree.
Status: NEW → ASSIGNED
Flags: needinfo?(cr) → needinfo?(gweng)
Comment on attachment 8639355 [details] [review]
[gaia] cr:lockscreentouch > mozilla-b2g:master

Thanks. The code now looks really great. Let's see if it works well on daily use.
Flags: needinfo?(gweng)
Attachment #8639355 - Flags: review?(gweng) → review+
(Assignee)

Comment 15

3 years ago
Thanks, Greg.
Keywords: checkin-needed
https://github.com/mozilla-b2g/gaia/commit/f64408f42bd81f2742f2b48725240016660ffe39
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → FxOS-S7 (18Sep)
Depends on: 1203269
(Assignee)

Updated

3 years ago
status-b2g-master: --- → fixed
You need to log in before you can comment on or make changes to this bug.