949439 - [DSDS] Follow Bug 926347, add progress bar to SIM Manager

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::First Time Experience, defect)

Description

[Michael Henretty [:mikehenrty][:mhenretty]]

Based on UX review of bug 926347, we decided we want a progress bar on the SIM Manager screen. Since bug 926347 was 1.3+ and had not much time left to land, we decided to add this progress bar in a follow-up here. 

See https://bugzilla.mozilla.org/show_bug.cgi?id=926347#c42.

Comment 1

[Michael Henretty [:mikehenrty][:mhenretty]]

Guillaume is going to take this as his first bug.

Comment 2

[Michael Henretty [:mikehenrty][:mhenretty]]

Attachment: Sim manager with progress bar screenshot

Comment 3

[Guillaume Marty [:gmarty]]

Attachment: Github PR for FTU refactoring

Hey Francisco! I was working on this bug and thought it would be a good idea to refactor the FTU to include the SIM unlock overlay as a regular step in the process and get rid of the old IccHelper.
This PR is still buggy and need polishing for edge cases, but I'd appreciate your comment on this. Am I on the right track? Shall I continue this way?

Comment 4

[Francisco Jordano [:arcturus] [:francisco]]

Comment on attachment 8404595 [details] [review]
Github PR for FTU refactoring

Hei Guillaume!!

Happy to see you around!

Unfortunately don't have much time right now since I'm focus on contacts, but will forward it to Fernando.

Fernando, would you mind?

Comment 5

[Fernando Campo (:fcampo)]

Sure, I'll take a look :)

Comment 6

[Fernando Campo (:fcampo)]

Comment on attachment 8404595 [details] [review]
Github PR for FTU refactoring

Hi gmarty, sorry it took me so long to check it.

I left some comments on github, but I understand that this is a work in progress, so I guess that you had most of them into account.

The approach looks good, but be aware of bug 984404. It's close to an end, and the changes on navigation will probably collide with your new code.

My main concern with this refactor is the behaviour when skipping steps, both passively (no SIM on device) or actively (user decides to skip PIN code), and how that reflects on the step bar, specially when going backwards. 
It has been causing us headaches since the beginning of times, so please be extra careful with that, as it's probably the part of FTE more prone to regressions.

Also, as I understand from bug 926347 comment 42, the navigation has to be shown in the screens that are steps of the FTE, but it is not clear that it applies to the PIN screen. 
This screen was designed to copy the system PIN screen, as a pop-up, and not as a step (more like an enabler to the sim-related steps), so I don't think that the step-bar should be shown on it. I understand that this is more a question for UX, so I'm ni? them

Comment 7

[Guillaume Marty [:gmarty]]

Hi Fernando,

Thanks for your feedback.

I wasn't aware of bug 984404, so it's better to get that one landed before I go further with the refactoring. Do you know when it is supposed to land? The sooner, the better for me.

Also, the navigation bar should be visible on all steps (including PIN unlocking and SIM manager screens), see the attachment above. For that reason, it makes sense to refactor them as regular steps in the process, and not just popups.

Also, in the meantime, I found bug 916826#c3, so I'll work in this direction.

Comment 8

[Fernando Campo (:fcampo)]

There's a proposal for Tutorial navigation on bug 976535 that might be helpful for you.

Comment 9

[Fernando Campo (:fcampo)]

(In reply to gmarty from comment #7)
> Hi Fernando,
> 
> Thanks for your feedback.
> 
> I wasn't aware of bug 984404, so it's better to get that one landed before I
> go further with the refactoring. Do you know when it is supposed to land?
> The sooner, the better for me.
Probably today, tops tomorrow :) I'm finishing the review now

Comment 10

[Guillaume Marty [:gmarty]]

(In reply to Fernando Campo (:fcampo) from comment #8)
> There's a proposal for Tutorial navigation on bug 976535 that might be
> helpful for you.

That'd be great to complete that one too. From my understanding, the patch is ready, so it just needs a rebase when bug 984404 lands, then the test suite needs to be updated.
I'll add a comment there.

Comment 11

[Jacqueline Savory [:jsavory] UX]

From the comment I made in bug 926347, I intended the SIM Pin page to include the progress bar as well, similar to how the wifi password works, but it wasn't possible in that time frame. 

For the SIM Pin page, the progress bar will remain in the same spot as the SIM Manager page, since it is all part of the same section. It looks to me like the screenshots added in comment 2 are correct.

Comment 12

[Guillaume Marty [:gmarty]]

Attachment: Github PR - Add progress bar to SIM Manager #18550

Comment 13

[Guillaume Marty [:gmarty]]

After further discussion, it has been considered that the FTU refactor will take longer. It is tracked at bug 916826.

For the time being, I submitted a new PR for a quick and dirty fix to the bug discussed here. Obviously, when the FTU will be refactored, the SIM manager screens will be fully integrated in the flow and won't require this fix.

Comment 14

[Francisco Jordano [:arcturus] [:francisco]]

Comment on attachment 8410651 [details] [review]
Github PR -  Add progress bar to SIM Manager #18550

Kindly passing the review to Borja since he has the DSDS device in London office.

@Borja, if you cannot fulfill this review please send me the device and I'll check.

Thanks.

Comment 15

[Borja Salguero [:borjasalguero]]

Comment on attachment 8410651 [details] [review]
Github PR -  Add progress bar to SIM Manager #18550

Hi! Please remove all changes not related with the progress bar. We want to have a clear track about the changes done, and we should follow one bug, one patch paradigm. If your patch is changing a lot of things not related with the scope of the patch, and imagine that there is any regression, it could be tough to identify when the change was added. Please remove all changes not related with the progress bar and I will take a look again. Thanks!

Comment 16

[Guillaume Marty [:gmarty]]

OK, I reverted all the changes on index.html for now.
How does it look?

But I want to merge it as there are a lot of things to fix in the HTML file (inconsistent tags, duplicate ID, missing references, misclosed tags...).

Comment 17

[Fernando Campo (:fcampo)]

I think you need to rebase gmarty, there're changes on tutorial_steps.js, and that file does no longer exists after landing of bug 976535.

My two cents about index changes, I agree that those need to be addressed eventually, but as Borja said, not the best idea to do it in this bug, it will complicate things when tracking changes.
I also vote for the changes needed for the feature to land. We can always open another bug for the inconsistencies on the index, or fix them along with the refactor.

Comment 18

[Guillaume Marty [:gmarty]]

Just rebased it. Hopefully, it's ready to be merged now :-)

Comment 19

[Fernando Campo (:fcampo)]

Comment on attachment 8410651 [details] [review]
Github PR -  Add progress bar to SIM Manager #18550

Nice, just tested and looks nice (only with single sim device, though, so still need to test it with DSDS). Review is still on borja, as he has the device :p

Just another more question to jsavory that just occurred to me when testing.

- If we are treating PIN screen as a step on the FTU, and we show a transition from step 1 (languages) to step 2 (SIM Manager), should we show then the next screen as step 3, even if we SKIP the PIN input?

If we don't, then the user will see the step moving together with the screen from 1 to 2, then press skip, and screen changing to wifi, but not the step indicator.

If we do, and assume that SIM PIN is a step (2) no matter what, and wifi is always step 3 (except when no SIM is detected), when user skips it going forward, skipping it on our way backwards will show a jump on the step (from 3 to 1).

Either way, it feels a little weird for the user.

Any opinions on this? Which one has the less impact on user experience?

Comment 20

[Jacqueline Savory [:jsavory] UX]

Hi Fernando, I agree that the SIM section, including the pin input needs to be its own section on the progress bar (step 2) and wifi should be its own section as well (step 3). Unless in the case you stated, the user has no SIM card. Otherwise, I think step 2 will be way too long if the user has multiple pins and connects to a wifi network.

I understand the problem with skipping from step 3 to 1 but I think it is less of a problem since most users will not be going back.

Comment 21

[Guillaume Marty [:gmarty]]

Is there anything preventing this PR to be merged? I reverted all the changes not directly related to this bug. Thanks.

Comment 22

[Gregor Wagner [:gwagner]]

(In reply to gmarty from comment #21)
> Is there anything preventing this PR to be merged? I reverted all the
> changes not directly related to this bug. Thanks.

You still need to wait for a review+.

Comment 23

[Borja Salguero [:borjasalguero]]

Im cleaning my review queue and I'll take a look tomorrow to your patch testing it with the DSDS device.
On the other hand the way of the navigation bar is working is quite confusing (the number of steps, the way we are showing to the user the 'progress' of the process) overall because the number of steps is not defined from the beginning (we have a lot of conditions for showing one step or another).
Maybe we should investigate other visual solution for this, but we will work in a separate patch.

Comment 24

[Borja Salguero [:borjasalguero]]

Comment on attachment 8410651 [details] [review]
Github PR -  Add progress bar to SIM Manager #18550

Hi! After testing with the DSDS device (both SIM Cards have NO PIN CODE needed), I've experienced the following issues:

STR 1:
- Plug 2 sims in the DSDS device
- Boot device for the first time, running FTU
- See 'languages screen'
- Tap on next, the 'SIM Card selection' is shown
- Tap on next, 'Enable Data' is shown
- Tap on BACK

EXPECTED:
- Going to 'SIM Card selection'

CURRENTLY:
- We go to 'Languages screen', which is weird taking into consideration the flow.

STR 2:
- Plug 2 sims in the DSDS device
- Boot device for the first time, running FTU
- See 'languages screen'
- Tap on next, the 'SIM Card selection' is shown
- Tap on next, 'Enable Data' is shown
- Tap on next, 'Wifi screen' is shown
- Tap on BACK

EXPECTED:
- Going to 'Enable Data'

CURRENTLY:
- We go to 'SIM Card selection', which is weird taking into consideration the flow.


STR 3:
- Plug 2 sims in the DSDS device
- Boot device for the first time, running FTU
- See 'languages screen'
- Tap on next, the 'SIM Card selection' is shown
- Tap on the SIM 2 to make this SIM the 'default' one

EXPECTED:
Default SIM Card is changed from SIM 1 to SIM 2

CURRENTLY:
Nothing happens.

Could you take a look to these scenarios? Thanks!

Comment 25

[Guillaume Marty [:gmarty]]

Hey Borja, we know the current implementation is not ideal and that's why we want to refactor it (See bug 916826).
But for the issues you describe are a bit out of the scope of this bug and should have their own bugs. The first 2 ones are expected because of the current implementation. The 3rd issue looks like a bug.

Can you fill in separate issues for these?

Comment 26

[Michael Henretty [:mikehenrty][:mhenretty]]

Comment on attachment 8410651 [details] [review]
Github PR -  Add progress bar to SIM Manager #18550

(In reply to Borja Salguero [:borjasalguero] from comment #24)
> STR 1:
>  ...
> STR 2:
>  ...

Yup, these are regressions in FTE navigation that exist in the current master. I will file some bugs for these.

> 
> STR 3:
> - Plug 2 sims in the DSDS device
> - Boot device for the first time, running FTU
> - See 'languages screen'
> - Tap on next, the 'SIM Card selection' is shown
> - Tap on the SIM 2 to make this SIM the 'default' one
> 
> EXPECTED:
> Default SIM Card is changed from SIM 1 to SIM 2
> 
> CURRENTLY:
> Nothing happens.

This is not actually a bug. According to spec [1], this page is informational only and is not interactive. It also says on the bottom of the screen in red "You can change the default settings at Settings->Sim Card Manager."

Borja, it turns out that none of these issues apply to Guillaume's patch, can you re-review Guillaume's code.

1.) https://bug917705.bugzilla.mozilla.org/attachment.cgi?id=8334341

Comment 27

[Michael Henretty [:mikehenrty][:mhenretty]]

Filed bug 1009240 for STR 1 and STR 2 from comment 24.

Comment 28

[Borja Salguero [:borjasalguero]]

Comment on attachment 8410651 [details] [review]
Github PR -  Add progress bar to SIM Manager #18550

Thanks for creating the bugs according to the regressions found when testing. R+ from my side. Thanks Guillaume!

Comment 29

[Guillaume Marty [:gmarty]]

https://github.com/mozilla-b2g/gaia/commit/45b983c5e27e1007f356276914ad116d20992dcd