881957 - [leo-pre-iot-br] No confirmation message when changing PIN code

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia::Settings, defect, P3)

Description

[David Palomino [:dpalomino]]

Device: leo
build 06/07:
Gecko revision : 08e8a7bbebec924c24a5187a3c0b2ef1ef6d08da
Gaia revision : e2346ca0297f40547e64a7eebc4bd2e4a4cdaf86
QC commercial RIL : AU_LINUX_GECKO_ICS_STRAWBERRY.01.01.00.019.115

There is no confirmation message (saying, ok, that's been properly modified) when the user tries to change PIN code. 

This is requested for Brazil. Is it sommething easy to implement?

Thanks!
David

Comment 1

[Alex Keybl [:akeybl]]

Lockscreen or SIM PIN?

Comment 2

[Alex Keybl [:akeybl]]

By asking whether or not this can be implemented, I'm guessing we don't think this is a regression from v1.0.1

Comment 3

[(inactive after 6/18) Alive Kuo [:alive]]

What is the requirement here? Which PIN do you want to change?

Comment 4

[Beatriz Rodríguez [:brg]]

Settings -> SIM Security -> Change PIN
If you change the PIN successfuly there is not any visual confirmation for the user.

I have checked that this behaviour with ikura v1.0.1 and there is no visual confirmation for the user either.

Comment 5

[(inactive after 6/18) Alive Kuo [:alive]]

Observation:
1) Android(Galaxy S2): Change SIM PIN -> a [SIM PIN Changed successfully] banner shows for a while.
2) iOS: Change SIM PIN -> Nothing happens after pressing saving in changing SIM PIN dialog.

Comment 6

[(inactive after 6/18) Alive Kuo [:alive]]

ni? from UX to decide what's necessary.
I personal don't think this is a bug...

Comment 7

[(inactive after 6/18) Alive Kuo [:alive]]

Nominate non-blocking.

Comment 8

[Wayne Chang [:wchang]]

Triage - not blocking for leo from deciding partners. nominate for koi?

Comment 9

[Stephany Wilkes]

This is non-blocking but assigning to Francis to address in future since PIN/lock is his domain.

Comment 10

[Francis Djabri [:djabber]]

Attachment: SIM PIN confirmation

(In reply to Alive Kuo [:alive] from comment #5)
> Observation:
> 1) Android(Galaxy S2): Change SIM PIN -> a [SIM PIN Changed successfully]
> banner shows for a while.
> 2) iOS: Change SIM PIN -> Nothing happens after pressing saving in changing
> SIM PIN dialog.

Let's follow the Android approach and show an alert banner for 3 seconds.

Comment 11

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Not taking features for v1.2 now.

Comment 12

[Ivan Tsay (:ITsay)]

Plus this one for better user experience.

Hi Arthur, please help take a look this bug. If you are overloaded please resign this one to other setting developer.

Comment 13

[Arthur Chen [:arthurcc][inactive after 6/30]]

Tzu-lin will help on this. Thanks!

Comment 14

[Tzu-Lin Huang [:dwi2][:tzhuang]]

We need some UX input about this.

Currently there is no toaster implementation in gaia. 
It will require non-trivial effort to implement toaster in system app and it certainly have impact to stability of system app.
In my opinion, considering the benefit of user experience improvement and the effort of toaster implementation, it might be too risky to do this.

Comment 15

[GoodMike]

Ux team suggest to have Status (as a toast) to inform users instead of a confirm window. see Building Blocks https://developer.mozilla.org/en-US/Apps/Design/Building_Blocks
-Status : Relays information to the user in a transitory fashion, typically to confirm a user action or to alert the user to a system event.-

Comment 16

[Tzu-Lin Huang [:dwi2][:tzhuang]]

Attachment: pull request

Hi Evelyn,

Please kindly help to review this patch, thanks.
This patch includes two things:
1. Implement Settings-wide toaster, basically following design of building block at https://developer.mozilla.org/en-US/Apps/Design/Building_Blocks#Status  
2. Display toast when user successfully change PIN

Comment 17

[Evelyn Hung]

Comment on attachment 832056 [details] [review]
pull request

I think the patch is good, but can we move the toast part to shared/ so every app can reuse it instead of making their own wheel?

Comment 18

[Tzu-Lin Huang [:dwi2][:tzhuang]]

I moved toaster to shared/ but 'Change PIN' is broken due to the patch of Bug 916059, set up dependency.
I'll set review flag once 'Change PIN' fixed.

Comment 19

[Tzu-Lin Huang [:dwi2][:tzhuang]]

Comment on attachment 832056 [details] [review]
pull request

Hi Evelyn,

Please help to review this patch.

I moved toaster to shared folder and added unit tests for it.

But the patch of bug 916059 broke the functionality of Change SIM PIN (I filed bug 940162 to track it). I suggest that we should not land this patch until bug 940162 is fixed.

Thanks

Comment 20

[Tzu-Lin Huang [:dwi2][:tzhuang]]

Comment on attachment 832056 [details] [review]
pull request

clear review flag since there will be more modification in this patch if bug 940162 landed

Comment 21

[Tzu-Lin Huang [:dwi2][:tzhuang]]

Comment on attachment 832056 [details] [review]
pull request

Hi Evelyn,

Please kindly help to review this patch since bug 940162 is landed.

Thanks

Comment 22

[Fabien Cazenave [:kaze]]

It seems wrong to me that we use toasters to notify the SIM PIN code is changed but not for SIM PIN2, or enabling FDN, or possibly a few other settings. I’ve just filed bug 941551 for that.

Comment 23

[Fabien Cazenave [:kaze]]

Comment on attachment 832056 [details] [review]
pull request

Looks good, but I’d prefer to rely on the navigator.mozL10n.localize() method and pass message IDs to the Toaster object rather than assigning raw strings to .innerHTML.

Comment 24

[Tzu-Lin Huang [:dwi2][:tzhuang]]

Comment on attachment 832056 [details] [review]
pull request

Hi Kaze,

I've modified the pull request to address your comment. 
Please help to review my patch since Evelyn is busy and you have already spend some time in the pull request.

Thanks

Comment 25

[Fabien Cazenave [:kaze]]

I hadn’t realized the `toaster.js` file was intended to be a shared lib. This worries me a bit because it requires a specific stylesheet to work (see the 'hidden' class name), shared/style/status.css — and creating a dependency between a shared JS lib and a shared CSS sheet is one of the main trap we wanted to avoid for this shared/ folder.

In other words, it looks a bit too much like a web component for me. That’s fine in a single app but I’m worried to land this as a shared resource.

Vivien, do you think it’s OK to create such a shared component?
Tim, will you reuse such a lib to replace the declarative toasters in the System app?

Comment 26

[Fabien Cazenave [:kaze]]

(In reply to Fabien Cazenave [:kaze] from comment #25)
> (see the 'hidden' class name)

s/hidden/visible/

Comment 27

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Fabien Cazenave [:kaze] from comment #25)
> I hadn’t realized the `toaster.js` file was intended to be a shared lib.
> This worries me a bit because it requires a specific stylesheet to work (see
> the 'hidden' class name), shared/style/status.css — and creating a
> dependency between a shared JS lib and a shared CSS sheet is one of the main
> trap we wanted to avoid for this shared/ folder.

I don't think this is an issue.

> In other words, it looks a bit too much like a web component for me. That’s
> fine in a single app but I’m worried to land this as a shared resource.

We could always move our shared resources to web components (or a polyfill) when we feel it's ready.

Comment 28

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Fabien Cazenave [:kaze] from comment #25)
> I hadn’t realized the `toaster.js` file was intended to be a shared lib.
> This worries me a bit because it requires a specific stylesheet to work (see
> the 'hidden' class name), shared/style/status.css — and creating a
> dependency between a shared JS lib and a shared CSS sheet is one of the main
> trap we wanted to avoid for this shared/ folder.
> 
> In other words, it looks a bit too much like a web component for me. That’s
> fine in a single app but I’m worried to land this as a shared resource.
> 
> Vivien, do you think it’s OK to create such a shared component?
> Tim, will you reuse such a lib to replace the declarative toasters in the
> System app?

It's a bit sad that there is not a platform way to do that. The main argument I have when it comes to land stuff in shared/js that are tied to shared/style is about the fact that it oftens hides stuff that should be done on the platform side.

I'm a bit curious here. Have we explored some platform native way of doing that with the new notification API?

Comment 29

[Fabien Cazenave [:kaze]]

(In reply to Vivien Nicolas (:vingtetun) (:21) (RTO - Review Time Off until 09/12/13) from comment #28)
> I'm a bit curious here. Have we explored some platform native way of doing
> that with the new notification API?

I hadn’t thought of that. I just did a quick test (that’s a 3-line patch!), it makes sense but there are key differences between notifications and toasters:
 • notifications happen on the system and they’re logged in the System drawer (⇒ when clicked, they bring an app on the foreground);
 • toasters happen within an app (⇒ they’re not visible on background apps)

Maybe we could think of toasters as “transient” notifications: that’s the term Jetpack uses [1], and transient notifications in Gnome-shell are not stacked into the main notification list [2]. We could propose to add a boolean “transient” property to the notification options.

If toasters can’t be seen as transient notifications, then they should be implemented as a shared web component. In this case, relying on a shared JS+CSS is probably the best mid-term solution.

For the very short-term, Neo (UX) is OK to use Notifications instead of toasters for the Settings app, see bug 941551 comment 3:

> As we discussed with Evelyn, We still have not system toast API. So, for
> now, I agree using notification instead of create one toast and just only
> for change password. But this is not for long tern design. We still need
> system toast for many situations. 

So unless the idea of transient notifications makes no sense for toasters, I’d suggest to do a three-line patch to use notifications here for this specific “SIM PIN change” use case for now, and continue the discussion on how to implement toasters.

[1] https://addons.mozilla.org/en-US/developers/docs/sdk/latest/modules/sdk/notifications.html 
[2] http://gfxmonk.net/2011/04/25/tip-transient-notifications-in-notify-send.html

Comment 30

[Tim Guan-tin Chien [:timdream] (please needinfo)]

dwi2, are you still working on this bug?

Comment 31

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Actually, we should consider this as a late feature at this stage so maybe this should not go to 1.3.

Comment 32

[Tzu-Lin Huang [:dwi2][:tzhuang]]

Hi Tim,

I didn't work on it recently.

I had an old patch of it with shared JS implementation of toasters waiting for review. I tried to minimize dependency between JS and css in shared folder according to Kaze's review comment.

However, I still think it is kind of weird to implement this feature in notification since there is no app to go to when user click the notification in utility tray. 
Also I think 'SIM PIN changed successfully' message is not that important to be kept in notification in utility tray.
So I am going to ask Kaze's opinion again if this patch is good enough to land or not. If not, I'll make notification version of it.


Hi Kaze,

I remove the dependency on .visible class. Now display or not display of toast relies on hidden attribute of toast element.
However, if you think this patch still has deep coupling between js and css in shared folder, I'll change to use Notification instead.

Thanks

Comment 33

[David Palomino [:dpalomino]]

Hi, 

This has been requested since 1.0 by most of the carriers, and it was waived with the condition of being implemented in 1.3. We think we should have this solved... 

thks!
David

Comment 34

[Jason Smith [:jsmith]]

Moving back to blocking+ per comment 33.

Comment 35

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Jason Smith [:jsmith] from comment #34)
> Moving back to blocking+ per comment 33.

Does this means carrier partners have the right to overrule feature complete date? I would like PM to weight in here.

Comment 36

[Bruce Huang [:bhuang] (inactive)]

Considering this has been pushed out from leo to koi and now to 1.3 (and has been on the blocker list at one point in 1.3), I would say we address this now to keep it from falling off the radar if David is calling it a certification blocker.

To chime in on comment 32, I agree that a one time toast seems more appropriate as compared to a notification that stays in the title bar.

Comment 37

[Tzu-Lin Huang [:dwi2][:tzhuang]]

Comment on attachment 832056 [details] [review]
pull request

Arthur,

Since Kaze seems to be busy right now, could you help to review this patch? Thanks

Comment 38

[Ivan Tsay (:ITsay)]

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #35)
> (In reply to Jason Smith [:jsmith] from comment #34)
> > Moving back to blocking+ per comment 33.
> 
> Does this means carrier partners have the right to overrule feature complete
> date? I would like PM to weight in here.

I wouldn't say our partner want to alter our FC but just want to bring back the discussion on this issue especially when it can be the blocker of the certification. As mentioned by Bruce, I also agree we can fix this one in 1.3 if it is helpful to resolve partner certification blocker. But the call should be still on us if this change needs big effort or have big impact on v1.3 stability.

Comment 39

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 832056 [details] [review]
pull request

Tzu-lin, thank you for the effort. Please check my comments in github and request the review when ready.

Comment 40

[Jason Smith [:jsmith]]

(In reply to Ivan Tsay (:ITsay) from comment #38)
> (In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from
> comment #35)
> > (In reply to Jason Smith [:jsmith] from comment #34)
> > > Moving back to blocking+ per comment 33.
> > 
> > Does this means carrier partners have the right to overrule feature complete
> > date? I would like PM to weight in here.
> 
> I wouldn't say our partner want to alter our FC but just want to bring back
> the discussion on this issue especially when it can be the blocker of the
> certification. As mentioned by Bruce, I also agree we can fix this one in
> 1.3 if it is helpful to resolve partner certification blocker. But the call
> should be still on us if this change needs big effort or have big impact on
> v1.3 stability.

The risk points here are valid here though - I'd imagine there's l10n risk with a patch like this. I agree we shouldn't be taking features onto 1.3 at this point.

Renominating for more discussion per the drivers discussion about reconsidering blockers that have localization risk.

Comment 41

[Beatriz Rodríguez [:brg]]

We undestand all the concerns, but the deadline for string freeze communicated is 1/21/14. This bug has a patch under improvement after the first review and I hope we can get it on time for v1.3 and avoid future blockers at certification side.
Please set 1.3+ flag back.

Comment 42

[Preeti Raghunath(:Preeti)]

Axel,

Do you see this as an issue

Comment 43

[Axel Hecht [:Pike]]

The feature and l10n freezes were on Dec 9. This bug hasn't kept us from shipping before, and it's not a regression, so I don't think it's OK to land it on 1.3.

Comment 44

[Noemí Freire (:noemi)]

ni to Beatriz Rodríguez to confirm if it would be a blocker for v1.3 certification process. Thanks!

Comment 45

[Jason Smith [:jsmith]]

(In reply to Noemí Freire (:noemi) from comment #44)
> ni to Beatriz Rodríguez to confirm if it would be a blocker for v1.3
> certification process. Thanks!

I think Axel's comment still stands here - there's too much l10n risk with a patch like this. Partners usually need to raise feature blockers for a release to the product team during feature planning for a particular release. However, this was never communicated as needing to be a committed feature by the System Platform team during 1.3 planning. We don't really accept features post feature complete, unless it was planned as a committed feature for that release. The quality team, engineering team, and l10n team are all agreement in here that we shouldn't take this given these factors presented here.

Bumping this up to Chris to make a decision here.

Comment 46

[Jason Smith [:jsmith]]

Chris mentioned offline we need to keep this as a blocker & manage the l10n risk here.

Comment 47

[Tzu-Lin Huang [:dwi2][:tzhuang]]

Comment on attachment 832056 [details] [review]
pull request

Hi Arthur,

Please help to review the patch again, Thanks
I'll squash the commits after review.

Comment 48

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 832056 [details] [review]
pull request

Per the offline discussion, let's add a `transition` flag in the option instead of using MutationObserver and a fixed waiting time.

Comment 49

[Tzu-Lin Huang [:dwi2][:tzhuang]]

Comment on attachment 832056 [details] [review]
pull request

Hi Arthur,

Comments addressed, please help to review it again. Thanks

Comment 50

[Beatriz Rodríguez [:brg]]

Thanks for your understanding and good work.

Comment 51

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 832056 [details] [review]
pull request

r=me. Thanks!

Comment 52

[Tzu-Lin Huang [:dwi2][:tzhuang]]

travis green light
https://travis-ci.org/mozilla-b2g/gaia/builds/17267109

landed on master

https://github.com/mozilla-b2g/gaia/commit/a013a2bb1d3f79d1312e51c022545d8eaa060721

Comment 53

[John Ford [:jhford] CET/CEST Berlin Time]

Uplifted a013a2bb1d3f79d1312e51c022545d8eaa060721 to:
v1.3: 7b649d33ed0afaf8e74f5cf18214aea4ad60cb2b

Comment 54

[isabelrios]

Verified on today's (01/24) 1.3 buri build:
Gecko-d387d39
Gaia-0a79a06

Now there is a layer informing user about the successfully change of the PIN. See screenshot attached.

Comment 55

[isabelrios]

Attachment: Sim changed successfully

Comment 56

[David Palomino [:dpalomino]]

(In reply to Isabel Rios [:isabel_rios] from comment #54)
> Verified on today's (01/24) 1.3 buri build:
> Gecko-d387d39
> Gaia-0a79a06
> 
> Now there is a layer informing user about the successfully change of the
> PIN. See screenshot attached.

Great! Thanks!

David