Closed
Bug 1031652
Opened 10 years ago
Closed 10 years ago
[tarako][dolphin] A warn message should be shown when dialing a normal number from lockscreen
Categories
(Firefox OS Graveyard :: Gaia::System, defect)
Tracking
(feature-b2g:2.1, b2g-v1.3T affected, b2g-v1.4 affected, b2g-v2.0 affected, b2g-v2.0M affected)
People
(Reporter: wei.gao, Assigned: wei.gao)
References
Details
(Whiteboard: [sprd321433])
Attachments
(5 files, 7 obsolete files)
91.67 KB,
image/png
|
cawang
:
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 |
46 bytes,
text/x-github-pull-request
|
flod
:
review-
|
Details | Review |
46 bytes,
text/x-github-pull-request
|
etienne
:
review+
flod
:
feedback-
|
Details | Review |
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•10 years ago
|
||
Hi Etienne This is a patch for this. Can you help to check this function request? Thanks a great.
Flags: needinfo?(etienne)
Assignee | ||
Updated•10 years ago
|
Updated•10 years ago
|
blocking-b2g: --- → 1.3T?
Comment 2•10 years ago
|
||
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•10 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.
Comment 4•10 years ago
|
||
does it happen on the flame or another vendcom RIL? QAWANTED
Keywords: qawanted
Assignee | ||
Comment 5•10 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.
Comment 6•10 years ago
|
||
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)
Comment 7•10 years ago
|
||
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•10 years ago
|
||
(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•10 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?
Updated•10 years ago
|
Attachment #8450792 -
Flags: review?(etienne)
Comment 10•10 years ago
|
||
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•10 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 12•10 years ago
|
||
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•10 years ago
|
||
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 14•10 years ago
|
||
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•10 years ago
|
||
Hi Etienne This is a screenshots of the emergency call warn. Please help to check it. Thanks.
Flags: needinfo?(etienne)
Assignee | ||
Comment 16•10 years ago
|
||
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•10 years ago
|
||
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 18•10 years ago
|
||
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 19•10 years ago
|
||
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 20•10 years ago
|
||
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•10 years ago
|
||
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•10 years ago
|
||
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 23•10 years ago
|
||
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)
Updated•10 years ago
|
Attachment #8454863 -
Flags: review?(cawang) → review+
Assignee | ||
Comment 24•10 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)
Comment 25•10 years ago
|
||
(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•10 years ago
|
||
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 27•10 years ago
|
||
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•10 years ago
|
||
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 29•10 years ago
|
||
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•10 years ago
|
||
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)
Updated•10 years ago
|
Attachment #8460804 -
Flags: review?(etienne) → review+
Assignee | ||
Comment 31•10 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)
Updated•10 years ago
|
blocking-b2g: 2.1? → ---
feature-b2g: --- → 2.1
Comment 32•10 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•10 years ago
|
||
Wei, I think you could be the owner for this bug, right?
Assignee: nobody → wei.gao
Comment 34•10 years ago
|
||
> 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 35•10 years ago
|
||
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 36•10 years ago
|
||
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•10 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•10 years ago
|
||
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 39•10 years ago
|
||
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•10 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•10 years ago
|
||
Hi Francesco By the way, when can we add new strings on those branches again?
Flags: needinfo?(francesco.lodolo)
Comment 42•10 years ago
|
||
(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•10 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)
Comment 44•10 years ago
|
||
(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)
Comment 45•10 years ago
|
||
https://github.com/mozilla-b2g/gaia/commit/6c88fd82c3b837da61f706c9bdbfa768fe065ff4 Landed on master.
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Comment 47•10 years ago
|
||
[Blocking Requested - why for this release]: 2.0 blocker per bug 1082998 comment 24
Comment 48•10 years ago
|
||
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)
Comment 49•10 years ago
|
||
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 50•10 years ago
|
||
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)
Comment 51•10 years ago
|
||
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.
Updated•10 years ago
|
Attachment #8517366 -
Flags: review?(etienne)
Updated•10 years ago
|
Flags: needinfo?(wehuang)
Comment 52•10 years ago
|
||
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 53•10 years ago
|
||
Comment on attachment 8517366 [details] [review] Pull request to 2.0 Flod?
Attachment #8517366 -
Flags: feedback?(francesco.lodolo)
Comment 54•10 years ago
|
||
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-
Comment 55•10 years ago
|
||
(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•10 years ago
|
||
[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?
Updated•10 years ago
|
blocking-b2g: 2.2? → ---
Comment 57•10 years ago
|
||
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.
Description
•