1103808 - [Settings][Dialog] sim_security/change_pin should be shown as a dialog

Status: VERIFIED FIXED

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

Description

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

As title, change sim_security/change_pin should be shown as a dialog. We should use settings dialog to show it.

Comment 1

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Attachment: patch on master

Arthur, can you help me review this patch ? In this patch, I tried to use DialogService to show simpin1-dialog and simpin2-dialog. By doing so, we can make it shown as dialog and don't have to rewrite too much on simpin_dialog.js

This is the first stage to refactor simpin_dialog, for the future, we can try to refactor it to make it more readable !

Comment 2

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

Comment on attachment 8542046 [details] [review]
patch on master

Cancel the review first as it would be better to use the same html and panel for all pin types.

Comment 3

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Comment on attachment 8542046 [details] [review]
patch on master

Hey Arthur, I just tried to rewrite the whole patch and tested some basic scenarios and it seems that patch works nice.

Can you give me some feedbacks about this path ? Thanks :)

Comment 4

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Hi Wilson !

I had some questions about gaia-header and can you help to shed some lights on that ? I checked the document on GitHub about gaia-header but I can't find anything about how to disable / hide buttons on gaia-header or how can I easily select buttons from the header and glue some extra logics on it ? (This may not be a good idea because this would break the design principle of web components)

If right now we don't have these kind of supports, I think we can try to add some extra attributes for it to make this happen. And if this needs some extra coding works, I am okay to give it a try :)

Thanks !!

Comment 5

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

(In reply to EJ Chen [:eragonj][:小龍哥][ni? if you need me] from comment #4)
> or how can
> I easily select buttons from the header and glue some extra logics on it ?
> (This may not be a good idea because this would break the design principle
> of web components)

My bad, it's not allowed to do this ! same as what I thought !

Comment 6

[Wilson Page [:wilsonpage]]

(In reply to EJ Chen [:eragonj][:小龍哥][ni? if you need me] from comment #5)
> (In reply to EJ Chen [:eragonj][:小龍哥][ni? if you need me] from comment #4)
> > or how can
> > I easily select buttons from the header and glue some extra logics on it ?
> > (This may not be a good idea because this would break the design principle
> > of web components)
> 
> My bad, it's not allowed to do this ! same as what I thought !

You can absolutely add your own logic to buttons inside gaia-header. Just like any element, the elements inside a custom-element are within your control.

var myButton = gaiaHeader.querySelector('.my-button');

myButton.addEventListener('click', ...);

Comment 7

[Wilson Page [:wilsonpage]]

(In reply to EJ Chen [:eragonj][:小龍哥][ni? if you need me] from comment #5)
> (In reply to EJ Chen [:eragonj][:小龍哥][ni? if you need me] from comment #4)
> > or how can
> > I easily select buttons from the header and glue some extra logics on it ?
> > (This may not be a good idea because this would break the design principle
> > of web components)
> 
> My bad, it's not allowed to do this ! same as what I thought !

You can absolutely add your own logic to buttons inside gaia-header. Just like any element, the elements inside a custom-element are within your control.

<gaia-header>
  <h1>My title</h1>
  <button>My button</button>
</gaia-header>

---

var myButton = gaiaHeader.querySelector('.my-button');
myButton.addEventListener('click', ...);

---

The only exception to this is GaiaHeader's action-button, which is hidden in the shadow-dom. You can respond to clicks on this like:

gaiaHeader.addEventListener('action', ...);

Comment 8

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

(In reply to Wilson Page [:wilsonpage] from comment #7)
> (In reply to EJ Chen [:eragonj][:小龍哥][ni? if you need me] from comment #5)
> > (In reply to EJ Chen [:eragonj][:小龍哥][ni? if you need me] from comment #4)
> > > or how can
> > > I easily select buttons from the header and glue some extra logics on it ?
> > > (This may not be a good idea because this would break the design principle
> > > of web components)
> > 
> > My bad, it's not allowed to do this ! same as what I thought !
> 
> You can absolutely add your own logic to buttons inside gaia-header. Just
> like any element, the elements inside a custom-element are within your
> control.
> 
> <gaia-header>
>   <h1>My title</h1>
>   <button>My button</button>
> </gaia-header>
> 
> ---
> 
> var myButton = gaiaHeader.querySelector('.my-button');
> myButton.addEventListener('click', ...);
> 
> ---
> 
> The only exception to this is GaiaHeader's action-button, which is hidden in
> the shadow-dom. You can respond to clicks on this like:
> 
> gaiaHeader.addEventListener('action', ...);

Yes i know this use case. What I really want to know is whether we can easily use some special attributes to show / hide the elements in shadow DOM ? or what we can do here is to use customized elements to achieve this ?

Comment 9

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Comment on attachment 8542046 [details] [review]
patch on master

Arthur, I just finished needed tests and some cleanups. I think this patch is good enough to be reviewed. Can you help me check it ? Thanks :)

Comment 10

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

Comment on attachment 8542046 [details] [review]
patch on master

Good job! As this is not a patch for refactor so I did not give comments on the architecture. We can file another bug for refactor when needed.

Most of the code is looking good. A general comment would be suggesting to use the promise object returning from the APIs and then we can get rid of the function wrappers. The other comments please refer to the PR, thanks.

Comment 11

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Comment on attachment 8542046 [details] [review]
patch on master

Arthur, all comments were addressed and unit tests were all updated too. In this change, I also noticed that there is a bug about FDN and I fixed that at the same time. Please help me review this patch when you have time, thanks ;)

Comment 12

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

Comment on attachment 8542046 [details] [review]
patch on master

r=me with the comments addressed, thanks!

Comment 13

[Wilson Page [:wilsonpage]]

(In reply to EJ Chen [:eragonj][:小龍哥][ni? if you need me] from comment #8)
> Yes i know this use case. What I really want to know is whether we can
> easily use some special attributes to show / hide the elements in shadow DOM
> ? or what we can do here is to use customized elements to achieve this ?

If you're referring to the 'action-button' in gaia-header you can hide it by simply removing the 'action' attribute:

`header.removeAttribute('action');`

or:

`header.action = null;`

The buttons in the light-dom (eg. your buttons) can be hidden in the usual way.

`myButton.hidden = true;`

or:

`.some-state .my-button { display: none; }`

The former is favoured as this will trigger the mutation observer to re-run the font-fit logic, the latter will not, and require you to call `header.runFontFit()` manually.

Comment 14

[Wilson Page [:wilsonpage]]

Ah, I just remembered, if you want to hide buttons using CSS you will need to use !important.

`.some-state .my-button { display: none !important; }`

This is as short term hack due to <style scoped> specificity, and won't be required when we have shadow-css selectors (:host, ::content, etc) in the platform.

Comment 15

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Coooooool, thanks Wilson.

I just checked some online articles about "::shadow" selectors and it looks promising to give us some chances to control the styling in the future. Thanks for these information about our web-components.

Big thanks :)

Comment 16

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Thanks all, this patch was merged into gaia/master : https://github.com/mozilla-b2g/gaia/commit/5a42fcb9e3e9b0a78ad0c3c780b33ede9a6e730a

Comment 17

[Shally Li（leave from Mozilla）]

This current bug has been verified as "pass" on the latest build of Flame 2.5&master and Aries KK 2.5&master by the STR in comment 0.

Actual results: SIM_security/change_pin in Settings is prompted as a dialog, and you can tap "X" icon to close this dialog.
See attachment: verified_Flame_master.png
Reproduce rate: 0/10


Device: Flame master_512mb (Pass)
Build ID               20151217150205
Gaia Revision          140f6ee998b07b354d1841fed902056179c90100
Gaia Date              2015-12-16 23:09:21
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/f143af51f6e35932927b8ccac2509facbbe7b539
Gecko Version          46.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20151217.182817
Firmware Date          Thu Dec 17 18:28:30 EST 2015
Firmware Version       v18D v4
Bootloader             L1TC000118D0

Device: Aries KK master (Pass)
Build ID               20151217120654
Gaia Revision          140f6ee998b07b354d1841fed902056179c90100
Gaia Date              2015-12-16 23:09:21
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/0711218a018d912036f7d3be2ae2649e213cfb85
Gecko Version          46.0a1
Device Name            aries
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.worker.20151217.112618
Firmware Date          Thu Dec 17 11:26:26 UTC 2015
Bootloader             s1

Device: Flame 2.5_512mb (Pass)
Build ID               20151217164854
Gaia Revision          eeed1451e0e48b63abe3199e4d6906adc2a762d2
Gaia Date              2015-12-17 14:53:36
Gecko Revision         http://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/94905dc59d7286b7fe627afbcddafc495894f08d
Gecko Version          44.0
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.worker.20151217.160156
Firmware Date          Thu Dec 17 16:02:06 UTC 2015
Firmware Version       v18D v4
Bootloader             L1TC000118D0

Device: Aries KK 2.5 (Pass)
Build ID               20151217165758
Gaia Revision          eeed1451e0e48b63abe3199e4d6906adc2a762d2
Gaia Date              2015-12-17 14:53:36
Gecko Revision         http://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/94905dc59d7286b7fe627afbcddafc495894f08d
Gecko Version          44.0
Device Name            aries
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.worker.20151217.160505
Firmware Date          Thu Dec 17 16:05:13 UTC 2015
Bootloader             s1

Comment 18

[Shally Li（leave from Mozilla）]

Attachment: verified_Flame_master.png