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

RESOLVED FIXED in 2.1 S2 (15aug)

Status

Firefox OS
Gaia::System
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: Wei Gao (Spreadtrum), Assigned: Wei Gao (Spreadtrum))

Tracking

unspecified
2.1 S2 (15aug)
ARM
Gonk (Firefox OS)

Firefox Tracking Flags

(feature-b2g:2.1, b2g-v1.3T affected, b2g-v1.4 affected, b2g-v2.0 affected, b2g-v2.0M affected)

Details

(Whiteboard: [sprd321433])

Attachments

(5 attachments, 7 obsolete attachments)

91.67 KB, image/png
Carrie Wang [New account: carrie75521@gmail.com]
: review+
Details
3.49 KB, patch
etienne
: review+
Details | Diff | Splinter Review
46 bytes, text/x-github-pull-request
etienne
: review+
flod
: feedback+
Details | Review | Splinter Review
46 bytes, text/x-github-pull-request
flod
: review-
Details | Review | Splinter Review
46 bytes, text/x-github-pull-request
etienne
: review+
flod
: feedback-
Details | Review | Splinter Review
(Assignee)

Description

3 years ago
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
(Assignee)

Comment 1

3 years ago
Created attachment 8447605 [details] [diff] [review]
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.
Flags: needinfo?(etienne)
(Assignee)

Updated

3 years ago
status-b2g-v1.3T: --- → affected
status-b2g-v1.4: --- → affected
Whiteboard: [sprd321433]
blocking-b2g: --- → 1.3T?
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?
(Assignee)

Comment 3

3 years ago
(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.
does it happen on the flame or another vendcom RIL? QAWANTED
Keywords: qawanted
(Assignee)

Comment 5

3 years ago
(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.

Updated

3 years ago
Keywords: qawanted
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
blocking-b2g: 1.3T? → 1.4?
Flags: needinfo?(cawang)
I think we can pop up a warning window which shows:

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


Thanks!
Flags: needinfo?(cawang)
(Assignee)

Comment 8

3 years ago
Created attachment 8450792 [details] [diff] [review]
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

3 years ago
This is the feature request and won't take it in v1.4. Nominate to 2.1
blocking-b2g: 1.4? → 2.1?
Attachment #8450792 - Flags: review?(etienne)
Please don't forget to set the review flag otherwise I might miss it :)
I'll have a look tomorrow!
Flags: needinfo?(etienne)
(Assignee)

Comment 11

3 years ago
(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 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?
Attachment #8450792 - Flags: review?(etienne)
(Assignee)

Comment 13

3 years ago
Created attachment 8453702 [details] [diff] [review]
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.
Attachment #8447605 - Attachment is obsolete: true
Attachment #8450792 - Attachment is obsolete: true
Attachment #8453702 - Flags: review?(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?
Attachment #8453702 - Flags: review?(etienne) → review+
(Assignee)

Comment 15

3 years ago
Created attachment 8454192 [details]
emergency-call-warn.png

Hi Etienne

This is a screenshots of the emergency call warn.
Please help to check it.
Thanks.
Flags: needinfo?(etienne)
(Assignee)

Comment 16

3 years ago
Created attachment 8454196 [details]
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.
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
Flags: needinfo?(etienne)
Comment on attachment 8454192 [details]
emergency-call-warn.png

Hey Carrie, not sure who should ui-review this, maybe you?
Attachment #8454192 - Flags: ui-review?(cawang)
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
Attachment #8453702 - Flags: review+
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".
Attachment #8454192 - Flags: ui-review?(cawang) → ui-review-
(Assignee)

Comment 21

3 years ago
Created attachment 8454862 [details] [diff] [review]
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.
Attachment #8453702 - Attachment is obsolete: true
Attachment #8454862 - Flags: review?(etienne)
(Assignee)

Comment 22

3 years ago
Created attachment 8454863 [details]
emergency-call-warn.png

Hi Carrie

I have modified this UI. Is this what we need?
Thanks.
Attachment #8454192 - Attachment is obsolete: true
Attachment #8454863 - Flags: review?(cawang)
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.
Attachment #8454862 - Flags: review?(etienne)
Attachment #8454863 - Flags: review?(cawang) → review+
(Assignee)

Comment 24

3 years ago
(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.
Flags: needinfo?(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!
Flags: needinfo?(etienne)
(Assignee)

Comment 26

3 years ago
Created attachment 8459094 [details] [diff] [review]
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.
Attachment #8454196 - Attachment is obsolete: true
Attachment #8454862 - Attachment is obsolete: true
Attachment #8459094 - Flags: review?(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.
Attachment #8459094 - Flags: review?(etienne)
(Assignee)

Comment 28

3 years ago
Created attachment 8459563 [details] [diff] [review]
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.
Attachment #8459094 - Attachment is obsolete: true
Attachment #8459563 - Flags: review?(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.
Attachment #8459563 - Flags: review?(etienne) → review+
(Assignee)

Comment 30

3 years ago
Created attachment 8460804 [details] [review]
[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.
Attachment #8460804 - Flags: review?(etienne)
Attachment #8460804 - Flags: review?(etienne) → review+
(Assignee)

Comment 31

3 years ago
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.
Flags: needinfo?(etienne)
blocking-b2g: 2.1? → ---
feature-b2g: --- → 2.1

Comment 32

3 years ago
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.
Target Milestone: --- → 2.1 S2 (15aug)

Comment 33

3 years ago
Wei, I think you could be the owner for this bug, right?
Assignee: nobody → wei.gao
> 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.
Flags: needinfo?(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.
Attachment #8460804 - Flags: feedback?(francesco.lodolo)
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).
Attachment #8460804 - Flags: feedback?(francesco.lodolo) → feedback+
(Assignee)

Comment 37

3 years ago
(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.
(Assignee)

Comment 38

3 years ago
Created attachment 8472028 [details] [review]
[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.
Attachment #8472028 - Flags: review?(etienne)
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).
Attachment #8472028 - Flags: review?(etienne) → review-
(Assignee)

Comment 40

3 years ago
(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.
(Assignee)

Comment 41

3 years ago
Hi Francesco

By the way, when can we add new strings on those branches again?
Flags: needinfo?(francesco.lodolo)
(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
Flags: needinfo?(francesco.lodolo)
(Assignee)

Comment 43

3 years ago
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.
Flags: needinfo?(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.
Flags: needinfo?(etienne)
https://github.com/mozilla-b2g/gaia/commit/6c88fd82c3b837da61f706c9bdbfa768fe065ff4

Landed on master.
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Duplicate of this bug: 1082998
[Blocking Requested - why for this release]: 2.0 blocker per bug 1082998 comment 24
blocking-b2g: --- → 2.0?
status-b2g-v2.0: --- → affected
status-b2g-v2.0M: --- → affected
Created attachment 8517366 [details] [review]
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!
Attachment #8517366 - Flags: review?(etienne)
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.
Flags: needinfo?(wehuang)
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?
Attachment #8517366 - Flags: review?(etienne)
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.
Attachment #8517366 - Flags: review?(etienne)
Flags: needinfo?(wehuang)
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.
Attachment #8517366 - Flags: review?(etienne)
Attachment #8517366 - Flags: review+
Attachment #8517366 - Flags: feedback?(gandalf)
Comment on attachment 8517366 [details] [review]
Pull request to 2.0

Flod?
Attachment #8517366 - Flags: feedback?(francesco.lodolo)
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.
Attachment #8517366 - Flags: feedback?(francesco.lodolo) → feedback-
(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.
[Triage] considering this timing of 2.0 and 2.1, de-nom for them and nom. to 2.2 for consideration.
blocking-b2g: 2.0? → 2.2?
blocking-b2g: 2.2? → ---
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.
Attachment #8517366 - Flags: feedback?(gandalf)
You need to log in before you can comment on or make changes to this bug.