[Settings] No cursor Display in Simpin input textfield on focus

RESOLVED FIXED in 1.1 QE2 (6jun)

Status

Firefox OS
Gaia::Settings
P1
normal
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Leo, Assigned: rudyl)

Tracking

unspecified
1.1 QE2 (6jun)
ARM
Gonk (Firefox OS)
Bug Flags:
in-moztrap +

Firefox Tracking Flags

(blocking-b2g:leo+, b2g18 fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 affected)

Details

(Whiteboard: [TD-25154], URL)

Attachments

(3 attachments)

(Reporter)

Description

5 years ago
1. Title : No cursor Display in Simpin input textfield on focus .
2. Precondition : 
3. Tester's Action : Settings >> Sim Security >> Change SimPin >> fill text fields of Simpin, create new Simpin                       
4. Detailed Symptom (ENG.) : There is no cursor to display in which textfield box,the number is getting typed.
5. GAIA version : 0d2b50d1c7083e5c19d7f0d0eebc019b7a7e8374
6. Expected : A cusor should be displayed in the textfield in which the number is getting typed
7.Reproducibility: Y
1)Frequency Rate : 100%
8.Comparison Results : 
1)Model Comparing : 
9. Attached files: 
1)Log : 
2)Test Contents : 
3)Video file :
(Reporter)

Comment 1

5 years ago
Created attachment 746874 [details]
screenshot of the view

The current field will not be focused and creates confusion to user in finding the current field.
(Reporter)

Comment 2

5 years ago
The action on focus should be to display cursor.
Can you give some information about whether cusor is not dispaying due to
SIM Pin customized text fields or some other issue.
please give your comment.
Flags: needinfo?(rlu)
Hi,

I guess this is due to our little trick to deal number+password input field case,

e.g. from the following section
 <div class="input-wrapper">
  <input type="number" x-inputmode="digits" name="simpin" size="8" maxlength="8" />
  <input type="text" name="simpinVis" size="8" maxlength="8" />
</div>

the shown input is type="text" while the focused input is type="number".
Since the focused one (with the cursor) is hidden, you could not see the cursor either.

Thanks.
Flags: needinfo?(rlu)
(Reporter)

Updated

5 years ago
Whiteboard: [TD-24003]

Updated

5 years ago
Target Milestone: --- → 1.1 QE2

Comment 4

5 years ago
(In reply to Rudy Lu [:rudyl] from comment #3)
> Hi,
> 
> I guess this is due to our little trick to deal number+password input field
> case,
> 
> e.g. from the following section
>  <div class="input-wrapper">
>   <input type="number" x-inputmode="digits" name="simpin" size="8"
> maxlength="8" />
>   <input type="text" name="simpinVis" size="8" maxlength="8" />
> </div>
> 
> the shown input is type="text" while the focused input is type="number".
> Since the focused one (with the cursor) is hidden, you could not see the
> cursor either.
> 
> Thanks.

Do you have any idea to fix this issue?
Need to show the cursor for user

Updated

5 years ago
blocking-b2g: --- → leo+
(Reporter)

Updated

5 years ago
Priority: -- → P1
Will take a look to try if we could resolve this with "x-inputmode".
Assignee: nobody → rlu
Created attachment 751344 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/9859/files

Pointer to Github pull-request
Comment on attachment 751344 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/9859/files

This is the 1st part of the patch to enable the keyboard app to support x-inputmode="digits" for type="password" and type="text", etc.

Dear David,

Could you please help take a look?
Thank you.
Attachment #751344 - Flags: review?(dflanagan)
(Reporter)

Updated

5 years ago
Whiteboard: [TD-24003] → [TD-29915]
(Reporter)

Updated

5 years ago
Whiteboard: [TD-29915] → [TD-25154]
Rudy,

This is the first time I've looked at the code for x-inputmode=digits. I didn't know we had that...  I've made some comments on github, but am not quite ready to do a full review yet. Overall the code looks fine, but I have some questions before I can understand it well enough to review carefully:

1) What is the code in render.js for?  What problem is it solving? It seems less directly related to bug description than the changes in keyboard.js

2) I don't like that we support the non-standard input mode "digits" without supporting the "numeric" mode that is actually in the HTML5 spec.  Should we consider supporting 'numeric' in addition to 'digits'?

3) We we ever use inputmode='digits' for anything other than PINs?  Are they always type=password?  If so, then maybe we could convert inputmode=digits to inputmode=numeric, to be closer to the standard, but add a special case where type=password inputmode=numeric does digits only without the decimal point, minus sign, etc. 

I don't know what we do on Android, so I suppose we should be compatible with that. And I'm not interested in holding up this Leo+ bug for a standards discussion.  But I do want to make sure that we're not gratiuitously deviating from standards here.
Flags: needinfo?(rlu)
Sorry for the late reply.

(In reply to David Flanagan [:djf] from comment #8)
> Rudy,
> 
> This is the first time I've looked at the code for x-inputmode=digits. I
> didn't know we had that...  I've made some comments on github, but am not
> quite ready to do a full review yet. Overall the code looks fine, but I have
> some questions before I can understand it well enough to review carefully:
> 
> 1) What is the code in render.js for?  What problem is it solving? It seems
> less directly related to bug description than the changes in keyboard.js

Originally, it depended on inputType to see we would need to apply "special-key", "big-key" class name to the keys in "pinLayout" and "numberLayout".
Right now, there are more cases, say inputmode="digits" and type="password", that we need to apply that special class names.

> 
> 2) I don't like that we support the non-standard input mode "digits" without
> supporting the "numeric" mode that is actually in the HTML5 spec.  Should we
> consider supporting 'numeric' in addition to 'digits'?
>

Yeah, I can manage to support "numeric" mode, too. Please stay tuned. 

> 3) We we ever use inputmode='digits' for anything other than PINs?  Are they
> always type=password?  If so, then maybe we could convert inputmode=digits
> to inputmode=numeric, to be closer to the standard, but add a special case
> where type=password inputmode=numeric does digits only without the decimal
> point, minus sign, etc. 
> 

So far, yes, that is for PIN code only.
I don't think we could use "numeric" here, since minus/other symbols are not allowed for PIN code, which is why "digits" mode was introduced to solve this particular requirement.

> I don't know what we do on Android, so I suppose we should be compatible
> with that. And I'm not interested in holding up this Leo+ bug for a
> standards discussion.  But I do want to make sure that we're not
> gratiuitously deviating from standards here.

I have checked Firefox on Android (Fennec) and I didn't see inputmode or x-inputmode got supported.
Since it is still "x"-inputmode here, so hopefully, we could refine what mode we need to support and the behavior after the standards are defined more clearly.
(I heard from somewhere that sometimes the implementation should go along while the standards is also evolving, hope this applies here as well.)

Thanks.
Flags: needinfo?(rlu)
What I was suggesting is that we use inputmode=numeric, but we make a special case for type=password to show the pinLayout.  And for other types we show the numberLayout. I don't know if that is a good idea or not.

Let me know when you want me to contine the review.

Updated

5 years ago
Duplicate of this bug: 850120

Updated

5 years ago
Attachment #751344 - Flags: review?(dflanagan)
Comment on attachment 751344 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/9859/files

Hi David,

I have refined the patch of keyboard part as follows,

1. Enable both numeric and digit inputmode for Keyboard.
   (Note that mode="digits" has been changed to "digit", according to Bug 746142#c9)
    This part needs Yuan's feedback.

2. Also fix a regression that sometime the suggestion bar would show up incorrectly, the solution is to call renderKeyboard() after IME activation, so that the suggestion/capitalization status could be reflected correctly.

The changes are in a separate commit, hope to make it easier to review,
https://github.com/RudyLu/gaia/commit/1f9487e7c935783d723a9dbaec40e65bf2c5d354

I will squash them if r+.
Thanks.
Attachment #751344 - Flags: review?(dflanagan)
Attachment #751344 - Flags: feedback?(xyuan)
Comment on attachment 751344 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/9859/files

r=djf for github commits 8aeefcf and 1f9487e.

I'm giving r+ here, but I do have some nits on github.  In particular, I did want to say that the singular form "inputmode=digit" sounds like an error to my ear.  I do think the plural version is better.

Also, please consider modifying the number layouts in layout.js to add a bigkey:true flag to the layouts or keys that need it.  That way render.js can just check the layout data to find out if the big-key CSS class is needed.  That seems like better modularity to me.

Thanks also for fixing when renderKeyboard() is called.
Attachment #751344 - Flags: review?(dflanagan) → review+
(In reply to Rudy Lu [:rudyl] from comment #12)
> Comment on attachment 751344 [details]
> Pointer to Github pull request:
> https://github.com/mozilla-b2g/gaia/pull/9859/files
> 
> Hi David,
> 
> I have refined the patch of keyboard part as follows,
> 
> 1. Enable both numeric and digit inputmode for Keyboard.
>    (Note that mode="digits" has been changed to "digit", according to Bug
> 746142#c9)
>     This part needs Yuan's feedback.

It seems there is no standard specification for inputmode now and it has change a lot since Bug 746142. We may not need to change the name |digits| to |digit|.

Mounir will be a better person to provide feeback as he knowns more about the inputmode and its standard.
Depends on: 820268
Flags: needinfo?(mounir)
Attachment #751344 - Flags: feedback?(xyuan) → feedback+
Hi David, Yuan,

Thanks for your feeback.

I have updated the patch with the following commit,
https://github.com/RudyLu/gaia/commit/11055a51c4c666456d15bed2523633f95f548b69

For 'digit' or digit"s" for the inputmode, I tend to use 'digit' because it synced with Gecko part of implementation.
e.g. https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/GeckoInputConnection.java#576
Created attachment 754410 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/9859/files

Pointer to Github pull-request
Comment on attachment 754410 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/9859/files

Dear Arthur,

Please help review the simpin dialog related changes, which could be found in commit, https://github.com/RudyLu/gaia/commit/bcc9805a96f3346a4006460553b930e30bf7e014.

In this patch, we use "type=password, x-inputmode=digit" for the SIM pin, PUK code, etc., instead of using the previous hack that would do the combining of one number input and one text input.

Thanks.
Attachment #754410 - Flags: review?(arthur.chen)
I found that we also have SIM pin related code in FTU, but not sure why it does use "inputmode=digits" before.

Hi Fernando,

May I know when we could see the SIM pin dialog in FTU (not the one in system app)?
Thanks.
Flags: needinfo?(fernando.campo)
(In reply to Rudy Lu [:rudyl] from comment #18)
> I found that we also have SIM pin related code in FTU, but not sure why it
> does use "inputmode=digits" before.
> 
> Hi Fernando,
> 
> May I know when we could see the SIM pin dialog in FTU (not the one in
> system app)?
> Thanks.

The SIM pin screen is the second screen on FTU, should appear after language screen (1st one), but only if we have a SIM inserted and PIN protected. If we don't have SIM, it will jump to wifi, if PIN is not enabled, it will only show the option for enabling mobile-data.
Flags: needinfo?(fernando.campo)
(In reply to Yuan Xulei [:yxl] from comment #14)
> (In reply to Rudy Lu [:rudyl] from comment #12)
> > Comment on attachment 751344 [details]
> > Pointer to Github pull request:
> > https://github.com/mozilla-b2g/gaia/pull/9859/files
> > 
> > Hi David,
> > 
> > I have refined the patch of keyboard part as follows,
> > 
> > 1. Enable both numeric and digit inputmode for Keyboard.
> >    (Note that mode="digits" has been changed to "digit", according to Bug
> > 746142#c9)
> >     This part needs Yuan's feedback.
> 
> It seems there is no standard specification for inputmode now and it has
> change a lot since Bug 746142. We may not need to change the name |digits|
> to |digit|.
> 
> Mounir will be a better person to provide feeback as he knowns more about
> the inputmode and its standard.

"digits" or "digit" doesn't change much to me. There is no standard for inputmode even if there might sound like it because Hixie wrote something. This is still not a standard. We should feel free to do whatever we want for the moment until Mozilla comes up with a real implementation we mean to support (not "x-" prefixed) and get standardized.
Flags: needinfo?(mounir)
Comment on attachment 754410 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/9859/files

Thanks for the patch. It looks good to me. r=me.
Attachment #754410 - Flags: review?(arthur.chen) → review+
Comment on attachment 754410 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/9859/files

Just updated the pull request for last part:
Use type=password, x-inputmode=digit for simpin input in FTU
 https://github.com/RudyLu/gaia/commit/db09b148263e68ac9d81101735fb4b44a503ea96

Dear Fernando,

Could you help take a look at this change?
Thanks.
Attachment #754410 - Flags: review?(fernando.campo)
Comment on attachment 754410 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/9859/files

FTU changes looks great, thanks for the patch!
Attachment #754410 - Flags: review?(fernando.campo) → review+
Merged to Gaia master,
https://github.com/mozilla-b2g/gaia/commit/8eec8e331ec58e0a1b3d0b926d0aaa0d0b50f477

Hi all,

Thanks for all the reviews.
Status: NEW → RESOLVED
Last Resolved: 5 years ago
status-b2g18: --- → affected
status-b2g18-v1.0.0: --- → wontfix
status-b2g18-v1.0.1: --- → affected
Resolution: --- → FIXED
Gaia v1-train:
5919649b3ab60759ad2b55dc7e38b4cfaec7d7b9
status-b2g18: affected → fixed

Updated

5 years ago
Flags: in-moztrap?

Updated

5 years ago
Flags: in-moztrap? → in-moztrap+
You need to log in before you can comment on or make changes to this bug.