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)

ARM
Gonk (Firefox OS)
defect

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.
Priority: -- → P1
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.
Let's get the concept in comment 1 (sans the 3rd party keyboard part) tested.
Assignee: nobody → jlu
Whiteboard: [p=1]
Target Milestone: --- → 2.1 S2 (15aug)
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
Whiteboard: [p=1] → [p=2]
Attachment #8468349 - Attachment is obsolete: false
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)
Comment on attachment 8468349 [details] [review]
Proposed Patch (PR @ GitHub)

Sorry, set a wrong requet.
Attachment #8468349 - Flags: review?(ofeng) → feedback?(ofeng)
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)
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?
NI Tim for comment 7 ^^^
Flags: needinfo?(timdream)
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-
I don't know why that line exists either. I think we can safely remove that line.
Flags: needinfo?(timdream)
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)
I think let's hold the patch until further clarification from Omega.
(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)
(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)
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)
Alright. Let's land the patch then. I rebased early this morning and TBPL seems pretty unstable right now, will merge after all green.
Master: https://github.com/mozilla-b2g/gaia/commit/73e2ecacbcbe93ab57dced4861b3beb34fcf68c6
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Component: Gaia::Keyboard → Gaia::System::Input Mgmt
Depends on: 1055908
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.

Attachment

General

Created:
Updated:
Size: