panda67231 Reporter
	Description • 10 years ago

[1.Description]:
The "Emergency number" is displayed as "Emergency nu..." and the text colour of "Connecting" and "via SIM1" is close to the background.

Attach the screenshots: EmergencyCall.png
Attach the logs: bugreport_EmergencyCall.txt & logcat_EmergencyCall.txt

Happened time: 4:00PM

[2.Testing Steps]: 
1. Open "dialer" app. 
2. Click on the phone pad icon on the bottom-right. 
3. MO an Emergency call (for example: 112)

[3.Expected Result]: 
3. The "Emergency number" should be displayed completely and the colour of the words "Connecting" and "via SIM1" should not be close to the background.

[4.Actual Result]: 
3. The "Emergency number" is displayed as "Emergency nu..." and the colour of the words "Connecting" and "via SIM1" is close to the background.

[5.Reproduction build]: 
Gaia        7709936aeb21859d1607dbd038489493803bb085
Gecko     https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/5bf038fae0f1
BuildID    20140522160202
Version    30.0

[6.Reproduction Frequency]: Always Recurrence

	Jason Smith [:jsmith]
	Comment 1 • 10 years ago

There's really two bugs here - so let's focus on the emergency call screen not displaying emergency number in full.

Beatriz - If the call screen displays "Emergency nu...", would this end up being a problem in certification?

	Beatriz Rodríguez [:brg]
	Comment 2 • 10 years ago

Sorry, I need to know how it looks like in Spanish to say if it will be a cert blocker or not. Could you please attach the screen in Spanish or let me know the string in Spanish?
Thanks!
Rationale:
- If it is something like "Llam. de emergencia" is ok
- If it is something like "Llamada de e..." is not ok.

	Jason Smith [:jsmith]
	Comment 3 • 10 years ago

Can you get a screenshot of this bug in Spanish?

	panda67231 Reporter
	Comment 4 • 10 years ago

Attached Spanish screenshot

	Jason Smith [:jsmith]
	Comment 5 • 10 years ago

(In reply to Beatriz Rodríguez [:brg] from comment #2)
> Sorry, I need to know how it looks like in Spanish to say if it will be a
> cert blocker or not. Could you please attach the screen in Spanish or let me
> know the string in Spanish?
> Thanks!
> Rationale:
> - If it is something like "Llam. de emergencia" is ok
> - If it is something like "Llamada de e..." is not ok.

Based on the screenshot I'm seeing, this hits your cert blocking criteria, right?

	Beatriz Rodríguez [:brg]
	Comment 6 • 10 years ago

(In reply to Jason Smith [:jsmith] from comment #5) 
> Based on the screenshot I'm seeing, this hits your cert blocking criteria,
> right?

Yes. Thanks for asking!

	Anthony Ricaud (:rik)
	Comment 7 • 10 years ago

I'm pretty sure we already shipped 1.1 and 1.3 with this issue. It should be fixed in bug 1007709 but that won't be uplifted because it's tied to the visual refresh.

QAwanted to check if it reproduces on 1.3.

	Jason Smith [:jsmith]
	Comment 8 • 10 years ago

(In reply to Anthony Ricaud (:rik) from comment #7)
> I'm pretty sure we already shipped 1.1 and 1.3 with this issue. It should be
> fixed in bug 1007709 but that won't be uplifted because it's tied to the
> visual refresh.
> 
> QAwanted to check if it reproduces on 1.3.

For obvious reasons - we can't do that. And that's really bad if we've shipped with this.

	Axel Hecht [:Pike]
	Comment 9 • 10 years ago

This is likely an issue across the board of languages, http://transvision.mozfr.org/string/?entity=apps/communications/dialer/shared.properties:emergencyNumber&repo=gaia_1_4 shows a host of long strings.

Evan added this in bug 870187.

	Francisco Jordano [:arcturus] [:francisco]
	Comment 10 • 10 years ago

Could we fix this as part of the visual refresh and uplift it to 1.4?

	Victoria Gerchinhoren [:vicky]
	Comment 11 • 10 years ago

This bug is invalid after the visual refresh changes due to the following:

1. In visual refresh there's a feature to make the size of the contact's name text (in this case "Emergency number") fluid, so the text will not be truncated. (https://bugzilla.mozilla.org/show_bug.cgi?id=1007709)

2. The background image is the user's wallpaper and there's a gradient overlay on top of the image to help the legibility of the "connecting" text.

Plus, this "emergency call" design was made up in implementation, it never went through a proper visual design process. So there's a new Emergency call design and it is implemented already:  https://bugzilla.mozilla.org/show_bug.cgi?id=993951

Thanks

	Victoria Gerchinhoren [:vicky]
	Comment 12 • 10 years ago

NI to Germán to confirm that this fluid size feature is spread as well in this case.

	Maria Angeles Oteo (:oteo)
	Comment 13 • 10 years ago

Vicky, the issue here is that they want it to be fixed in v1.4 and we can not uplift these Visual Refresh patches to this version so it seems that a specific patch for this bug in 1.4 would be the only option, not sure if it's feasible. As you say, German will have to confirm

	Victoria Gerchinhoren [:vicky]
	Comment 14 • 10 years ago

Ok, this screen has been made up in development, and that's why we're having this kind of issues. This screen should have the wallpaper background and say "Llamada de emergencia" or "emergency call", no need for that wired siren illustration.

	Germán Toro del Valle (:gtorodelvalle) Assignee
	Comment 18 • 10 years ago

BTW, Etienne, notice I prepared a patch to v1.4 to solve the issue since trying to cherry-pick some commit in master could be kind of risky :-)

	Germán Toro del Valle (:gtorodelvalle) Assignee
	Comment 19 • 10 years ago

Comment on attachment 8447957 [details]
21180.html

Hi Anthony, I confirmed that all the original changes were needed to get the desired behaviour. If we do not pass the second parameter of HandledCall.formatPhoneNumber() as falsy, the "Emergency number" text is truncated as "Emergency nu..." (or whatever) when the call changes its state (is connected or hung up, for example). Consequently, https://github.com/mozilla-b2g/gaia/pull/21180/files#diff-24c70b7d83da756ecd2b39d890d1bb63R145 is NOT enough to solve the issue as we discussed via IRC.

So I am back in favor of the original changes. Of course, we can comment any issue you consider appropriate ;) Thanks!

	Germán Toro del Valle (:gtorodelvalle) Assignee
	Comment 20 • 10 years ago

Although I already did it, I can ask Loli to heavily test the patch in the device if you feel more confortable this way :)

	Anthony Ricaud (:rik)
	Comment 21 • 10 years ago

Comment on attachment 8447957 [details]
21180.html

Thanks for the explanation. I'm going to forward the review to Doug since he has fresher experience on font size changes.

The patch still contains the emergency background changes and that's not ok to land. We need the least amount of changes possible.

	Doug Sherk (:drs) (inactive)
	Comment 22 • 10 years ago

Comment on attachment 8447957 [details]
21180.html

I find myself in agreement with Anthony that this is unnecessarily risky, but I have another proposed method to do this which I'm pretty sure would work and is fairly low-risk.

Instead of half-cherry-picking some work that happened in other bugs and didn't get uplifted, I'd rather special case the emergency call header text. We can write a CSS selector that includes the .emergency-active state and forces the contact/number text (in this case, "Emergency number") down to the minimum allowed font size by FSM. This way, we're not depending on risky run-time resizing code.

	Doug Sherk (:drs) (inactive)
	Comment 23 • 10 years ago

Addendum: I think we should consider bolding/changing the color of the text (perhaps to red) if we're going to shrink it. Please ask for ui-review and ux-review once you have a patch ready.

	Germán Toro del Valle (:gtorodelvalle) Assignee
	Comment 24 • 10 years ago

Hi guys! I am need-infoing Vicky here since she was the one who asked for the wallpaper setting for emergency calls.

Although I was self-confident with the proposed solution :p I am OK with Doug's suggestion.

Anyhow, waiting on you guys to come to an agreement regarding the wallpaper setting issue. Thanks!

	Germán Toro del Valle (:gtorodelvalle) Assignee
	Comment 25 • 10 years ago

On the other hand, I started implementing Doug suggestion and it is not as straight-forward as it may seem :) The point is that whenever the call changes its state a call to https://github.com/mozilla-b2g/gaia/blob/v1.4/apps/communications/dialer/js/handled_call.js#L231 is made forcing the calculation of the font size of the text shown. Since true is passed as the second parameter, the maximum font size is set and the "Emergency num..." is shown.
So I could include another filter to only update the font size if the call is not an emergency one. In fact, I did and prepared a new PR. I will not set the previous one as obsolete until we decide the way to go ;)

	Victoria Gerchinhoren [:vicky]
	Comment 27 • 10 years ago

(In reply to Doug Sherk (:drs) from comment #23)
> Addendum: I think we should consider bolding/changing the color of the text
> (perhaps to red) if we're going to shrink it. Please ask for ui-review and
> ux-review once you have a patch ready.

This should not happen.

	Germán Toro del Valle (:gtorodelvalle) Assignee
	Comment 28 • 10 years ago

In fact, I included the wallpaper as the background image as requested by Vicky in comment 14 ;)

	Germán Toro del Valle (:gtorodelvalle) Assignee
	Comment 29 • 10 years ago

In fact, I included the wallpaper as the background image as requested by Vicky in comment 14 ;)

	Germán Toro del Valle (:gtorodelvalle) Assignee
	Comment 30 • 10 years ago

In fact, I included the wallpaper as the background image as requested by Vicky in comment 14 ;)

	Victoria Gerchinhoren [:vicky]
	Comment 31 • 10 years ago

Comment on attachment 8449302 [details]
emergency_call_ended.png

Call ended should have the hang up icon next to the text, not the outgoing one

	Germán Toro del Valle (:gtorodelvalle) Assignee
	Comment 32 • 10 years ago

Sorry, I was not specific enough about the issues this patch was fixing. Please, consider only that the "Emergency number" text is shown with no ellipsis since this is a blocker for 1.4. Would you reconsider the previous ui-review taking this into consideration? :)

If you consider the icon issue should also be a blocker, please ping me via Skype and we can discuss it with Mari Ángeles and open a new bug if appropriate ;-) Thanks!

	Germán Toro del Valle (:gtorodelvalle) Assignee
	Comment 33 • 10 years ago

Need-infoing Vicky regarding my comment 32.

	Victoria Gerchinhoren [:vicky]
	Comment 34 • 10 years ago

NOt a blocker but an issue.

	Germán Toro del Valle (:gtorodelvalle) Assignee
	Comment 35 • 10 years ago

Yeah, the point it to focus in this bug on what it has been reported in its title and description :-p If you consider it appropriate, no problem to create a new bug about the icon issue for 1.4 :) Would you be so kind, Loli, to create this bug for us so you can also track it? Thank you very much! ;)

	Doug Sherk (:drs) (inactive)
	Comment 36 • 10 years ago

(In reply to Doug Sherk (:drs) from comment #22)
> Comment on attachment 8447957 [details]
> 21180.html
> Instead of half-cherry-picking some work that happened in other bugs and
> didn't get uplifted, I'd rather special case the emergency call header text.
> We can write a CSS selector that includes the .emergency-active state and
> forces the contact/number text (in this case, "Emergency number") down to
> the minimum allowed font size by FSM. This way, we're not depending on risky
> run-time resizing code.

Anthony, do you agree with this?

	Anthony Ricaud (:rik)
	Comment 37 • 10 years ago

I like how simple it is. That means introducing an !important and that's bad but because this is for a branch, I'm fine with it.

	Doug Sherk (:drs) (inactive)
	Comment 38 • 10 years ago

Comment on attachment 8449297 [details]
21265.html

Thanks for making the proposed changes. This is definitely a lot cleaner. I'm surprised that we don't need an !important as Anthony said, but I tested it and it works fine for me. I left some comments on the PR.

(In reply to Victoria Gerchinhoren [:vicky] from comment #34)
> NOt a blocker but an issue.

Germán, would you file a followup for this and then reflag Vicky for ui-review on attachment 8449302 [details]?

	Germán Toro del Valle (:gtorodelvalle) Assignee
	Comment 39 • 10 years ago

Ups, in fact need-infoing Loli regarding comment 36 ;) Thanks!

	Loli (:lolimartinezcr)
	Comment 40 • 10 years ago

(In reply to Germán Toro del Valle from comment #39)
> Ups, in fact need-infoing Loli regarding comment 36 ;) Thanks!

Thanks German!

New bug created https://bugzilla.mozilla.org/show_bug.cgi?id=1034032

	Germán Toro del Valle (:gtorodelvalle) Assignee
	Comment 41 • 10 years ago

Comment on attachment 8449297 [details]
21265.html

Comments by Doug included ;) Thanks!

	Doug Sherk (:drs) (inactive)
	Comment 42 • 10 years ago

Comment on attachment 8449297 [details]
21265.html

I just realized a really bad problem that the test that you added uncovered. I thought that the call node was cleared every time a call is made/ended, but it turns out that it persists. There's no teardown logic for the .emergency-call class, so we'll have to add that.

I left a couple of extra comments in the PR. On the next revision, please make sure that the following works correctly:

1) A regular call is made and doesn't have the .emergency-call class.
2) An emergency call is made and does have the .emergency-call class.
3) An emergency call is made and does have the .emergency-call class. Then it's ended and a regular call is made, and it doesn't have the .emergency-call class.

	Germán Toro del Valle (:gtorodelvalle) Assignee
	Comment 43 • 10 years ago

Comment on attachment 8449297 [details]
21265.html

Hi Doug, I included your comments and moved the included tests to make it clearer to the reader ;)

Regarding the reusing of the call node, they are not reused since each new HandleCall clones its own call node (see https://github.com/mozilla-b2g/gaia/blob/master/apps/callscreen/js/handled_call.js#L30 ). The problem with the test case failing was that I reused (copy & pasted) some code and was using an emergency number in that test case so obviously the emergency-call class was set, as it had to be. My fault, sorry!

Although the tests are being check, I am going to ask for a new review right now ;)

	Germán Toro del Valle (:gtorodelvalle) Assignee
	Comment 44 • 10 years ago

Great! Travis and TBPL are fine with the patch :)

	Anthony Ricaud (:rik)
	Comment 45 • 10 years ago

Comment on attachment 8449297 [details]
21265.html

This patch contains the emergency wallpaper changes that have not been agreed to land on 1.4. Please ask for review again when the patch is only fixing the truncation problem.

	Germán Toro del Valle (:gtorodelvalle) Assignee
	Comment 46 • 10 years ago

Vicky, according to comment 21 and comment 45 by Anthony and as we just discussed in IRC, I am removing the background change from this patch. If you want the wallpaper background instead of the red ringing bell in v1.4, please file a bug and nominate it for v1.4 ;) Thanks!

	Doug Sherk (:drs) (inactive)
	Comment 47 • 10 years ago

(In reply to Anthony Ricaud (:rik) from comment #45)
> Comment on attachment 8449297 [details]
> 21265.html
> 
> This patch contains the emergency wallpaper changes that have not been
> agreed to land on 1.4. Please ask for review again when the patch is only
> fixing the truncation problem.

Thanks, Anthony. I could swear I read you write that somewhere else, but when I looked for it I couldn't find it.

	Anthony Ricaud (:rik)
	Comment 48 • 10 years ago

(In reply to Doug Sherk (:drs) from comment #47)
> Thanks, Anthony. I could swear I read you write that somewhere else, but
> when I looked for it I couldn't find it.
Comment 21 ;)

	Germán Toro del Valle (:gtorodelvalle) Assignee
	Comment 49 • 10 years ago

Showing the wallpaper fix removed from the patch ;)

	Germán Toro del Valle (:gtorodelvalle) Assignee
	Comment 50 • 10 years ago

Merged in v1.4: https://github.com/mozilla-b2g/gaia/commit/5c9e1e4131d3ac8915ed88b72bb66dc7d97be6a0

	Loli (:lolimartinezcr)
	Comment 51 • 10 years ago

Tested and working
Hamachi
1.4
Gecko-a9ee8ae
Gaia-5c9e1e4

	Victoria Gerchinhoren [:vicky]
	Comment 52 • 10 years ago

Removing NI as there's nothing to answer here.

	Loli (:lolimartinezcr)
	Comment 53 • 10 years ago

(In reply to Victoria Gerchinhoren [:vicky] from comment #52)
> Removing NI as there's nothing to answer here.

Vicky i have verified this bug, for this reason i change status to  VERIFIED. Please if you think it should be other status, please tell me.

Tested and working
Hamachi
1.4
Gecko-a9ee8ae
Gaia-5c9e1e4