Closed Bug 1134961 Opened 9 years ago Closed 9 years ago

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

Categories

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

defect

Tracking

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

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

People

(Reporter: thills, Assigned: drs)

References

Details

(Whiteboard: [planned-sprint c=1])

Attachments

(8 files, 7 obsolete files)

35.38 KB, image/png
Details
35.36 KB, image/png
Details
24.68 KB, image/png
Details
20.49 KB, image/png
Details
46 bytes, text/x-github-pull-request
drs
: review+
Details | Review
19.90 KB, image/png
Details
3.05 KB, patch
julienw
: review+
jlorenzo
: qa-approval+
Details | Diff | Splinter Review
46.24 KB, image/png
Details
STR:
1.  Create a contact with a phone number with a '+ in it. (e.g. +14085551212)
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 '+14085551212'
Actual:
The suggestion bar matches to the number '14085551212+'

I believe this needs a bdi wrapper around it.
Blocks: dialer-rtl
Nominating for 2.2 since RTL is a feature for this release.
blocking-b2g: --- → 2.2?
Whiteboard: [planned-sprint c=?]
Target Milestone: --- → 2.2 S7 (6mar)
Assignee: nobody → thills
Whiteboard: [planned-sprint c=?] → [planned-sprint c=1]
Triage: RTL is a must-have for 2.2.
blocking-b2g: 2.2? → 2.2+
Status: NEW → ASSIGNED
Priority: -- → P1
Attached image ltrsuggestioncomma.png
Attached image 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 +14085551212 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
Flags: needinfo?(nefzaoui)
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
Flags: needinfo?(nefzaoui) → needinfo?(swilkes)
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!
Flags: needinfo?(swilkes) → needinfo?(thills)
Hi Stephany,

The rtlsuggestioncomma.png attachment has the arabic.

-tamara
Flags: needinfo?(swilkes)
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.
Flags: needinfo?(swilkes) → needinfo?(lebedel.delphine)
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!
Flags: needinfo?(lebedel.delphine) → needinfo?(nefzaoui)
Flags: needinfo?(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
Flags: needinfo?(swilkes)
That's fine. Thanks Tamara!
Flags: needinfo?(swilkes)
Target Milestone: 2.2 S7 (6mar) → 2.2 S8 (20mar)
Test case has been added in moztrap:
https://moztrap.mozilla.org/manage/case/16006/
Flags: in-moztrap+
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).
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
Flags: needinfo?(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.
Flags: needinfo?(azasypkin)
OS: Mac OS X → All
Hardware: x86 → All
Target Milestone: 2.2 S8 (20mar) → 2.2 S9 (3apr)
Attachment #8583799 - Flags: review?(drs)
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.
Attachment #8583799 - Flags: review?(drs)
Attached image Suggestion Overlay
Attached image ICEContacts.png
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
Flags: needinfo?(schung)
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 :/
Flags: needinfo?(schung)
See Also: → 1146240, 1146241
Assignee: thills → drs
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.
Attachment #8584655 - Flags: review?(thills)
Attachment #8584655 - Flags: review?(schung)
Attachment #8584655 - Flags: review?(francisco)
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
Attachment #8584655 - Flags: review?(thills) → review+
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.
Attachment #8584655 - Flags: review?(francisco) → review+
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?
Attachment #8584655 - Flags: review?(schung) → review-
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.
Attachment #8584655 - Attachment is obsolete: true
Attachment #8587065 - Flags: review?(schung)
Comment on attachment 8587065 [details] [diff] [review]
Properly ellipsize and display the "+" symbol in and on suggestion bar.

Carrying r+ from francisco and thills.
Attachment #8587065 - Flags: review+
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.
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?
Attachment #8575996 - Attachment is obsolete: true
Attachment #8583799 - Attachment is obsolete: true
Attachment #8587065 - Attachment is obsolete: true
Attachment #8587065 - Flags: review?(schung)
Attachment #8587443 - Flags: review?(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.
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.
Attachment #8587660 - Flags: review?(azasypkin)
Attachment #8587443 - Attachment is obsolete: true
Attachment #8587443 - Flags: review?(azasypkin)
Updated PR.

Better patch. Use `rem`'s instead of `%`'s for width.
Attachment #8587660 - Attachment is obsolete: true
Attachment #8587660 - Flags: review?(azasypkin)
Attachment #8587758 - Flags: review?(azasypkin)
(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 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.
Attachment #8587758 - Flags: review?(schung)
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.
Attachment #8587758 - Flags: review?(azasypkin)
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.
Attachment #8587758 - Attachment is obsolete: true
Attachment #8587758 - Flags: review?(schung)
Attachment #8587965 - Flags: review?(schung)
Attachment #8587965 - Flags: review?(azasypkin)
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.
Attachment #8587965 - Flags: qa-approval?(jlorenzo)
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.
Attachment #8587965 - Flags: review?(schung)
Attachment #8587965 - Flags: review?(felash)
Attachment #8587965 - Flags: review?(azasypkin)
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.
Attachment #8587965 - Flags: review?(felash) → review+
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.
Flags: needinfo?(jlorenzo)
Attachment #8587965 - Flags: qa-approval?(jlorenzo) → qa-approval+
Comment on attachment 8584556 [details] [review]
[gaia] DouglasSherk:1134961-dialer-suggestion-bar-rtl > mozilla-b2g:master

Thanks for the reviews/testing, everyone.
Attachment #8584556 - Flags: review+
Keywords: checkin-needed
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.
Attachment #8587965 - Flags: approval-gaia-v2.2?(bbajaj)
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Attachment #8587965 - Flags: approval-gaia-v2.2?(bbajaj) → approval-gaia-v2.2+
See Also: → 1151019
(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.
Flags: needinfo?(jlorenzo)
Attached image 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 '+14085551212'
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
Status: RESOLVED → VERIFIED
QA Whiteboard: [MGSEI-Triage+]
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
Flags: needinfo?(nefzaoui)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: