1134961 - [Dialer] [RTL] Additional contact info in suggestion bar in dialer is not bdi

Status: VERIFIED FIXED

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

Description

[Tamara Hills [:thills]]

STR:
1.  Create a contact with a phone number with a '+ in it. (e.g. [phone])
2.  Put the phone in arabic (I put that as second step only because it's easier to add a contact in english)
2.  Start dialing typing in that number in the dialer (e.g '408')

Expected:
The suggestion bar matches to the number '[phone]'
Actual:
The suggestion bar matches to the number '14085551212+'

I believe this needs a bdi wrapper around it.

Comment 1

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

Nominating for 2.2 since RTL is a feature for this release.

Comment 2

[Johan Lorenzo [:jlorenzo]]

Triage: RTL is a must-have for 2.2.

Comment 3

[Tamara Hills [:thills]]

Attachment: ltrsuggestioncomma.png

Comment 4

[Tamara Hills [:thills]]

Attachment: rtlsuggestioncomma.png

Hi Ahmed,

Can you take a look at this attachment?  The purpose of this bug was to make the suggestion bar phone number bdi so that in rtl it shows up as [phone] instead of 14085551212+.  Wrapping in bdi fixed this, but I'm noticing an extra space at the end of the number now.  I know that in english, we do a comma and then a space, but in this screenshot there is a space and then a comma.  I'm not sure if this is correct or not. 

Can you help?

Thanks,

-tamara

Comment 5

[Tamara Hills [:thills]]

Hi Stephany,

Do you have any info on how we are treating commas in Arabic?  Can you take a look at my comment 4 to Ahmed and see if we have any general guidance on this?

Thanks,

-tamara

Comment 6

[Stephany Wilkes]

Tamara, how does the comma look when the language on the phone is set to Arabic? That screenshot will be more helpful than the English. Thanks!

Comment 7

[Tamara Hills [:thills]]

Hi Stephany,

The rtlsuggestioncomma.png attachment has the arabic.

-tamara

Comment 8

[Stephany Wilkes]

My guess is this is something with the interpretation of the character - i.e. English punctuation is sticking around when Arabic punctuation should be used. Commas in Arabic are different: the English comma is (,) while the Arabic comma (الفاصلة) points the opposite way (،) and it is written on top of the line.

Setting an ni? for Delphine to see if punctuation is handled as part of string changes; I'm just not sure.

Comment 9

[Delphine Lebédel [:delphine - Use needinfo]]

I confirm this is a string that needs to be changed. In fact, all commas in Arabic need to be fixed, this is not the only instance it seems.
Ahmed asked me to ni him here, as he's the main localizer. thanks!

Comment 10

[Tamara Hills [:thills]]

Delphine/Stephany, 

I'm just going to pull the comma out and make that a localizeable string in the dialer since we have not heard back from Ahmed.  I need to get this one moving as it's a blocker

Stephany, if you disagree, please let me know, but we need to make this actionable soon.

Thanks,

-tamara

Comment 11

[Stephany Wilkes]

That's fine. Thanks Tamara!

Comment 12

[Lancy(Leave from Mozilla)]

Test case has been added in moztrap:
https://moztrap.mozilla.org/manage/case/16006/

Comment 13

[Tamara Hills [:thills]]

Attachment: WIP for fixing the + in the number

This WIP just handles fixing the + in the number.  Basically puts bdi around it.  What's missing from this is dealing with the comma in between the number and the type of number (e.g. mobile).

Comment 14

[Tamara Hills [:thills]]

Hi Oleg,

Wondering if you have time to look at this bdi issue.  I've posted a WIP for this.  The remaining work is the comma.  Let us know if you're able to look at it.

Thanks,

-tamara

Comment 15

[Oleg Zasypkin [:azasypkin]]

(In reply to Tamara Hills [:thills] from comment #14)
> Hi Oleg,
> 
> Wondering if you have time to look at this bdi issue.  I've posted a WIP for
> this.  The remaining work is the comma.  Let us know if you're able to look
> at it.
> 
> Thanks,
> 
> -tamara

Hey Tamara, 

I'm on PTO currently (till March 27th). I can take a look at the issue as soon as I'm back if it's still not fixed.

Comment 16

[GitHub Autolander Bot]

Attachment: [gaia] tamarahills:bugfix/1134961-Contact-info-suggestionbar-rtl > mozilla-b2g:master

Comment 17

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8583799 [details] [review]
[gaia] tamarahills:bugfix/1134961-Contact-info-suggestionbar-rtl > mozilla-b2g:master

I started to review this, but while testing it, I found that it was broken. I left some comments on the PR, anyways.

Comment 18

[Tamara Hills [:thills]]

Attachment: Suggestion Overlay

Comment 19

[Tamara Hills [:thills]]

Attachment: ICEContacts.png

Comment 20

[Tamara Hills [:thills]]

Hi Steve,

Wanted to see if you can have a look at this RTL patch as I've put in a lot of time
and still can't seem to get it right!  Some background...

1.  This contact_overlay code (which is the key to all of this and is part of a 
button) is used in 3 places:  
a) In the emergency call app (for ICE contacts)
b) In the dialer in the suggestion bar
c) In the dialer in the list of suggestions when there are multiple.

In summary, I want to make sure that the ellipsis are on the right side for english characters.  
As a result, I've used the pattern <bdi>class="someclass"</bdi> where the textOverflow is
set to ellipsis and overflow hidden, whitespace: nowrap.  Julien had pointed me to this
workaround to get the ellipsis in the right direction.  I had to set the display to
inline-block on this and the width to 100%.  What you will see in the patch is that if you
have a name and number that are very long, things will look fairly decent.  If it's a 
short name and short number things will be indented and off a bit in the layout.  See 
screenshot in [1] as an example.  This is from the ICE Contacts overlay.  There seems to
be something interfering when the ellipsis are done inside the <bdi> tag.  So, I'm
having hard time finding a solution that works for ellipsed names/numbers as well as
names/numbers which are not ellipsed.

I also have a line height issue with Arabic next to English character set, but that
is sort of secondary at this point.

Just curious if you've seen anything like this where the ellipsis inside the bdi tag
seems to throw off the rest of the formatting and wondering if you have any hints.

[1] https://bug1134961.bugzilla.mozilla.org/attachment.cgi?id=8584159

Thanks,

-tamara

Comment 21

[Steve Chung [:steveck]]

Hi Tamara,

Sorry for the late reply because I'm confused about the contact overlay styling. Just looking to the css and the indention seems quite normal to me because the name and number field both set the additional padding-left/right in contact overlay, but for the text take the full width it will ignore the orignal padding in button that makes the layout looks "correct".

TBH I have no idea about the root cause, since button element is a black box and having div or other block element inside is not encouraged, Maybe I'm worng about the contact overlay layout because the boxing adjustment seems some black magic to me to make layout looks fine. Maybe you can try to replace the button with div element first to see if the layout is still broken.Sorry that I didn't provide any useful information :/

Comment 22

[GitHub Autolander Bot]

Attachment: [gaia] DouglasSherk:1134961-dialer-suggestion-bar-rtl > mozilla-b2g:master

Comment 25

[Doug Sherk (:drs) (inactive)]

Attachment: Properly ellipsize and display the "+" symbol in and on suggestion bar.

This fixes this bug, bug 1146240, and bug 1146241.

I'm using `dir="auto"` as described in bug 1146687 comment 10.

Steve, I'm asking you for review since you know RTL better than us.

Comment 26

[Tamara Hills [:thills]]

Comment on attachment 8584655 [details] [diff] [review]
Properly ellipsize and display the "+" symbol in and on suggestion bar.

Hi Doug,

It's a really nice fix.  Very simple.  I tested all the cases in english and arabic.

Thanks,
-tamara

Comment 27

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

Comment on attachment 8584655 [details] [diff] [review]
Properly ellipsize and display the "+" symbol in and on suggestion bar.

Tried on the phone, working for me.

Comment 28

[Steve Chung [:steveck]]

Attachment: ar-contact-overlay-ltr

Comment 29

[Steve Chung [:steveck]]

Comment on attachment 8584655 [details] [diff] [review]
Properly ellipsize and display the "+" symbol in and on suggestion bar.

Review of attachment 8584655 [details] [diff] [review]:
-----------------------------------------------------------------

Please see attachment 8585904 [details] that this patch might introduce some regression. 2 main problems:
1) tel number field alignment.
2) name field text align issue if text direction is opposite to system.

Just a reminder that the direction setting will ignore the parent's text-align: start/end. You could override with text-align: left/right to original button css to make sure the content aligned correctly(and remove the ellipsis direction)

::: shared/elements/contacts/contact_in_overlay.html
@@ +1,5 @@
>  <element name="contact-in-overlay" extends="button">
>    <template>
>  
> +      <div class="js-name ci__name ellipsis" dir="auto"></div>
> +      <div class="ci__details">

Once you move the ellipsis to the number span, please note the details field is shifted when name field exceed the original width. You will need to set the width: 100% and margin: 0 auto back to force this field aligns to original position.

@@ +6,2 @@
>          <span class="js-tel-type ci__tel-type"></span>
> +        <span class="js-tel ellipsis" dir="auto"></span>

Not sure why not use dir="ltr" in dom or direction: ltr in css. Is there any chance that the number field need to display as rtl?

::: shared/style/contacts/contact_in_overlay.css
@@ +13,5 @@
>  }
>  
>  .contact-item .ci__tel-type:not(:empty):after {
>    content: ',';
> +  margin-right: 5px;

Maybe -moz-margin-end would be more appropriate?

Comment 30

[Doug Sherk (:drs) (inactive)]

Attachment: Properly ellipsize and display the "+" symbol in and on suggestion bar.

PR updated.

Thanks, Steve. This should address everything you described.

(In reply to Steve Chung [:steveck] from comment #29)
> Comment on attachment 8584655 [details] [diff] [review]
> Properly ellipsize and display the "+" symbol in and on suggestion bar.
> 
> Review of attachment 8584655 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: shared/style/contacts/contact_in_overlay.css
> @@ +13,5 @@
> >  }
> >  
> >  .contact-item .ci__tel-type:not(:empty):after {
> >    content: ',';
> > +  margin-right: 5px;
> 
> Maybe -moz-margin-end would be more appropriate?

I don't think so, yet. Once we localize commas, that will make sense, but otherwise use of `-moz-margin-end` right now will make the string look like "foo ,bar", which never looks correct. Maybe I'm wrong, though.

Comment 31

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8587065 [details] [diff] [review]
Properly ellipsize and display the "+" symbol in and on suggestion bar.

Carrying r+ from francisco and thills.

Comment 32

[Steve Chung [:steveck]]

Comment on attachment 8587065 [details] [diff] [review]
Properly ellipsize and display the "+" symbol in and on suggestion bar.

Review of attachment 8587065 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry If I didn't exlain clearly in the previous feedback and let me reiterate why we need text-aglin and dir here:
 - text-aglin will align the text to the position you want, but it will not affect the ellipsis direction
 - dir will affect both text align and ellipsis direction, but text-aglin: left/right(not start/end, sad) will have higher prority to override the aglin position.

So the robust way to algin the text properly with correct ellipsis direction is to:
 1) Set the button text-align to left by default and to right when system is rtl, The original actiona menu button is set to start but it will be affected by dir
 2) Set specific element with proper direction, I think forcing number to ltr make sense, but contact should be auto anyway.

::: shared/elements/contacts/contact_in_overlay.html
@@ +1,4 @@
>  <element name="contact-in-overlay" extends="button">
>    <template>
>  
> +      <div class="js-name ci__name ellipsis" dir="ltr"></div>

This name field shouldn't set ltr because it will force the ellipsis always displayed at right no matter the contact is rtl or lrt. Set it as auto like previous version is more reasonable

::: shared/style/contacts/contact_in_overlay.css
@@ +13,5 @@
>  }
>  
>  .contact-item .ci__tel-type:not(:empty):after {
>    content: ',';
> +  margin-right: 5px;

Actually it should be judged by l10n team because we are not sure about where to set the space(See https://github.com/mozilla-b2g/gaia/blob/master/apps/sms/locales/sms.en-US.properties#L114 in message app) But let's not picky on this issue now and leave it as it is since it's too late for localization.

@@ +58,5 @@
>    margin: 0 auto;
>    width: 100%;
>  }
> +
> +html[dir="rtl"] .ellipsis {

I think it's more clear to set text-align left/right to menu button directly.

Comment 33

[Doug Sherk (:drs) (inactive)]

Attachment: Properly ellipsize and display the "+" symbol in and on suggestion bar.

PR updated.

Steve, I think I appropriately dealt with all of your comments this time.

Oleg, could you review this, since Steve is on PTO?

Comment 34

[Oleg Zasypkin [:azasypkin]]

Comment on attachment 8587443 [details] [diff] [review]
Properly ellipsize and display the "+" symbol in and on suggestion bar.

Review of attachment 8587443 [details] [diff] [review]:
-----------------------------------------------------------------

Everything looks good to me, except for the few things, see below. 

I'll be on PTO tomorrow, but still can finish review, just ni? whenever you're ready.

::: shared/elements/contacts/contact_in_overlay.html
@@ +6,1 @@
>          <span class="js-tel-type ci__tel-type"></span>

Not sure how severe it's for Dialer, but I suspect tel type can be custom tag of custom length. Do we want to ellipsis it? What part of full width should it take (should be easy to configure since it's flex item)? + custom tag is predefined RTL/LTR string and no matter what locale is currently set.

::: shared/style/contacts/contact_in_overlay.css
@@ +29,5 @@
>  }
>  
>  .ci__details {
> +  width: 100%;
> +  margin: 0 auto;

Do we really need this margin rule?

@@ +60,5 @@
>  }
> +
> +/* Override BB styles for RTL */
> +html[dir="rtl"] [role="dialog"][data-type="action"] > menu > button.contact-item,
> +html[dir="rtl"] .contact-item {

I don't see Steve's recommendation in the patch - "1) Set the button text-align to left by default ....., The original action menu button is set to start but it will be affected by dir". Otherwise short RTL name is right aligned in LTR locale that doesn't seem correct.

Comment 35

[Doug Sherk (:drs) (inactive)]

Attachment: Properly ellipsize and display the "+" symbol in and on suggestion bar.

PR updated.

Let's try again. This handles every case that I can think of. It's not perfect in every place, but I think it's acceptable.

Comment 36

[Doug Sherk (:drs) (inactive)]

Attachment: Properly ellipsize and display the "+" symbol in and on suggestion bar.

Updated PR.

Better patch. Use `rem`'s instead of `%`'s for width.

Comment 37

[Steve Chung [:steveck]]

(In reply to Oleg Zasypkin [:azasypkin] from comment #34)

> Not sure how severe it's for Dialer, but I suspect tel type can be custom
> tag of custom length. Do we want to ellipsis it? What part of full width
> should it take (should be easy to configure since it's flex item)? + custom
> tag is predefined RTL/LTR string and no matter what locale is currently set.
> 
Just a reminder that flexbox inside the button tag might not work correctly, that's why I didn't suggest using more flex feature to accomplish it. Anyway you can still give it a try.
 
> ::: shared/style/contacts/contact_in_overlay.css
> @@ +29,5 @@
> >  }
> >  
> >  .ci__details {
> > +  width: 100%;
> > +  margin: 0 auto;
> 
> Do we really need this margin rule?
> 
Sadly we do... :/ otherwise width will be expanded by overflowed contact name

Comment 38

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8587758 [details] [diff] [review]
Properly ellipsize and display the "+" symbol in and on suggestion bar.

Steve, if you want to finish the review, would be welcome as well. I didn't notice any problems without the `margin: 0 auto` line, even when both the tel and tel type elements are overflowing.

Comment 39

[Oleg Zasypkin [:azasypkin]]

Comment on attachment 8587758 [details] [diff] [review]
Properly ellipsize and display the "+" symbol in and on suggestion bar.

Review of attachment 8587758 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry, but with the latest patch I see the following issues:

1. Phone type separator (",") is displayed from the wrong side for RTL phone tag in LTR locale;
2. There is no phone type separator (",") visible with long LTR phone type in LTR locale.

Comment 40

[Doug Sherk (:drs) (inactive)]

Attachment: Properly ellipsize and display the "+" symbol in and on suggestion bar.

Updated PR.

(In reply to Oleg Zasypkin [:azasypkin] from comment #39)
> Comment on attachment 8587758 [details] [diff] [review]
> Properly ellipsize and display the "+" symbol in and on suggestion bar.
> 
> Review of attachment 8587758 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Sorry, but with the latest patch I see the following issues:
> 
> 1. Phone type separator (",") is displayed from the wrong side for RTL phone
> tag in LTR locale;
> 2. There is no phone type separator (",") visible with long LTR phone type
> in LTR locale.

This patch addresses both issues.

Hopefully this'll be the last review. We have to get this landed.

Comment 41

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8587965 [details] [diff] [review]
Properly ellipsize and display the "+" symbol in and on suggestion bar.

Steve and Oleg are both on PTO, so we shouldn't hold our breath for their review. We don't actually need their r+ to land this though, as they were just helping us with best practices and spotting mistakes that I missed.

Johan, could you verify this? If it looks good to you, we'll land it with r+ from Francisco and Tamara. If Steve and/or Oleg find anymore problems, we can deal with them in followups.

Comment 42

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8587965 [details] [diff] [review]
Properly ellipsize and display the "+" symbol in and on suggestion bar.

Julien has "volunteered" to help us here.

Comment 43

[Julien Wajsberg [:julienw]]

Comment on attachment 8587965 [details] [diff] [review]
Properly ellipsize and display the "+" symbol in and on suggestion bar.

lgtm !

Unrelated issues reported on IRC and were filed separately.

Comment 44

[Johan Lorenzo [:jlorenzo]]

Comment on attachment 8587965 [details] [diff] [review]
Properly ellipsize and display the "+" symbol in and on suggestion bar.

Looks good to me.

Testing done:
* Languages: Arabic, English
* Widgets: Suggestion bar, suggestion list, ICE contacts in emergency dialer
* Contacts used:
** Long LTR name which shows ellipsis
** Same thing for LTR
** A contact who has a pause at the end of his phone number
** A contact who has a pause in the middle of his phone number
** A contact with a long LTR label for its phone number
** A contact with a long RTL label for its phone number
** A contact with a long RTL carrier
** A contact with a long RTL carrier

I tested every combination.

I encountered 2 issues in Contacts (label goes of the screen - known issue, an RTL elipsis that is at the wrong position for the phone number label). No issues in Dialer.

They look minor enough to me. NI myself to file them.

Comment 45

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8584556 [details] [review]
[gaia] DouglasSherk:1134961-dialer-suggestion-bar-rtl > mozilla-b2g:master

Thanks for the reviews/testing, everyone.

Comment 46

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8587965 [details] [diff] [review]
Properly ellipsize and display the "+" symbol in and on suggestion bar.

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): New functionality / unhandled in the past.
[User impact] if declined: The Dialer's suggestion bar will look wrong in several ways, including wrong position of commas, ellipsizing, and + symbols.
[Testing completed]: QA approval from Johan on master, Julien and I also tested the latest version.
[Risk to taking this patch] (and alternatives if risky): Low.
[String changes made]: None.

Comment 47

[GitHub Autolander Bot]

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

Comment 48

[Ryan VanderMeulen [:RyanVM]]

v2.2: https://github.com/mozilla-b2g/gaia/commit/8c0adb46b4e3eb8fd41fbcc7e1f1079d98fb8215

Comment 49

[Johan Lorenzo [:jlorenzo]]

(In reply to Johan Lorenzo [:jlorenzo] (QA) from comment #44)
> I encountered 2 issues in Contacts (label goes of the screen - known issue,
> an RTL elipsis that is at the wrong position for the phone number label). No
> issues in Dialer.
> 
> They look minor enough to me. NI myself to file them.

Filed bug 1151878.

Comment 50

[Sue]

Attachment: verify_pass.png

This issue has been verified passed on latest build of Flame 2.2/3.0 with the same steps in comment 0. The suggestion bar matches to the number '[phone]'
See attachment:verify_pass.png
Rate: 0/5

Device: Flame 2.2 (pass)
Build ID               20150422162503
Gaia Revision          41a85c5f9db291d4f7c0e94c8416b5115b4ee407
Gaia Date              2015-04-21 17:23:41
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/367b3e608cd8
Gecko Version          37.0
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150422.200151
Firmware Date          Wed Apr 22 20:02:02 EDT 2015
Bootloader             L1TC000118D0

Device: Flame 3.0 (pass)
Build ID               20150422160203
Gaia Revision          9d4f756aa35cb7f030a92f3c1f65fb55254ddd1d
Gaia Date              2015-04-22 17:32:36
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/a9311ec2dd39
Gecko Version          40.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150422.193515
Firmware Date          Wed Apr 22 19:35:27 EDT 2015
Bootloader             L1TC000118D0

Comment 51

[Sue]

This issue also has been veriried passed on Nexus5 2.2/3.0 with the same steps in comment 0.
Rate:0/5

Device: Nexus 5 2.2 (pass)
Build ID               20150422162503
Gaia Revision          41a85c5f9db291d4f7c0e94c8416b5115b4ee407
Gaia Date              2015-04-21 17:23:41
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/367b3e608cd8
Gecko Version          37.0
Device Name            hammerhead
Firmware(Release)      5.1
Firmware(Incremental)  eng.cltbld.20150422.195718
Firmware Date          Wed Apr 22 19:57:35 EDT 2015
Bootloader             HHZ12f

Device: Nexus 5 3.0 (pass)
Build ID               20150422010202
Gaia Revision          15134b080b5f406e5aa36f5136c17dafb4e31f64
Gaia Date              2015-04-21 19:52:45
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/946ac85af8f4
Gecko Version          40.0a1
Device Name            hammerhead
Firmware(Release)      5.1
Firmware(Incremental)  eng.cltbld.20150422.044004
Firmware Date          Wed Apr 22 04:40:22 EDT 2015
Bootloader             HHZ12f