Closed
Bug 1035117
Opened 10 years ago
Closed 10 years ago
[Keyboard] To support password input
Categories
(Firefox OS Graveyard :: Gaia::System::Input Mgmt, defect, P1)
Tracking
(Not tracked)
RESOLVED
FIXED
2.1 S2 (15aug)
People
(Reporter: Omega, Assigned: mnjul)
References
Details
(Whiteboard: [p=2])
Attachments
(1 file)
If user only selects the IMEs which don't support password input (e.g. Zhuyin and Pinyin), when user taps a password input, it should switch IME to Built-in English forcely. In another case, if user selects English, Deutsch and Zhuyin, user should be only able to switch between English and Deutsch in a password input.
Reporter | ||
Updated•10 years ago
|
Blocks: keyboard-ux-update
Updated•10 years ago
|
Priority: -- → P1
Assignee | ||
Comment 1•10 years ago
|
||
I discussed with Rudy with some experimentation on another mobile operation system. Some observations with the mobile OS when on <input type="password">: (ps. please regard "latin" as "English", "French", "German", ...) 1. If no latin keyboard is enabled, the OS will always bring up a English keyboard (you can't switch to other keyboards). This is true even the OS UI language is some latin language other than English, e.g. French. 2. If one latin keyboard is enabled, the OS will bring up that one keyboard, even though it might not be English. You can't switch keyboards. 3. If more than one latin keyboards are enabled, the OS will allow you to cycle through those keyboards, just like in normal situations except only latin keyboards will be available (no Chinese, Japanese, ...). 4. Diacritics seems always available. From these observations it seems more like we'll need to define a new group in Input Management, 'password', and alter all latin keyboards to support it. There is one interesting issue brought up by Tim yesterday regarding 3rd-party keyboard and type='password': the 3rd-party keyboard is essentially able to record user keystrokes if so desired. We might need to disallow 3rd-party keyboard from accessing input='password' unless it's certified or something like that.
Assignee | ||
Comment 2•10 years ago
|
||
Let's get the concept in comment 1 (sans the 3rd party keyboard part) tested.
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → jlu
Whiteboard: [p=1]
Target Milestone: --- → 2.1 S2 (15aug)
Assignee | ||
Comment 3•10 years ago
|
||
Comment on attachment 8468349 [details] [review] Proposed Patch (PR @ GitHub) Patch does not conform to spec -- Settings needs to allow user to have *no* password-support keyboard and input management would fall back automatically.
Attachment #8468349 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Whiteboard: [p=1] → [p=2]
Assignee | ||
Updated•10 years ago
|
Attachment #8468349 -
Attachment is obsolete: false
Assignee | ||
Comment 4•10 years ago
|
||
Comment on attachment 8468349 [details] [review] Proposed Patch (PR @ GitHub) Hi Omega, As said earlier yesterday, please check if the patch meets your expectation. There is one known issue: 1. Enable only the Zhuyin keyboard, aside from the Number keyboard. 2. Go to UI Keyboard test, focus on the password input field. The English keyboard is brought up, as specified in this bug. 3. Open the utility tray's IME switcher. When the list is brought up, press 'Cancel'. 4. You will see that Zhuyin keyboard is brought up, instead of the English keyboard. I have identified the possible cause of the issue but I need to discuss with Tim when he is back regarding why the specific code is lying there.
Attachment #8468349 -
Flags: review?(ofeng)
Assignee | ||
Comment 5•10 years ago
|
||
Comment on attachment 8468349 [details] [review] Proposed Patch (PR @ GitHub) Sorry, set a wrong requet.
Attachment #8468349 -
Flags: review?(ofeng) → feedback?(ofeng)
Assignee | ||
Comment 6•10 years ago
|
||
Comment on attachment 8468349 [details] [review] Proposed Patch (PR @ GitHub) Hi Tim, As this patch largely involves input management, here's the request for your review. The patch is actually more about the "fallback" mechanism (as in UX spec: Settings would allow user to have no password-supporting layouts, and under such situation bring up a fallback ('en') layout when <input type="password"> is focused. In this patch I mainly hope to get the architecture of the fallback mechanism right. Issues noteworthy: 1. I didn't reuse/modify KeyboardHelper's checkDefaults/changeDefaultLayouts as the former affects caller (which sticks into Settings module) and the latter affects settings; and IMO the "fallback" is supposed to be more silent/implicit. We might want to move the fallback configuration from being hardcoded in KeyboardHelper to somewhere like keyboard_layouts.json, but I guess that can be done in a follow-up bug. 2. KeyboardManager's added codes are not unit-tested because the whole updateLayouts function was not unit-tested. I think we can open up a follow-up bug for this, right?
Attachment #8468349 -
Flags: review?(timdream)
Assignee | ||
Comment 7•10 years ago
|
||
Also Tim, do you know the reason of this specific extra line of code: https://github.com/mnjul/gaia/blob/bug_1035117_keyboard_input_password_type/apps/system/js/keyboard_manager.js#L685 ? This line is causing the 'known issue' in comment 4. It re-retrieves showingLayout when the IMESwitcher is canceled. However, since |hideKeyboard()| is called before IMESwitcher is launched (line 669), |showingLayout.type| would be reset to 'text' -- thus, when IMESwitcher is canceled, it's not 'password'-type layout that would be brought up, but the 'text'-type layout. I asked Greg (the committer of the code) and Rudy and they said they didn't know the specific reason of the line of the code. I commented out the code locally and everything felt fine. So is it there for a reason? Or is it just "a slip of hand" and let's try remove it?
Reporter | ||
Comment 9•10 years ago
|
||
Comment on attachment 8468349 [details] [review] Proposed Patch (PR @ GitHub) Hi John, Everything is good but the IME selector issue you mentioned. Please fix that one, thanks!
Attachment #8468349 -
Flags: feedback?(ofeng) → feedback-
Comment 10•10 years ago
|
||
I don't know why that line exists either. I think we can safely remove that line.
Flags: needinfo?(timdream)
Comment 11•10 years ago
|
||
Comment on attachment 8468349 [details] [review] Proposed Patch (PR @ GitHub) The fallback part is weird but I can't think of better API, if Omega insist quietly and surprisingly enabling English layout of the built-in keyboard for the user, we should take this patch. I still want to argue not allowing user to disable all password layouts in Settings. If a prompt is undesired, we can straightforwardly disable the enabled checkbox of the last password-capable layout (when we detect that layout is the last one). Needinfo Omega for considerations.
Attachment #8468349 -
Flags: review?(timdream) → review+
Flags: needinfo?(ofeng)
Assignee | ||
Comment 12•10 years ago
|
||
I think let's hold the patch until further clarification from Omega.
Reporter | ||
Comment 13•10 years ago
|
||
(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #11) > Comment on attachment 8468349 [details] [review] > Proposed Patch (PR @ GitHub) > > The fallback part is weird but I can't think of better API, if Omega insist > quietly and surprisingly enabling English layout of the built-in keyboard > for the user, we should take this patch. Let's do it. :) > I still want to argue not allowing user to disable all password layouts in > Settings. If a prompt is undesired, we can straightforwardly disable the > enabled checkbox of the last password-capable layout (when we detect that > layout is the last one). Needinfo Omega for considerations. Disabling checkbox directly doesn't tell the user why he cannot uncheck it, but a prompt helps. So I think it's fine to show a prompt in this condition.
Flags: needinfo?(ofeng)
Comment 14•10 years ago
|
||
(In reply to Omega Feng [:Omega] [:馮於懋] from comment #13) > (In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from > comment #11) > > Comment on attachment 8468349 [details] [review] > > Proposed Patch (PR @ GitHub) > > > > The fallback part is weird but I can't think of better API, if Omega insist > > quietly and surprisingly enabling English layout of the built-in keyboard > > for the user, we should take this patch. > Let's do it. :) > > > I still want to argue not allowing user to disable all password layouts in > > Settings. If a prompt is undesired, we can straightforwardly disable the > > enabled checkbox of the last password-capable layout (when we detect that > > layout is the last one). Needinfo Omega for considerations. > Disabling checkbox directly doesn't tell the user why he cannot uncheck it, > but a prompt helps. So I think it's fine to show a prompt in this condition. I am confused, if we should do prompt, we don't need to fallback to a En layout since there is always be a password capable layout. Should we do prompt OR fallback to En layout?
Flags: needinfo?(ofeng)
Reporter | ||
Comment 15•10 years ago
|
||
Sorry for confusing. I talked to John and found that I misunderstood about the prompt. Let me clarify here: 1) Use En fallback. 2) User can choose only non-password-supported layouts, and there is no prompt for that.
Flags: needinfo?(ofeng)
Assignee | ||
Comment 16•10 years ago
|
||
Alright. Let's land the patch then. I rebased early this morning and TBPL seems pretty unstable right now, will merge after all green.
Assignee | ||
Comment 17•10 years ago
|
||
Master: https://github.com/mozilla-b2g/gaia/commit/73e2ecacbcbe93ab57dced4861b3beb34fcf68c6
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•10 years ago
|
Component: Gaia::Keyboard → Gaia::System::Input Mgmt
Assignee | ||
Comment 18•10 years ago
|
||
FTR: My patch missed new unit tests for keyboard_manager.js. bug 1055908 will deal with it.
You need to log in
before you can comment on or make changes to this bug.
Description
•