Closed Bug 881957 Opened 7 years ago Closed 6 years ago

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

Categories

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

x86_64
Windows 7
defect

Tracking

(blocking-b2g:1.3+, b2g-v1.3 fixed)

VERIFIED FIXED
blocking-b2g 1.3+
Tracking Status
b2g-v1.3 --- fixed

People

(Reporter: dpalomino, Assigned: dwi2)

References

Details

(Keywords: late-l10n, Whiteboard: [leo-pre-iot-br])

Attachments

(3 files)

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
Lockscreen or SIM PIN?
Flags: needinfo?(dpv)
By asking whether or not this can be implemented, I'm guessing we don't think this is a regression from v1.0.1
blocking-b2g: leo? → leo+
What is the requirement here? Which PIN do you want to change?
Assignee: nobody → alive
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.
Flags: needinfo?(dpv)
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.
ni? from UX to decide what's necessary.
I personal don't think this is a bug...
Flags: needinfo?(firefoxos-ux-bugzilla)
Nominate non-blocking.
blocking-b2g: leo+ → leo?
Priority: -- → P3
Triage - not blocking for leo from deciding partners. nominate for koi?
blocking-b2g: leo? → koi?
This is non-blocking but assigning to Francis to address in future since PIN/lock is his domain.
Flags: needinfo?(firefoxos-ux-bugzilla) → needinfo?(fdjabri)
Attached image 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.
Flags: needinfo?(fdjabri)
Not taking features for v1.2 now.
blocking-b2g: koi? → 1.3?
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.
Assignee: alive → arthur.chen
blocking-b2g: 1.3? → 1.3+
Flags: needinfo?(arthur.chen)
Tzu-lin will help on this. Thanks!
Flags: needinfo?(arthur.chen)
Assignee: arthur.chen → 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.
Flags: needinfo?(mtsai)
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.-
Flags: needinfo?(mtsai)
Attached file 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
Attachment #832056 - Flags: review?(ehung)
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?
Attachment #832056 - Flags: review?(ehung) → feedback+
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.
Depends on: 916059
Depends on: 940162
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
Attachment #832056 - Flags: review?(ehung)
Comment on attachment 832056 [details] [review]
pull request

clear review flag since there will be more modification in this patch if bug 940162 landed
Attachment #832056 - Flags: review?(ehung) → review-
Comment on attachment 832056 [details] [review]
pull request

Hi Evelyn,

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

Thanks
Attachment #832056 - Flags: review- → review?(ehung)
Blocks: 941551
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 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.
Attachment #832056 - Flags: feedback-
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
Attachment #832056 - Flags: review?(ehung) → review?(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?
Flags: needinfo?(timdream)
Flags: needinfo?(21)
(In reply to Fabien Cazenave [:kaze] from comment #25)
> (see the 'hidden' class name)

s/hidden/visible/
(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.
Flags: needinfo?(timdream)
(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?
Flags: needinfo?(21)
(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
URL: G
dwi2, are you still working on this bug?
Flags: needinfo?(tzhuang)
Actually, we should consider this as a late feature at this stage so maybe this should not go to 1.3.
blocking-b2g: 1.3+ → 1.3?
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
Flags: needinfo?(tzhuang) → needinfo?(kaze)
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
Moving back to blocking+ per comment 33.
blocking-b2g: 1.3? → 1.3+
(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.
Flags: needinfo?(itsay)
Flags: needinfo?(bhuang)
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.
Flags: needinfo?(bhuang)
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
Attachment #832056 - Flags: review?(kaze) → review?(arthur.chen)
Flags: needinfo?(kaze)
(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.
Flags: needinfo?(itsay)
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.
Attachment #832056 - Flags: review?(arthur.chen)
(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.
blocking-b2g: 1.3+ → 1.3?
Keywords: late-l10n
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.
Axel,

Do you see this as an issue
Flags: needinfo?(l10n)
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.
Flags: needinfo?(l10n)
ni to Beatriz Rodríguez to confirm if it would be a blocker for v1.3 certification process. Thanks!
Flags: needinfo?(brg)
(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.
Flags: needinfo?(clee)
Chris mentioned offline we need to keep this as a blocker & manage the l10n risk here.
blocking-b2g: 1.3? → 1.3+
Flags: needinfo?(clee)
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.
Attachment #832056 - Flags: review?(arthur.chen)
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.
Attachment #832056 - Flags: review?(arthur.chen)
Comment on attachment 832056 [details] [review]
pull request

Hi Arthur,

Comments addressed, please help to review it again. Thanks
Attachment #832056 - Flags: review?(arthur.chen)
Thanks for your understanding and good work.
Flags: needinfo?(brg)
Comment on attachment 832056 [details] [review]
pull request

r=me. Thanks!
Attachment #832056 - Flags: review?(arthur.chen) → review+
travis green light
https://travis-ci.org/mozilla-b2g/gaia/builds/17267109

landed on master

https://github.com/mozilla-b2g/gaia/commit/a013a2bb1d3f79d1312e51c022545d8eaa060721
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Uplifted a013a2bb1d3f79d1312e51c022545d8eaa060721 to:
v1.3: 7b649d33ed0afaf8e74f5cf18214aea4ad60cb2b
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.
Status: RESOLVED → VERIFIED
(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
You need to log in before you can comment on or make changes to this bug.