1059087 - [Contacts] No space between 'Carrier Name' and 'Label'

Status: VERIFIED FIXED

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

Description

[Eric Chang [:ericcc] [:echang]]

Attachment: ContactsCarrierName.png

### STR
1. Have a contact with a phone number and carrier name.
2. View in detail view.

### Actual
No space between 'Carrier Name' and 'Label'

### Expected
With some space

### Version
Gaia      ea93363a8c424d65a9ad91438ce6961377a20f98
Gecko     https://hg.mozilla.org/mozilla-central/rev/f9bfe115fee5
BuildID   20140826160204
Version   34.0a1
ro.build.version.incremental=110
ro.build.date=Fri Jun 27 15:57:58 CST 2014
B1TC00011230

Comment 1

[Eric Chang [:ericcc] [:echang]]

[Blocking Requested - why for this release]: Regression

Comment 2

[Eric Chang [:ericcc] [:echang]]

Attachment: CallInformation.png

Also found in Call Information.

Comment 3

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

Probably a regression of the work we did for sharing the way to display contact information

Comment 4

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

triage: identifiable regression

Comment 5

[Anthony Ricaud (:rik)]

Moving this to Dialer because it's our responsibility. (and it also manifests in Call Info)

Comment 6

[Doug Sherk (:drs) (inactive)]

For reference, the purpose of components is not for tracking who is doing what, but for tracking the component the work is being done on. If someone encountered this bug, they would likely file it against contacts. Also, the regressing bug is against contacts. For these reasons, even though it manifests in the call info page, I'm moving it back to contacts.

I think this is being caused by the following very fragile code:
https://github.com/mozilla-b2g/gaia/blob/master/shared/js/contacts/contacts_buttons.js#L118

Here, we set the data-l10n-id to 'mobile', but then once it's translated, we append the 'separator' string, which in US English is ', ', to it. The obvious problem with this is that if the locale changes or l10n retranslates this for any reason, the separator/comma will be gone.

So this was always a problem (or at least since v2.0), but now it's more of a problem since we're directly calling translateFragment() on the contacts buttons containers where we didn't used to. I'm not sure what the best way is to fix this, so I'll talk with Stas and/or Zibi and then report back.

Comment 7

[Zibi Braniecki [:zbraniecki][:gandalf]]

So, I'd suggest splitting this into three <span/>'s within <h2/>:
 - type
 - separator
 - carrier

Separator and carrier would be hidden by default.

If type has l10nId, set it on type, otherwise remove data-l10n-id and set textContent.
If carrier exists, unhide separator and carrier and set carrier's textContent to carrier.

Flod - does it sound like a good solution? RTL should switch, and I don't think we need to allow localizers to mingle with the order of those two by themselves, right?

Comment 8

[Francesco Lodolo [:flod]]

Sounds like a good solution.

My doubt is if we actually need a separator. Can't we just have a distinct <span> for carrier, spaced from the existing element with padding/margin? What are the possible variants of this text?

P.S. still on vacation with spotty hotel's wi-fi, replies might take a while.

Comment 9

[Doug Sherk (:drs) (inactive)]

(In reply to Francesco Lodolo [:flod] (offline from Aug 31 to Sep 7) from comment #8)
> My doubt is if we actually need a separator. Can't we just have a distinct
> <span> for carrier, spaced from the existing element with padding/margin?
> What are the possible variants of this text?

The separator contains a comma.

Comment 10

[Doug Sherk (:drs) (inactive)]

Attachment: Fix missing spacing between "Carrier Name" and "Type Label".

PR: https://github.com/mozilla-b2g/gaia/pull/23742

This fixes the problem properly, as described in comment 7 and comment 8.

Comment 11

[Zibi Braniecki [:zbraniecki][:gandalf]]

I still don't understand this condition: https://github.com/mozilla-b2g/gaia/blob/6c844de438a58dbf7368e7a3ce0af1effcb21e7b/shared/js/contacts/contacts_buttons.js#L109-L114 - it seems to me like the code will only be fired if escapedType is falsy, in which case it will always get to this.DEFAULT_TEL_TYPE in mozL10n.get

What am I missing here?

Comment 12

[Francesco Lodolo [:flod]]

(In reply to Doug Sherk (:drs) from comment #9)
> The separator contains a comma.

That's exactly what I asked: do we need it or is it enough to have visual space between items, since they're already styled differently?

Comment 13

[Doug Sherk (:drs) (inactive)]

(In reply to Zibi Braniecki [:gandalf] from comment #11)
> I still don't understand this condition:
> https://github.com/mozilla-b2g/gaia/blob/
> 6c844de438a58dbf7368e7a3ce0af1effcb21e7b/shared/js/contacts/contacts_buttons.
> js#L109-L114 - it seems to me like the code will only be fired if
> escapedType is falsy, in which case it will always get to
> this.DEFAULT_TEL_TYPE in mozL10n.get
> 
> What am I missing here?

I think PHONE_MAP_TYPE is some kind of backwards compatibility shim. I'm not sure why |escapedType| is one of the possibilities in this branch, so I think I could remove that in this patch.

As for entering the branch to begin with, users can set custom types, so escapedType won't be falsy then.

(In reply to Francesco Lodolo [:flod] (offline from Aug 31 to Sep 7) from comment #12)
> (In reply to Doug Sherk (:drs) from comment #9)
> > The separator contains a comma.
> 
> That's exactly what I asked: do we need it or is it enough to have visual
> space between items, since they're already styled differently?

I'm inclined to leave it, but we can ask for VD input.

Comment 14

[Zibi Braniecki [:zbraniecki][:gandalf]]

(In reply to Doug Sherk (:drs) from comment #13)
> As for entering the branch to begin with, users can set custom types, so
> escapedType won't be falsy then.

That's exactly my concern. If the user will set custom Type then escapedType will be truthy, so the !escapedType will be falsy, so the branch will never be launched!

So it seems to me that the only scenario in which it will be launched is if escapedType is falsy, in which case it can be reduced to:

if (!escapedType) {
  escapedType = navigator.mozL10n.get(this.DEFAULT_TEL_TYPE);
}

and from there it's easy to just assign data-l10n-id instead :)

Comment 15

[Fang Shih [:fang]]

> > That's exactly what I asked: do we need it or is it enough to have visual
> > space between items, since they're already styled differently?
> 
> I'm inclined to leave it, but we can ask for VD input.
 
Yes, For the consistence with other carrier separator, we should stay with the comma. Thanks!

Comment 16

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

Comment on attachment 8484534 [details] [diff] [review]
Fix missing spacing between "Carrier Name" and "Type Label".

Thanks Doug!

Comment 17

[Doug Sherk (:drs) (inactive)]

Attachment: Remove unnecessary mozL10n.get() calls.

(In reply to Zibi Braniecki [:gandalf] from comment #14)
> That's exactly my concern. If the user will set custom Type then escapedType
> will be truthy, so the !escapedType will be falsy, so the branch will never
> be launched!
> 
> So it seems to me that the only scenario in which it will be launched is if
> escapedType is falsy, in which case it can be reduced to:
> 
> if (!escapedType) {
>   escapedType = navigator.mozL10n.get(this.DEFAULT_TEL_TYPE);
> }
> 
> and from there it's easy to just assign data-l10n-id instead :)

I wrote a followup patch for this. Should be a quick review. It's also on the PR now.

Comment 18

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

Comment on attachment 8487356 [details] [diff] [review]
Remove unnecessary mozL10n.get() calls.

Tried on the PR and working like charm.

Great work Doug!

Comment 19

[Doug Sherk (:drs) (inactive)]

https://github.com/mozilla-b2g/gaia/commit/14709c76da02d7353f52cce8dafee9d08a6afc18

Comment 20

[Ryan VanderMeulen [:RyanVM]]

This isn't fixed on v2.1 yet. Correcting the flags to reflect that. Please request approval when you get a chance, though.

Comment 21

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8484534 [details] [diff] [review]
Fix missing spacing between "Carrier Name" and "Type Label".

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): bug 1054995
[User impact] if declined: The phone type (e.g. "Mobile", "Work") and carrier name on the call info and contact pages will be mashed together, which is aesthetically unpleasing.
[Testing completed]: Francisco and I tried this.
[Risk to taking this patch] (and alternatives if risky): Low. Currently depends on bug 1030346, but a branch patch can be made which doesn't.
[String changes made]: None.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

Needs rebasing for v2.1 uplift.

Comment 23

[Doug Sherk (:drs) (inactive)]

https://github.com/mozilla-b2g/gaia/commit/713448b8963cd53c561f4b38640f8c63b655ce33

Comment 24

[Eric Chang [:ericcc] [:echang]]

Gaia-Rev        b542080231bb9cdbdb282ab926965ad3f91f7460
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-aurora/rev/d614ae398c03
Build-ID        20140923155123
Version         34.0a2
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  27
FW-Date         Thu Sep  4 14:59:02 CST 2014
Bootloader      L1TC10011800

Comment 25

[pete siphantong [:Petes]]

This bug is verified fixed on the Flame 2.1 (319mb) and the Flame 2.2 (319mb)


Flame 2.2 Master KK (319mb) (Full Flash)

Device: Flame 2.2 Master
BuildID: 20141011040204
Gaia: 95f580a1522ffd0f09302372b78200dab9b6f322
Gecko: 3f6a51950eb5
Gonk: 52c909e821d107d414f851e267dedcd7aae2cebf
Version: 35.0a1 (2.2 Master)
Firmware: V180
User Agent: Mozilla/5.0 (Mobile; rv:35.0) Gecko/35.0 Firefox/35.0

Flame 2.1 KK (319mb) (Full Flash)

Device: Flame 2.1
BuildID: 20141011000201
Gaia: f5d4ff60ffed8961f7d0380ada9d0facfdfd56b1
Gecko: d813d79d3eae
Gonk: 52c909e821d107d414f851e267dedcd7aae2cebf
Version: 34.0a2 (2.1)
Firmware: V180
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0


Result: There is space between 'Carrier Name' and 'Label'