1017365 - [V1.4][Dialer]The "Emergency number" is displayed as "Emergency nu..."

Status: VERIFIED FIXED

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

Description

[panda67231]

Attachment: attachment.rar

[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

Comment 1

[Jason Smith [:jsmith]]

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?

Comment 2

[Beatriz Rodríguez [:brg]]

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.

Comment 3

[Jason Smith [:jsmith]]

Can you get a screenshot of this bug in Spanish?

Comment 4

[panda67231]

Attachment: Spanish.png

Attached Spanish screenshot

Comment 5

[Jason Smith [:jsmith]]

(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?

Comment 6

[Beatriz Rodríguez [:brg]]

(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!

Comment 7

[Anthony Ricaud (:rik)]

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.

Comment 8

[Jason Smith [:jsmith]]

(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.

Comment 9

[Axel Hecht [:Pike]]

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.

Comment 10

[Francisco Jordano [:arcturus] [:francisco]]

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

Comment 11

[Victoria Gerchinhoren [:vicky]]

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

Comment 12

[Victoria Gerchinhoren [:vicky]]

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

Comment 13

[Maria Angeles Oteo (:oteo)]

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

Comment 14

[Victoria Gerchinhoren [:vicky]]

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.

Comment 15

[Germán Toro del Valle (:gtorodelvalle)]

Attachment: emergency_call_english.png

Comment 16

[Germán Toro del Valle (:gtorodelvalle)]

Attachment: emergengy_call_french.png

Comment 17

[Germán Toro del Valle (:gtorodelvalle)]

Attachment: 21180.html

Comment 18

[Germán Toro del Valle (:gtorodelvalle)]

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 :-)

Comment 19

[Germán Toro del Valle (:gtorodelvalle)]

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!

Comment 20

[Germán Toro del Valle (:gtorodelvalle)]

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

Comment 21

[Anthony Ricaud (:rik)]

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.

Comment 22

[Doug Sherk (:drs) (inactive)]

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.

Comment 23

[Doug Sherk (:drs) (inactive)]

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.

Comment 24

[Germán Toro del Valle (:gtorodelvalle)]

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!

Comment 25

[Germán Toro del Valle (:gtorodelvalle)]

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 ;)

Comment 26

[Germán Toro del Valle (:gtorodelvalle)]

Attachment: 21265.html

Comment 27

[Victoria Gerchinhoren [:vicky]]

(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.

Comment 28

[Germán Toro del Valle (:gtorodelvalle)]

Attachment: emergency_call_outgoing.png

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

Comment 29

[Germán Toro del Valle (:gtorodelvalle)]

Attachment: emergency_call_ongoing.png

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

Comment 30

[Germán Toro del Valle (:gtorodelvalle)]

Attachment: emergency_call_ended.png

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

Comment 31

[Victoria Gerchinhoren [:vicky]]

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

Comment 32

[Germán Toro del Valle (:gtorodelvalle)]

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!

Comment 33

[Germán Toro del Valle (:gtorodelvalle)]

Need-infoing Vicky regarding my comment 32.

Comment 34

[Victoria Gerchinhoren [:vicky]]

NOt a blocker but an issue.

Comment 35

[Germán Toro del Valle (:gtorodelvalle)]

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! ;)

Comment 36

[Doug Sherk (:drs) (inactive)]

(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?

Comment 37

[Anthony Ricaud (:rik)]

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.

Comment 38

[Doug Sherk (:drs) (inactive)]

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]?

Comment 39

[Germán Toro del Valle (:gtorodelvalle)]

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

Comment 40

[Loli (:lolimartinezcr)]

(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

Comment 41

[Germán Toro del Valle (:gtorodelvalle)]

Comment on attachment 8449297 [details]
21265.html

Comments by Doug included ;) Thanks!

Comment 42

[Doug Sherk (:drs) (inactive)]

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.

Comment 43

[Germán Toro del Valle (:gtorodelvalle)]

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 ;)

Comment 44

[Germán Toro del Valle (:gtorodelvalle)]

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

Comment 45

[Anthony Ricaud (:rik)]

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.

Comment 46

[Germán Toro del Valle (:gtorodelvalle)]

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!

Comment 47

[Doug Sherk (:drs) (inactive)]

(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.

Comment 48

[Anthony Ricaud (:rik)]

(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 ;)

Comment 49

[Germán Toro del Valle (:gtorodelvalle)]

Attachment: 21398.html

Showing the wallpaper fix removed from the patch ;)

Comment 50

[Germán Toro del Valle (:gtorodelvalle)]

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

Comment 51

[Loli (:lolimartinezcr)]

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

Comment 52

[Victoria Gerchinhoren [:vicky]]

Removing NI as there's nothing to answer here.

Comment 53

[Loli (:lolimartinezcr)]

(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