Closed Bug 1150953 Opened 5 years ago Closed 5 years ago

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

Categories

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

defect
Not set

Tracking

(blocking-b2g:2.2+, b2g-v2.2 verified, b2g-master verified)

VERIFIED FIXED
2.2 S10 (17apr)
blocking-b2g 2.2+
Tracking Status
b2g-v2.2 --- verified
b2g-master --- verified

People

(Reporter: drs, Assigned: drs)

References

Details

Attachments

(7 files, 3 obsolete files)

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.
Blocks: 1151019
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
Attachment #8588133 - Flags: review?(thills) → review+
Attachment #8588131 - Flags: review+
Keywords: checkin-needed
Keywords: checkin-needed
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.
Attachment #8588133 - Flags: approval-gaia-v2.2?(bbajaj)
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
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.
Flags: needinfo?(fbraun)
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.
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".)
No, actually, you're right. The phone type (e.g. "Mobile") can be entered by the user, and this isn't being sanitized.
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.)
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! ;)
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.
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
Attachment #8588131 - Attachment is obsolete: true
Attachment #8588133 - Attachment is obsolete: true
Attachment #8588133 - Flags: approval-gaia-v2.2?(bbajaj)
Attachment #8588453 - Flags: review?(thills)
Attachment #8588453 - Flags: review?(gsvelto)
Attachment #8588453 - Flags: review?(etienne)
Attachment #8588453 - Flags: review?(anthony)
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
Flags: needinfo?(drs)
Comment on attachment 8588453 [details] [diff] [review]
Split phone number and tel type into separate spans in Callscreen.

Investigating the issues in comment 18.
Attachment #8588453 - Flags: review?(gsvelto)
Attachment #8588453 - Flags: review?(etienne)
Attachment #8588453 - Flags: review?(anthony)
PR updated, and fixed both issues in comment 18.
Attachment #8588453 - Attachment is obsolete: true
Attachment #8588453 - Flags: review?(thills)
Flags: needinfo?(drs)
Attachment #8588609 - Flags: review?(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
Attachment #8588609 - Flags: review?(thills) → review+
Thanks for the review, Tamara.
Keywords: checkin-needed
Attachment #8588451 - Flags: review+
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Target Milestone: 2.2 S9 (3apr) → 2.2 S10 (17apr)
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.
Attachment #8588609 - Flags: approval-gaia-v2.2?(bbajaj)
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
Flags: needinfo?(drs)
Flags: needinfo?(pmathur)
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.
Flags: needinfo?(drs)
Flags: needinfo?(pmathur) → needinfo?(ktucker)
Flags: needinfo?(ktucker)
Keywords: qawanted
QA Contact: ychung
Flags: needinfo?(ychung)
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)
Flags: needinfo?(fbraun) → sec-review+
Duplicate of this bug: 1151770
Duplicate of this bug: 1151772
Attached image 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
Flags: needinfo?(ychung) → needinfo?(ktucker)
QA Whiteboard: [QAnalyst-Triage?]
Keywords: qawanted
Yes, please file separate bugs :) Thanks !
See Also: → 1152127
See Also: → 1152131
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.
Status: RESOLVED → VERIFIED
Keywords: verifyme
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(ktucker)
triage: major issue on new feature=>RTL. We need to block it and get uplifted.
blocking-b2g: 2.2? → 2.2+
Attachment #8588609 - Flags: approval-gaia-v2.2?(bbajaj) → approval-gaia-v2.2+
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
QA Whiteboard: [QAnalyst-Triage+] → [QAnalyst-Triage+][MGSEI-Triage+]
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.