1150953 - [RTL] Callscreen contact phone number "+" symbol is on the wrong side

Status: VERIFIED FIXED

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

Description

[Doug Sherk (:drs) (inactive)]

STR:
1. Set language to Arabic.
2. Create a contact with a "+" symbol in their phone number.
3. Call them.

Expected:
"+" symbol is on the left side of the phone number.

Actual:
"+" symbol is on the right side of the phone number.

Comment 1

[GitHub Autolander Bot]

Attachment: [gaia] DouglasSherk:1150953-callscreen-bdi-contact > mozilla-b2g:master

Comment 2

[Doug Sherk (:drs) (inactive)]

Attachment: Wrap contact phone numbers in <bdi> tags on the Callscreen.

Comment 3

[Tamara Hills [:thills]]

Comment on attachment 8588133 [details] [diff] [review]
Wrap contact phone numbers in <bdi> tags on the Callscreen.

Hi Doug,

It looks good.  I tested following scenarios in both english & arabic:

single call (inbound/outbound)
two calls (one with contact info, one without)
conf call - conference group details page check
Lockscreen incoming call
one established call + one incoming call with a contact info with +
ICE:  make a call from an ICE contact with a +

Thanks,

-tamara

Comment 4

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8588133 [details] [diff] [review]
Wrap contact phone numbers in <bdi> tags on the Callscreen.

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): None / newly needed functionality.
[User impact] if declined: "+" symbol will be on the wrong side for contacts on the Callscreen.
[Testing completed]: Tamara and I tested it.
[Risk to taking this patch] (and alternatives if risky): Low.
[String changes made]: None.

Comment 5

[GitHub Autolander Bot]

Pull request has landed in master: https://github.com/mozilla-b2g/gaia/commit/050e521cc201f92d025201ec2ba4188ffe39f15b

Comment 6

[Andrew Sutherland [:asuth] (he/him)]

Noticed this patch while doing a regression hunt.  Changes from textContent to innerHTML are usually a bad idea from a security and correctness perspective, and usually one would want to label all uses of innerHTML with an indication of the provenance of the data that ensures that it's safe.

I briefly investigated and it appears additionalContactInfo is likely from getPhoneNumberPrimaryInfo which may return the contact's name or organization.  It does not appear that any sanitization is occurring anywhere in the code I can see, and I'm very sure we don't define the mozContact name or organization to be pre-escaped.

ni'ing :freddyb as a security person since I'm not sure what the security team mapping is for specific apps and couldn't easily find it on https://wiki.mozilla.org/Security/B2G.

Comment 7

[Doug Sherk (:drs) (inactive)]

Ideally, we would separate the affected fields into a structure like this:

```html
<div>
  <span></span>
  <span></span>
</div>
```

Right now, we just write `innerHTML` directly into the `span.additionalContactInfo` element, which is equivalent to the `<div>` above.

Since this showed up the day before 2.2 FC, we didn't have time for the proper fix, so I went ahead with the `innerHTML` fix. I didn't think of the security implications of this, but everything you said makes sense.

I'd be fine with backing this out until we can fix it correctly. Will wait for :freddyb's answer, first.

Comment 8

[Andrew Sutherland [:asuth] (he/him)]

Wow!  Fast response.  I think I may be overstating the risk here.  I think I screwed up while tracing through the logic.  It might only be the case that the phone-number is passed through un-escaped.  (Well that and the localized string like "Mobile".)

Comment 9

[Doug Sherk (:drs) (inactive)]

No, actually, you're right. The phone type (e.g. "Mobile") can be entered by the user, and this isn't being sanitized.

Comment 10

[Andrew Sutherland [:asuth] (he/him)]

Ah.  Maybe getPhoneNumberAndType could be modified to return a DocumentFragment for the interim and have that replace the existing children of its mount-point?  That way you can still use textContent in your sub-nodes and not need to worry about passing around HTML strings.  (This is assuming you can guarantee that method is only called in a page-context and not on a worker.)

Comment 11

[Andrew Sutherland [:asuth] (he/him)]

In any event, I understand the FC rush.  I'm around all weekend if you want to point a review at me.  (As owner of the sub-module you can ask anyone you want to review! ;)

Comment 12

[Doug Sherk (:drs) (inactive)]

That's a good idea for a temporary solution. Let's back this out and do it that way instead. Leaving the needinfo for Freddy as I'd like to see his answer anyways.

Comment 13

[Doug Sherk (:drs) (inactive)]

https://github.com/mozilla-b2g/gaia/commit/e370e6beecd28785beef8c7ff299cf788693f0cc

Thanks, may do just that if I can't get a hold of any of my peers.

Comment 14

[GitHub Autolander Bot]

Attachment: [gaia] DouglasSherk:1150953-callscreen-bdi > mozilla-b2g:master

Comment 15

[Doug Sherk (:drs) (inactive)]

Attachment: Split phone number and tel type into separate spans in Callscreen.

It ended up not being much easier to use DocumentFragment, so I just went all the way and switched us to the correct <span> structure described in comment 7.

Flagging every Dialer peer for review as we need to get this landed today.

Comment 16

[Tamara Hills [:thills]]

Attachment: ellipsis on wrong side.

Comment 17

[Tamara Hills [:thills]]

Attachment: call ended overlap on conf group end call

Comment 18

[Tamara Hills [:thills]]

Hi Doug, 

I found two minor issues.  I'm not sure these are part of this patch or not.  

1.  For a call waiting second incoming call with contact info and a long contact name (ellipsized), the ellipsis seem to be on the wrong side in RTL.
2.  I think there is a padding issue (in both RTL and LTR) when you go into the conference group details and one of the names is ellipsized and you then end that call.  There is a bit of overlap of the "call ended" text.

I attached screenshots of both above.

Thanks,

-tamara

Comment 19

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8588453 [details] [diff] [review]
Split phone number and tel type into separate spans in Callscreen.

Investigating the issues in comment 18.

Comment 20

[Doug Sherk (:drs) (inactive)]

Attachment: Split phone number and tel type into separate spans in Callscreen.

PR updated, and fixed both issues in comment 18.

Comment 21

[Tamara Hills [:thills]]

Comment on attachment 8588609 [details] [diff] [review]
Split phone number and tel type into separate spans in Callscreen.

Hi Doug,

Looks good.  I retested all the cases in English/Arabic and also with/without ellipsis.  I also re-ran the tests.

Thanks,

-tamara

Comment 22

[Doug Sherk (:drs) (inactive)]

Thanks for the review, Tamara.

Comment 23

[GitHub Autolander Bot]

Pull request has landed in master: https://github.com/mozilla-b2g/gaia/commit/b8508bbf73756e61ac1c4281c5f2595cedb6b173

Comment 24

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8588609 [details] [diff] [review]
Split phone number and tel type into separate spans in Callscreen.

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): None / newly needed functionality.
[User impact] if declined: "+" symbol will be on the wrong side for contacts on the Callscreen, and there will be several padding/ellipsizing issues with contacts with long phone numbers and/or phone types.
[Testing completed]: Tamara and I tested it.
[Risk to taking this patch] (and alternatives if risky): Medium as this hits lots of code.
[String changes made]: None.

Comment 25

[bhavana bajaj [:bajaj]]

Given the medium risk here, I am hoping to get some more testing on master to see if there are any fallouts.
:drs, given the codepath this is touched, could you guide QA though some basic scenarios to cover for verification ?Thanks!

:ktucker, NI'ing you hoping you can assign some one to test tomorrow's builds once we have more info

Comment 26

[Doug Sherk (:drs) (inactive)]

Sure, I'm in total agreement on doing more testing.

This patch affects almost every place in the Callscreen where we display contact info or phone numbers. So, that includes:

1. The 1st incoming number display, e.g. your phone is off, and you receive a call from a contact.
2. The 2nd incoming number display, e.g. you're already talking with someone, and you receive another call from a contact.
3. The on call display, e.g. you're talking with a contact and the Callscreen shows their information.
4. The on call display with multiple calls, e.g. you're talking with a contact and have another contact on hold.
5. The conference call participants list.

The cases where we're dealing with contacts are more heavily impacted than the ones where we have no contact associated with a phone number, but we should test both, just with a heavier emphasis on contacts.

Comment 27

[Frederik Braun [:freddy]]

Thank you for bringing this to my attention, Andrew!
The patch looks good and textContent is always preferred over innerHTML of course.


If we'd *have* to have it this way
>  '<bdi>' + matchingTel.value + '</bdi>';
we might have used the tagged.js library in shared (https://github.com/mozilla-b2g/gaia/blob/master/shared/js/tagged.js) like this:

> Tagged.escapeHTML`<bdi>${matchingTel.value}</bdi>`

(The clear separation of dynamic input vs. static string components helps escaping where needed)

Comment 30

[Yeojin Chung [:YeojinC]]

Attachment: VoiceMailLockscreen.png

The + sign is located on the left side correctly on call screen. However, I found the following issues. Please let me know if any of these should be filed:

1) When the user gets a missed call from a non-saved contact with + in the number (ie. a foreign number) or get a new voicemail, the + sign is on the right side on the notification. (See attached screenshot).

2) When the user receives a second incoming call from a contact with + sign while already being in the call with the 1st call, the contact on the callscreen does NOT show the + and country code. THIS ISSUE ALSO REPRODUCES IN ENGLISH.

Environmental Variables:
Device: Flame 3.0 (KK, 319mb, full flash)
Build ID: 20150407010204
Gaia: c710bac533b76635161315bf907d004e000549cb
Gecko: ab0490972e1e
Gonk: b83fc73de7b64594cd74b33e498bf08332b5d87b
Version: 40.0a1 (3.0)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:40.0) Gecko/40.0 Firefox/40.0

Comment 31

[Yeojin Chung [:YeojinC]]

Attachment: 2ndIncomingArabic.png

Comment 32

[Julien Wajsberg [:julienw] (PTO -> Jul 24)]

Yes, please file separate bugs :) Thanks !

Comment 33

[Yeojin Chung [:YeojinC]]

Closing the bug as verified fixed, and filed new bug 1152127 and bug 1152131 for the issues mentioned in Comment 30.

Result: The + sign is located on the left side correctly on call screen.

Environmental Variables:
Device: Flame 3.0 (KK, 319mb, full flash)
Build ID: 20150407010204
Gaia: c710bac533b76635161315bf907d004e000549cb
Gecko: ab0490972e1e
Gonk: b83fc73de7b64594cd74b33e498bf08332b5d87b
Version: 40.0a1 (3.0)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:40.0) Gecko/40.0 Firefox/40.0

===================================================
Leaving verifyme for 2.2 uplift/verification.

Comment 34

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

triage: major issue on new feature=>RTL. We need to block it and get uplifted.

Comment 35

[Ryan VanderMeulen [:RyanVM]]

v2.2: https://github.com/mozilla-b2g/gaia/commit/9fca432ea20db39fc9820287a2d81eff5df0138d

Comment 36

[Sue]

Attachment: v2.2_callscreen_pass.png

This issue has been verified successfully on latest build of Flame 2.2 with the same steps in comment 0. The "+" symbol is located on the left side correctly on call screen.
See attachment:v2.2_callscreen_pass.png
Rate:0/5

Device: Flame 2.2 (pass)
Build ID               20150412002502
Gaia Revision          cec00d643f517ffd96cde559cd3bbd43ab85816c
Gaia Date              2015-04-10 21:41:12
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/5005522fd68e
Gecko Version          37.0
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150412.040730
Firmware Date          Sun Apr 12 04:07:41 EDT 2015
Bootloader             L1TC000118D0