1031652 - [tarako][dolphin] A warn message should be shown when dialing a normal number from lockscreen

Status: RESOLVED FIXED

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

Description

[Wei Gao (Spreadtrum)]

OS version
---------------------------------------------
FireFoxOS v1.3t v1.4

Reproduce steps:
---------------------------------------------
1) turn on lockscreen with a password in it.
2) lock the screen.
3) dial a normal number which is not an emergency number from lockscreen.

Expected result:
---------------------------------------------
Show a warn message

Actual result:
---------------------------------------------
nothing happened

Probability:
---------------------------------------------
Always Recurrence

Comment 1

[Wei Gao (Spreadtrum)]

Attachment: show_message_when_dial_on_lockscreen.patch

Hi Etienne

This is a patch for this.
Can you help to check this function request?

Thanks a great.

Comment 2

[Etienne Segonzac (:etienne)]

Comment on attachment 8447605 [details] [diff] [review]
show_message_when_dial_on_lockscreen.patch

Review of attachment 8447605 [details] [diff] [review]:
-----------------------------------------------------------------

First things first, why isn't that part of the error handling in telephony_helper.js.
Do we have a vendcom ril specific issue here?

Comment 3

[Wei Gao (Spreadtrum)]

(In reply to Etienne Segonzac (:etienne) from comment #2)
> First things first, why isn't that part of the error handling in
> telephony_helper.js.

Hi Etienne

As the dial panel from lockscreen is in system app, I think this error handling can only be added in system.
How do you think about it?
Thanks a lot.

Comment 4

[Joe Cheng [:jcheng] (please needinfo)]

does it happen on the flame or another vendcom RIL? QAWANTED

Comment 5

[Wei Gao (Spreadtrum)]

(In reply to Joe Cheng [:jcheng] from comment #4)
> does it happen on the flame or another vendcom RIL? QAWANTED

Yes, it happen on the flame.
Thanks.

Comment 6

[Joe Cheng [:jcheng] (please needinfo)]

i tend to believe that it has been working like this for the whole time
ni? Carrie for the expected behavior

as long as the emergency call can be made (which is the most important), not having feedback on making normal calls is much less severe. 

moving this to 1.4? see if dolphin needs this

Thanks

Comment 7

[Carrie Wang [New account: [e-mail]]]

I think we can pop up a warning window which shows:

title:
Emergency call only
---------------------------
string:
<numbers> is not an emergency number.


Thanks!

Comment 8

[Wei Gao (Spreadtrum)]

Attachment: show_warn_message_when_dial_on_lockscreen_2.patch

(In reply to Carrie Wang [:carrie] from comment #7)
> I think we can pop up a warning window which shows:
> 
> title:
> Emergency call only
> ---------------------------
> string:
> <numbers> is not an emergency number.

That's cool.
I changed the patch, please help to review it. 
But it still need two new l10n keys.
Thanks.

Comment 9

[Ivan Tsay (:ITsay)]

This is the feature request and won't take it in v1.4. Nominate to 2.1

Comment 10

[Etienne Segonzac (:etienne)]

Please don't forget to set the review flag otherwise I might miss it :)
I'll have a look tomorrow!

Comment 11

[Wei Gao (Spreadtrum)]

(In reply to Etienne Segonzac (:etienne) from comment #10)
> Please don't forget to set the review flag otherwise I might miss it :)

Oh, I am sorry I forget this flag again. 
I will pay attention to it afterwards.
Thanks.

Comment 12

[Etienne Segonzac (:etienne)]

Comment on attachment 8450792 [details] [diff] [review]
show_warn_message_when_dial_on_lockscreen_2.patch

Review of attachment 8450792 [details] [diff] [review]:
-----------------------------------------------------------------

::: apps/system/emergency-call/js/dialer.js
@@ +28,5 @@
>            this._installHandlers(call);
> +        }.bind(this)).catch(function(errorName) {
> +          var _ = navigator.mozL10n.get;
> +          this._emergencyAlertTitle.textContent = _('Emergency call only');
> +          this._emergencyAlertMsg.textContent = sanitizedNumber + _('is not an emergency number.');

can you set `data-l10n-id`s in index.html so we just have to toggle _emergencyAlert.hidden?

Comment 13

[Wei Gao (Spreadtrum)]

Attachment: show_warn_message_when_dial_on_lockscreen.patch

Hi Etienne

Nice to see you here again. 
First of all, thank you for your patience to me on another bug.

I have modified the patch, Could you help to review it.
Thanks a great again.

Comment 14

[Etienne Segonzac (:etienne)]

Comment on attachment 8453702 [details] [diff] [review]
show_warn_message_when_dial_on_lockscreen.patch

Review of attachment 8453702 [details] [diff] [review]:
-----------------------------------------------------------------

The code looks good!
we'll probably want a ui-review too, can you attach screenshots?

Comment 15

[Wei Gao (Spreadtrum)]

Attachment: emergency-call-warn.png

Hi Etienne

This is a screenshots of the emergency call warn.
Please help to check it.
Thanks.

Comment 16

[Wei Gao (Spreadtrum)]

Attachment: V40711-091314.mp4

There is one thing strange about 'OK' button.
When I click 'OK', the number above the keyboard will disappear, but I didn't let it do so.
Through my testing, I found the "CallHandler.init();" will execute as soon as I click 'OK' button. That is to say, the html of emergency call restart.
I don't know why.

Hi Etienne
Could you help me?
Thanks a lot.

Comment 17

[Etienne Segonzac (:etienne)]

Cleaning the phone number after the call is expected: https://github.com/mozilla-b2g/gaia/blob/master/apps/emergency-call/js/dialer.js#L23

And now that you mention it, we might want to move the CallHandler.init in the load callback next to Keypad.init

Comment 18

[Etienne Segonzac (:etienne)]

Comment on attachment 8454192 [details]
emergency-call-warn.png

Hey Carrie, not sure who should ui-review this, maybe you?

Comment 19

[Etienne Segonzac (:etienne)]

Comment on attachment 8453702 [details] [diff] [review]
show_warn_message_when_dial_on_lockscreen.patch

Review of attachment 8453702 [details] [diff] [review]:
-----------------------------------------------------------------

Clearing the review flag while we figure out the l10n details.

::: apps/system/emergency-call/index.html
@@ +20,5 @@
> +    <form role="dialog" data-type="confirm" id="emergencyAlert" hidden>
> +      <section>
> +        <h1 id="emergencyAlert-title" data-l10n-id="emergency-call-only">Emergency call only</h1>
> +        <p id="emergencyAlert-msg" data-l10n-id="not-emergency-call">
> +          <a id="dial-number"></a> is not an emergency number.

Does the "not-emergency-call" l10n key exist?

Anyway, we should use an l10n key with a parameter instead of the <a id="dial-number">, you'll find an example of an l10n key with parameters here: https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/dialer/js/telephony_helper.js#L221

Comment 20

[Carrie Wang [New account: [e-mail]]]

Comment on attachment 8454192 [details]
emergency-call-warn.png

Hi, 

so close! According to the building blocks, the title "Emergency call only" should be title case so that will be "Emergency Call Only".

Comment 21

[Wei Gao (Spreadtrum)]

Attachment: show_warn_message_when_dial_on_lockscreen.patch

Hi Etienne

I have added l10n keys in it. 
But I wonder if 'data-l10n-id' should be added in <p id="emergencyAlert-msg"></p> as a {{number}} is needed for it. So I use below code instead.
+          this._emergencyMsg.textContent = _("emergency-call-error",
+                                             {number: sanitizedNumber});

Can it be granted?
Thanks a lot.

Comment 22

[Wei Gao (Spreadtrum)]

Attachment: emergency-call-warn.png

Hi Carrie

I have modified this UI. Is this what we need?
Thanks.

Comment 23

[Etienne Segonzac (:etienne)]

Comment on attachment 8454862 [details] [diff] [review]
show_warn_message_when_dial_on_lockscreen.patch

Review of attachment 8454862 [details] [diff] [review]:
-----------------------------------------------------------------

::: apps/system/emergency-call/js/dialer.js
@@ +25,4 @@
>        if (promiseOrCall && promiseOrCall.then) {
>          promiseOrCall.then(function(call) {
>            this._installHandlers(call);
> +        }.bind(this)).catch(function(errorName) {

I think  we need to put this inside a |navigator.mozL10n.once(function() {})| block

@@ +26,5 @@
>          promiseOrCall.then(function(call) {
>            this._installHandlers(call);
> +        }.bind(this)).catch(function(errorName) {
> +          var _ = navigator.mozL10n.get;
> +          this._emergencyMsg.textContent = _("emergency-call-error",

nit: single quotes |'|

::: apps/system/locales/system.en-US.properties
@@ +5,5 @@
>  close=Close
>  skip=Skip
>  
> +# emergency call on lockscreen
> +emergency-call-only=Emergency Call Only

I don't think the emergency-call app is loading this l10n file.
We probably want to create an l10n file for the emergency-call app.

Comment 24

[Wei Gao (Spreadtrum)]

(In reply to Etienne Segonzac (:etienne) from comment #23)
> Comment on attachment 8454862 [details] [diff] [review]
> show_warn_message_when_dial_on_lockscreen.patch
> 
> Review of attachment 8454862 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: apps/system/emergency-call/js/dialer.js
> @@ +25,4 @@
> >        if (promiseOrCall && promiseOrCall.then) {
> >          promiseOrCall.then(function(call) {
> >            this._installHandlers(call);
> > +        }.bind(this)).catch(function(errorName) {
> 
> I think  we need to put this inside a |navigator.mozL10n.once(function()
> {})| block

Dear  Etienne
What's |navigator.mozL10n.once| function? I can't understand it.
Do you mean I should use |LazyL10n.get(function localized(_) {}| ?

> ::: apps/system/locales/system.en-US.properties
> @@ +5,5 @@
> >  close=Close
> >  skip=Skip
> >  
> > +# emergency call on lockscreen
> > +emergency-call-only=Emergency Call Only
> 
> I don't think the emergency-call app is loading this l10n file.
> We probably want to create an l10n file for the emergency-call app.

I found |<link rel="resource" type="application/l10n" href="/locales/locales.ini">| in index.html of emergency-call.
I guess emergency-call app can load |apps/system/locales/system.en-US.properties|.
How do you think about it?

Thanks a great.

Comment 25

[Etienne Segonzac (:etienne)]

(In reply to Wei Gao (Spreadtrum) from comment #24)
> (In reply to Etienne Segonzac (:etienne) from comment #23)
> > Comment on attachment 8454862 [details] [diff] [review]
> > show_warn_message_when_dial_on_lockscreen.patch
> > 
> > Review of attachment 8454862 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: apps/system/emergency-call/js/dialer.js
> > @@ +25,4 @@
> > >        if (promiseOrCall && promiseOrCall.then) {
> > >          promiseOrCall.then(function(call) {
> > >            this._installHandlers(call);
> > > +        }.bind(this)).catch(function(errorName) {
> > 
> > I think  we need to put this inside a |navigator.mozL10n.once(function()
> > {})| block
> 
> Dear  Etienne
> What's |navigator.mozL10n.once| function? I can't understand it.
> Do you mean I should use |LazyL10n.get(function localized(_) {}| ?

The emergency call app isn't using the LazyL10n, it's using the l10n lib directly.
navigator.mozl10n.once() make sure your function is called when l10n is ready.

> 
> > ::: apps/system/locales/system.en-US.properties
> > @@ +5,5 @@
> > >  close=Close
> > >  skip=Skip
> > >  
> > > +# emergency call on lockscreen
> > > +emergency-call-only=Emergency Call Only
> > 
> > I don't think the emergency-call app is loading this l10n file.
> > We probably want to create an l10n file for the emergency-call app.
> 
> I found |<link rel="resource" type="application/l10n"
> href="/locales/locales.ini">| in index.html of emergency-call.
> I guess emergency-call app can load
> |apps/system/locales/system.en-US.properties|.
> How do you think about it?

It's probably worth it to have a separate file anyway.

Thanks!

Comment 26

[Wei Gao (Spreadtrum)]

Attachment: show_warn_message_when_dial_on_lockscreen.patch

Dear Etienne

I have modified the patch, but when I used |navigator.mozL10n.once|, the callback can not be executed, I don't know why. Then I used |navigator.mozL10n.ready| instead, can it be granted?

Please help to review.
Thanks a lot.

Comment 27

[Etienne Segonzac (:etienne)]

Comment on attachment 8459094 [details] [diff] [review]
show_warn_message_when_dial_on_lockscreen.patch

Review of attachment 8459094 [details] [diff] [review]:
-----------------------------------------------------------------

Are you basing your patch on gaia master?

The emergency app should be in apps/emergency-call and not apps/system/emergency-call... and then mozL10n.once should work (we're encouraged to use once() instead of ready(), it landed in bug 993189).
Also we probably want to name the new l10n file emergency-call.en-US.properties, not system.

Comment 28

[Wei Gao (Spreadtrum)]

Attachment: emergencycall-warn.patch

Dear Etienne

I am sorry for I used v1.4 to make that patch.
I have modified the patch on mater again.
Could you help to review again?
Thanks a great.

Comment 29

[Etienne Segonzac (:etienne)]

Comment on attachment 8459563 [details] [diff] [review]
emergencycall-warn.patch

Review of attachment 8459563 [details] [diff] [review]:
-----------------------------------------------------------------

Looking good!
Please open a gaia pull request so we can see the gaia-try results.

Comment 30

[Wei Gao (Spreadtrum)]

Attachment: [master] pull request

Dear Etienne

This is a RP for master.
Please help to review it.

By the way, as |navigator.mozL10n.once| is not defined on v1.4, can |navigator.mozL10n.ready| be granted on v1.4?

Thanks a great.

Comment 31

[Wei Gao (Spreadtrum)]

Dear Etienne

There is only emergency-call.en-US.properties in locales folder, do we need other properties files of other languages? How can we add that?

By the way, as |navigator.mozL10n.once| is not defined on v1.4, can |navigator.mozL10n.ready| be granted on v1.4?

Thanks.

Comment 32

[Kai-Chih Hu [:khu]]

Looks like we got R+ already. Mark this bug as target milestone = sprint 2 now. If I misunderstood, please feel free to change it. Thanks.

Comment 33

[Kai-Chih Hu [:khu]]

Wei, I think you could be the owner for this bug, right?

Comment 34

[Etienne Segonzac (:etienne)]

> By the way, as |navigator.mozL10n.once| is not defined on v1.4, can
> |navigator.mozL10n.ready| be granted on v1.4?
> 

Please open a pull request against 1.4 and ask me for review on it.

Comment 35

[Etienne Segonzac (:etienne)]

Comment on attachment 8460804 [details] [review]
[master] pull request

This PR is ready to land, just asking Francesco for a quick feedback since we're introducing a new l10n file and I'm always worried to do it wrong.

The try issues look like intermittents, I've restarted the jobs just to make sure.

Comment 36

[Francesco Lodolo [:flod]]

Comment on attachment 8460804 [details] [review]
[master] pull request

Looks good to me (also tested, works as expected).

Side note: we can't fix this on < 2.0, given the new strings (I read comments about 1.4).

Comment 37

[Wei Gao (Spreadtrum)]

(In reply to Etienne Segonzac (:etienne) from comment #34)
> > By the way, as |navigator.mozL10n.once| is not defined on v1.4, can
> > |navigator.mozL10n.ready| be granted on v1.4?
> > 
> 
> Please open a pull request against 1.4 and ask me for review on it.

OK, I will open a PR right now.
Thanks for your suggestion.

Comment 38

[Wei Gao (Spreadtrum)]

Attachment: [v1.4] pull request

Dear Etienne

Welcome to come back. :)
This is a RP for v1.4, please help to review it.

Thanks a great.

Comment 39

[Francesco Lodolo [:flod]]

Comment on attachment 8472028 [details] [review]
[v1.4] pull request

Just to make comment 36 more explicit: you can't add new strings on string frozen branches (i.e. anything besides master right now).

Comment 40

[Wei Gao (Spreadtrum)]

(In reply to Francesco Lodolo [:flod] from comment #39)
> Comment on attachment 8472028 [details] [review]
> [v1.4] pull request
> 
> Just to make comment 36 more explicit: you can't add new strings on string
> frozen branches (i.e. anything besides master right now).

Oh, I am sorry for missing it.
Thank Francesco very much, I will modify it.

Comment 41

[Wei Gao (Spreadtrum)]

Hi Francesco

By the way, when can we add new strings on those branches again?

Comment 42

[Francesco Lodolo [:flod]]

(In reply to Wei Gao (Spreadtrum) from comment #41)
> By the way, when can we add new strings on those branches again?

You can't, exactly like you can't add features past a certain date on those branches.
https://wiki.mozilla.org/Release_Management/B2G_Landing#Versions_and_Scheduling

Comment 43

[Wei Gao (Spreadtrum)]

Dear Etienne

As new strings can not be added on v1.4 now, how can we realize the UI of warn message on v1.4?
Could you give some advice?
Thanks.

Comment 44

[Etienne Segonzac (:etienne)]

(In reply to Wei Gao (Spreadtrum) from comment #43)
> Dear Etienne
> 
> As new strings can not be added on v1.4 now, how can we realize the UI of
> warn message on v1.4?
> Could you give some advice?
> Thanks.

Sadly I don't think we'll be able to land this on 1.4.

Comment 45

[Etienne Segonzac (:etienne)]

https://github.com/mozilla-b2g/gaia/commit/6c88fd82c3b837da61f706c9bdbfa768fe065ff4

Landed on master.

Comment 47

[Sherman Chen [:chens] (inactive)]

[Blocking Requested - why for this release]: 2.0 blocker per bug 1082998 comment 24

Comment 48

[Sherman Chen [:chens] (inactive)]

Attachment: Pull request to 2.0

Hi Etienne, 

As discussed in bug 1082998, this patch resolved conflicts and it's for 2.0 branch , would you take a look and give some feedback? thanks!

Comment 49

[Sherman Chen [:chens] (inactive)]

This patch should solve the issue on 2.0, but as :flod described in comment 39, I think 2.0 branch also in string freeze stage. If this is really a partner blocker, I would suggest partner take care translation work so it can show properly translated string.

Comment 50

[Etienne Segonzac (:etienne)]

Comment on attachment 8517366 [details] [review]
Pull request to 2.0

I don't understand why the l10n part of this patch is different from the 1.4 and the master version. Did you encounter an issue while uplifting?

Comment 51

[Sherman Chen [:chens] (inactive)]

Yes, there was no corresponding strings and no 'emergency-call.en-US.properties' back in 2.0 branch. In 2.0  still use locale.ini to import shared l10n files from dialer, but it's been replaced since 2.1 and has its own l10n file. That's all the difference comes.

Comment 52

[Etienne Segonzac (:etienne)]

Comment on attachment 8517366 [details] [review]
Pull request to 2.0

Thanks, all clear.
Flagging the l10n team since I guess this should be on their radar.

Comment 53

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8517366 [details] [review]
Pull request to 2.0

Flod?

Comment 54

[Francesco Lodolo [:flod]]

Comment on attachment 8517366 [details] [review]
Pull request to 2.0

It's an f- for me, adding strings on 2.0 at this point doesn't make any sense.

But what you actually need is release drivers' opinion: it's up to them to evaluate how blocking this is, and the risks involved.

The only alternative I see is to load the strings we have in system.properties (we have "OK" and "Emergency calls only"), but I don't know what issues this could create in term of performances.

The only strings in common between the two files are: 'cancel', 'close', 'unlock-a11y-button.ariaLabel', 'longDateFormat', 'resume'. So I don't see risks of overwriting relevant strings.

Comment 55

[bhavana bajaj [:bajaj]]

(In reply to Francesco Lodolo [:flod] from comment #54)
> Comment on attachment 8517366 [details] [review]
> Pull request to 2.0
> 
> It's an f- for me, adding strings on 2.0 at this point doesn't make any
> sense.
> 
> But what you actually need is release drivers' opinion: it's up to them to
> evaluate how blocking this is, and the risks involved.
> 
> The only alternative I see is to load the strings we have in
> system.properties (we have "OK" and "Emergency calls only"), but I don't
> know what issues this could create in term of performances.
> 
> The only strings in common between the two files are: 'cancel', 'close',
> 'unlock-a11y-button.ariaLabel', 'longDateFormat', 'resume'. So I don't see
> risks of overwriting relevant strings.

I agree on not landing this on 2.0 or 2.1 at this point, please refer to https://bugzilla.mozilla.org/show_bug.cgi?id=1082998#c34.

Comment 56

[Wesly Huang (EPM)]

[Triage] considering this timing of 2.0 and 2.1, de-nom for them and nom. to 2.2 for consideration.

Comment 57

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8517366 [details] [review]
Pull request to 2.0

removing myself from f? until we get a go ahead from flod on new strings.