Closed Bug 1003330 Opened 8 years ago Closed 8 years ago

Mobile identity UI


(Firefox OS Graveyard :: Gaia::System, defect)

Gonk (Firefox OS)
Not set


(feature-b2g:2.0, b2g-v2.0 fixed)

2.0 S3 (6june)
feature-b2g 2.0
Tracking Status
b2g-v2.0 --- fixed


(Reporter: ferjm, Assigned: borjasalguero)



(Whiteboard: ft:loop)


(3 files, 2 obsolete files)

Bug 988469 is going to expose an API to allow privileged apps to obtain a verified phone number. We need to design and build some chrome UI bits for the following cases:

- Ask permission to the user to let the app get her phone number.
- Provide the user the ability to give a phone number that doesn't belong to a SIM inserted in the device (this is specially required for the Desktop case and might not be needed for Firefox OS).
- Provide the user the ability to select the desired SIM or phone number in a multi-SIM scenario.
- Ask the user for the verification code in case that the chosen phone number does not belong to any of the inserted SIMs or the code is not received in the device after a certain timeout.
- Allow the user to cancel the verification process.
Assignee: nobody → ferjmoreno
Blocks: webapi
No longer blocks: webapi
Attached patch WIP (obsolete) — Splinter Review
Attached patch WIP (obsolete) — Splinter Review
Attachment #8421068 - Attachment is obsolete: true
feature-b2g: --- → 2.0
Whiteboard: ft:loop
Assignee: ferjmoreno → borja.bugzilla
Target Milestone: --- → 2.0 S3 (6june)
Attached file Pull Request
Patch ready to review!
Attachment #8421791 - Attachment is obsolete: true
Attachment #8434130 - Flags: review?(ferjmoreno)
Comment on attachment 8434130 [details] [review]
Pull Request

Thanks Borja! Excellent work! :)

I did a first review pass and left a few comments in Github.

Also, apart from what I mentioned in the PR, while testing this patch I found a few issues:

1- The progress bar for the verification timeout is not working.

2- Error alerts are not working (at least on the device).

3- The input field error dance is not working.

4- Cannot re-launch the dialog after clicking in the home button.

5- I can manually scroll from the SIM options to the manual phone number input.

6- The phone number input is too long and doesn't fit in the screen.
Attachment #8434130 - Flags: review?(ferjmoreno) → feedback+
Comment on attachment 8434130 [details] [review]
Pull Request

All issues detected were related with your suggestions, so now everything should work as expected! r?
Attachment #8434130 - Flags: review?(ferjmoreno)
Comment on attachment 8434130 [details] [review]
Pull Request

Great job as usual! :)

I left a few more comments in the PR, minor issues only.

Since this is adding a new system dialog, we need to get sr? from a module owner. Thanks in advance Alive!
Attachment #8434130 - Flags: superreview?(alive)
Attachment #8434130 - Flags: review?(ferjmoreno)
Attachment #8434130 - Flags: review+
Oh, and remember to run the tools/ script if you haven't done it already, please.
Comment on attachment 8434130 [details] [review]
Pull Request

I only check the dialog and its manager.

Basically what's being implemented in current system app is:
Things about the iframe belong to one dialog / one window should be done in the dialog / window module.
The XXXX manager's load will be less if the instances managed by him doing its own work well.

Also, if next time you want to communicate from an embedded iframe to system app, window.parent.XXXX is somewhat bad coupling. Try dispatch event like 'fxa-xxxxxx' to window.parent and ask some one to caught it.
Attachment #8434130 - Flags: superreview?(alive) → review+

R+ and I've addressed all your suggestions. I'll move to events when adding the new errors in a follow-up, once the whole list will be available. Thanks!
Closed: 8 years ago
Resolution: --- → FIXED
Note for English: "Ok" should be spelled "OK".
Target Milestone: 2.0 S3 (6june) → ---
Looks like TBPL doesn't like unicode file names ?

Also, is this file expected: ?
(In reply to Julien Wajsberg [:julienw] from comment #13)
> Looks like TBPL doesn't like unicode file names ?
> Also, is this file expected:
> 9e164703a195517d1b9de5c1c346285673ab5dd6#diff-20 ?

Thanks Julien for pointing out where the problem was. It's an old file (actually it was a test for the gif animation), and I dont know why it was in the PR. Removing it and waiting for re-merging.
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #12)
> Reverted for TBPL bustage.
> Master:
> 2e5636e852a9354a5f8072b179cf16f72647cfd6

Hi Ryan, my fault, it seems an old file was included by error in the PR. I've removed it and it's in a new PR:

Waiting Travis to re-merge this. Sorry for this issue!

There was an old file included in the PR, so Integration tests were failing. I've remove the file and waiting Travis to merge.
(In reply to Francesco Lodolo [:flod] from comment #11)
> Note for English: "Ok" should be spelled "OK".

As I had to re-push it, I've added this.
Merged (I hope this time with no issue!) :)
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.0 S3 (6june)
You need to log in before you can comment on or make changes to this bug.