1126587 - [RTL] Too long e-mail address truncate the beginning instead of the end

Status: VERIFIED FIXED

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

Description

[Clément Lefèvre]

With RTL (Arabic) format, if a contact does have a too long e-mail address, it will truncate the beginning of the address instead of the end.

Flame 3.0:

Build ID               20150126010231
Build Type             user
Gaia Revision          0f662dffef27599443cfcd790c2b39190a2b35c8
Gaia Date              2015-01-25 00:48:56
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/fa91879c8428
Gecko Version          38.0a1
Device ID              flame
Firmware(Release)      4.4.2
Firmware(Incremental)  39
Firmware Date          Thu Oct 16 18:19:14 CST 2014
Bootloader             L1TC00011880

Comment 1

[Clément Lefèvre]

Seems to exist at least for Settings too, so probably to other places into the OS.
Depends on bug 1126606.

Comment 3

[Johan Lorenzo [:jlorenzo]]

I see the same behavior in English. Would you mind checking it too?

Comment 4

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

in RTL language, truncation is expected to be at the "beginning" of a word since this that's where the word ends.
Not sure this can be changed for when using non-RTL characters within the RTL behaviour. 
Will close this one as INVALID but keep an eye out on Bugs such as Bug 1126883,since that addresses the LTR case

Comment 5

[Clément Lefèvre]

(In reply to Delphine Lebédel [:delphine - use need info] from comment #4)
> in RTL language, truncation is expected to be at the "beginning" of a word
> since this that's where the word ends.
> Not sure this can be changed for when using non-RTL characters within the
> RTL behaviour. 
> Will close this one as INVALID but keep an eye out on Bugs such as Bug
> 1126883,since that addresses the LTR case

No, it seems this can be determined. I discussed the problem with julienw two days ago. It seems they solved this problem in the Messages app by using an automatic property for these elements instead of manually setting it. That way, the behavior (truncated side) will change depending on the nature of the string.

Anyway, I NI him in case I would say a mistake.(In reply to Johan Lorenzo

[:jlorenzo] (QA) from comment #3)
> I see the same behavior in English. Would you mind checking it too?

You mean, RTL english? (If yes, I don't have it on my build), because currently:
- English have right side truncated, which is the end for occidental characters strings;
- Arabic have left side truncated, but considering it's here occidental characters strings (e-mails for example), it truncated the beginning of the string.

Comment 6

[Julien Wajsberg [:julienw]]

we used "dir=auto" in the SMS app; you can try inputting an email as recipient in the "new message" panel you'll see it's properly truncated.

Comment 7

[Johan Lorenzo [:jlorenzo]]

(In reply to Clément Lefèvre from comment #5)
> [:jlorenzo] (QA) from comment #3)
> > I see the same behavior in English. Would you mind checking it too?
> 
> You mean, RTL english?

I should have taken a screenshot. You're right I don't see anything suspicious anymore. I might have hit another bug while switching LTR/RTL languages on the fly.

Comment 8

[Johan Lorenzo [:jlorenzo]]

I meant regular English.

Comment 9

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

(In reply to Clément Lefèvre from comment #0)
> With RTL (Arabic) format, if a contact does have a too long e-mail address,
> it will truncate the beginning of the address instead of the end.

This is why I closed this bug as resolved invalid. In Arabic language, what you describe is what is expected. Truncation should happen on the left of the word instead of the right.
If this bug is not invalid and you meant in RTL BUT using non-RTL language, then let's reopen this.
Leaving as is at the time, feel free to take action on it.

Comment 10

[Clément Lefèvre]

(In reply to Delphine Lebédel [:delphine - use need info] from comment #9)
> (In reply to Clément Lefèvre from comment #0)
> > With RTL (Arabic) format, if a contact does have a too long e-mail address,
> > it will truncate the beginning of the address instead of the end.
> 
> This is why I closed this bug as resolved invalid. In Arabic language, what
> you describe is what is expected. Truncation should happen on the left of
> the word instead of the right.
> If this bug is not invalid and you meant in RTL BUT using non-RTL language,
> then let's reopen this.
> Leaving as is at the time, feel free to take action on it.

Yes I meant in RTL mode (like Arabic) but for LTL strings, as I was talking about e-mail addresses for example. This result on the beginning of the string being truncated instead of the end.

I'll join two screenshots, for edit mode and for the other mode and for contact list.

About reopening the bug, I NI you Delphine, as I does not have the rights on BMO to do so.

Comment 11

[Clément Lefèvre]

Attachment: RTL for LTL string when editing contact

Comment 12

[Clément Lefèvre]

Attachment: RTL for LTL string when watching a contact

Comment 13

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

Ok reopening, thanks for clarifying Clément!
I would assume this would probably have to follow the same rules and that Ahmed shows here: 
https://bug1126900.bugzilla.mozilla.org/attachment.cgi?id=8560153

Comment 14

[Clément Lefèvre]

Attachment: RTL for LTL string in the contact list

If someone could check in the phone call history too ; I do not have a SIM card right now in the phone to check this and maybe it does have the same effet on long name or long numbers.

In the opposite case, NI me and I'll test it with a SIM card later.

Comment 15

[Julien Wajsberg [:julienw]]

Attachment: how it works in SMS app

FWIW here is how it works in the SMS app in RTL mode (but it should be the same in LTR mode actually -- basically this should only depend on the content, not on the language).

I think it's how it should work everywhere as well.

Comment 16

[Dylan Oliver [:doliver]]

triage: agree with comment #15 that we should follow this pattern in the other apps.

Comment 17

[GitHub Autolander Bot]

Attachment: [gaia] autra:bug-1126587 > mozilla-b2g:master

Comment 18

[Augustin Trancart [:autra]]

Comment on attachment 8566496 [details] [review]
[gaia] autra:bug-1126587 > mozilla-b2g:master

Hi Jose,

Could you review this please? 

Clément, FYI this also affect other apps as well. So you might want to check new behavior when this patch lands.

Comment 19

[Jose Manuel Cantera]

Comment on attachment 8566496 [details] [review]
[gaia] autra:bug-1126587 > mozilla-b2g:master

Hi Augustin,

I would prefer this kind of changes to be reviewed by a BB owner. 

thanks!

Comment 20

[Julien Wajsberg [:julienw]]

Comment on attachment 8566496 [details] [review]
[gaia] autra:bug-1126587 > mozilla-b2g:master

This is more for Pavel :)

Comment 21

[David Flanagan [:djf]]

Comment on attachment 8566496 [details] [review]
[gaia] autra:bug-1126587 > mozilla-b2g:master

I agree with Julien that Pavel might be a better reviewer. But since I have already started my review, I'm going to complete it.

First, I am the module owner for the shared/ module, but in reality only for shared/js. We apparently don't have a module owner for shared/style, and I do not know who any of the people are who review patches to these files. Pavel seems like a good choice.

Having said that, however, I'd give this patch a r- for these reasons:

1) a generic change like this to shared code this late in the 2.2 cycle seems risky. I'd prefer to patch the individual apps that we know are affected and not risk causing regressions to other apps (There might be apps that use a bdi element in a list but do not want display:inline-block, for example.) And since we are moving toward using web components on master, there isn't much long term value in making improvements to the share stylesheets. I'd say that the risk of this shared change outweighs the benefit.

2) The contacts app seems to use "li > p > bdi" for these contact names and email addresses.  The list.css file already has styles for "li p". And you want to add "li bdi" that duplicates the overflow properties.  This seems too general. Can you use "li > p > bdi:only-child"? If that works, it seems specific enough that it would be much safer. It would still be easier to get reviews done if you made this change in app-specific stylesheets instead of shared stylesheets.

3) You could also try the approach Julien suggested and just set .dir="auto" on the relevant P elements in the contacts app.

4) For the display of contact names note that the bdi element contains indivdiually styled given name and family name. Will your fix work correctly in that case? The third screenshot you attached appears to show a single long string rather than a two part name, so be sure to test with actualy two-part names.

5) In comment 18, you mention that this patch affects other apps. If you really want to land a change to the shared file, please determine which other apps are affected by it and list them here. Don't just leave that up to QA.

Comment 22

[Pavel Ivanov [:ivanovpavel][:pivanov] UX]

+1 for David comment

Comment 23

[Augustin Trancart [:autra]]

Thanks for this very complete review! I will update my patch accordingly (not sure it will be in time for 2.2 but I'll try). Sorry for the mistake in reviewers, I didn't know you were only module owner for shared/js. Will try to remember that!

Note that for 3), dir="auto" does not work here because we still want the text to be right-aligned (in RTL languages) regardless of the actual content (RTL or LTR string). That's why we can't use dir="auto". We need to inherit dir rtl or ltr.

Otherwise, what you told me makes really sense to me, thanks!

Comment 25

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

Augustin did you provide a new patch? In that case you should flag again for review.

Comment 26

[Augustin Trancart [:autra]]

Comment on attachment 8566496 [details] [review]
[gaia] autra:bug-1126587 > mozilla-b2g:master

Oh thanks, I forgot to flip the r flag again!

Comment 27

[Pavel Ivanov [:ivanovpavel][:pivanov] UX]

Comment on attachment 8566496 [details] [review]
[gaia] autra:bug-1126587 > mozilla-b2g:master

I think David is more familiar with this code :)

Comment 28

[Augustin Trancart [:autra]]

Actually no, it is not ready yet because there is still the title of contact detail that is not right.

Comment 29

[Augustin Trancart [:autra]]

Comment on attachment 8566496 [details] [review]
[gaia] autra:bug-1126587 > mozilla-b2g:master

So this patch correct the ellipsis in the list and in the contact detail page (the email field).
However it does not correct the ellipsis in the title of the contact detail page. This is done by the gaia-header component. To fix this, my only guess right now would be to add a dir="auto" to the h1 and remove the direction="rtl" or "ltr" in the css, but that is too much for me to try without some advice :-)

Comment 30

[James Burke [:jrburke]]

Comment on attachment 8566496 [details] [review]
[gaia] autra:bug-1126587 > mozilla-b2g:master

This is a contacts app fix, and looking at the module owners page:
https://wiki.mozilla.org/Modules/FirefoxOS

It looks like Francisco might be module owner there, so I will route the review to him, since I do not have owner/peer status for that module.

Comment 31

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

Comment on attachment 8566496 [details] [review]
[gaia] autra:bug-1126587 > mozilla-b2g:master

Tried and working perfectly, thanks!

Comment 32

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

Please ask for approval 2.2 once this lands on master

Comment 33

[GitHub Autolander Bot]

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

Comment 34

[Augustin Trancart [:autra]]

Comment on attachment 8566496 [details] [review]
[gaia] autra:bug-1126587 > mozilla-b2g:master

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): rtl support
[User impact] if declined: user won't see the correct side of email adress, which can hinder readability of contact list
[Testing completed]: on flame v2.2
[Risk to taking this patch] (and alternatives if risky): low
[String changes made]: none

Comment 35

[KTucker [:KTucker][Inactive 3/4/2016]]

The landing on master caused bug 1139185 so I don't know if you want to uplift this to 2.2.

Comment 36

[bhavana bajaj [:bajaj]]

(In reply to KTucker [:KTucker] from comment #35)
> The landing on master caused bug 1139185 so I don't know if you want to
> uplift this to 2.2.

Looks like the follow-up is up for review. I'll wait till that's fixed on 3.0 as well so both these can be uplifted at once..

Comment 37

[Ryan VanderMeulen [:RyanVM]]

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

Comment 38

[Norry.L.F(Leave from Mozilla)]

Attachment: header title.png

Hi clement,
During verifying this bug, these cases mentioned on attachments 8560179  8560180 8560182, are fixed now, but I find another case, please see attachment, when you are watching one contact which is with long LTR string name, the name shown on header bar will be truncated at the beginning. And I am also doubt another case: whether long number should be truncated at begining or ending, please also see attachment. 
Could you confirm this? and may I reopen this bug until these two cases are fixed?
Thanks.

Comment 39

[Jayme Mercado [:JMercado] (Inactive 3/4/2016)]

This issue is verified fixed on the latest Nightly Flame 3.0 and 2.2 builds.

Actual Results: Long names and emails written in LTR languages are correctly truncated when the device is set to RTL.

Environmental Variables:
Device: Flame 3.0 KK (319MB) (Full Flash)
BuildID: 20150313010238
Gaia: eabe35cf054d47087b37c1ca7db8143717fbd7f3
Gecko: 42afc7ef5ccb
Gonk: ebad7da532429a6f5efadc00bf6ad8a41288a429
Version: 39.0a1 (3.0) 
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:39.0) Gecko/39.0 Firefox/39.0

Environmental Variables:
Device: Flame 2.2 KK (319MB) (Full Flash)
BuildID: 20150313002507
Gaia: 4aefc3f6f30a40ac67fdf841b7c90cd648b85369
Gecko: 049713f3b0ed
Gonk: ebad7da532429a6f5efadc00bf6ad8a41288a429
Version: 37.0 (2.2) 
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0

Comment 40

[Clément Lefèvre]

@Jayme Mercado: Is it possible that there is any difference between OTA updates and the one you tested?
Because on latest nightly by OTA, I'm able to reproduce what Norry.L.F is talking about and so to say that the bug isn't 100% fixed right now.

@Norry.L.F: As mentioned above, I can reproduce what you're pointing out. Maybe should we create contact with way more informations filled to check every possible information?

By the way, if someone could create a vCard with arabic fields inside to test, it could be fine too (no arabic keyboard to type some random things…)

NI on both of you as I thing the bug should be reopened.

Comment 41

[Julien Wajsberg [:julienw]]

Please file a separate bug for the header issue as this is clearly different issues.

See also bug 1138340 for a similar issue in the SMS app.


For the Phone number issue, I think it should have been fixed here as well, but let's file a separate bug as this will make things easier.

Comment 42

[Norry.L.F(Leave from Mozilla)]

Attachment: ArabicContact.png

With Julien's suggestions, we submit two bugs.

For the header issue, link to this new issue: bug 1143594
For the number issue, link to this new issue: bug 1143599, and it also descrips another two colums in edit screen: company & street. 

And we have tried to create a contact with arabic fields inside, and it looks fine except for number filed, please see attachment.

Build ID               20150315162500
Gaia Revision          a6b2d3f8478ec250beb49950fecbb8a16465ff6f
Gaia Date              2015-03-15 14:33:22
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/18619f8f6c5c
Gecko Version          37.0
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150315.195030
Firmware Date          Sun Mar 15 19:50:42 EDT 2015
Bootloader             L1TC000118D0

Comment 43

[Clément Lefèvre]

Cleaning needinfo, I CC'd me on the two other bugs, I guess that there are nothing to see here anymore as there are bugs opened for them.

Comment 44

[Jayme Mercado [:JMercado] (Inactive 3/4/2016)]

Comment 41 and 42 make the ni? on me irrelevant.