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

VERIFIED FIXED in 2.2 S7 (6mar)

Status

defect
P2
normal
VERIFIED FIXED
4 years ago
4 years ago

People

(Reporter: clement.lefevre, Assigned: autra)

Tracking

unspecified
2.2 S7 (6mar)
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

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

Details

(Whiteboard: [2.2-bug-bash])

Attachments

(7 attachments)

(Reporter)

Description

4 years ago
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
(Reporter)

Updated

4 years ago
Blocks: contacts-rtl
(Reporter)

Comment 1

4 years ago
Seems to exist at least for Settings too, so probably to other places into the OS.
Depends on bug 1126606.
(Reporter)

Updated

4 years ago
Blocks: 1126606
(Reporter)

Updated

4 years ago
Duplicate of this bug: 1126606
I see the same behavior in English. Would you mind checking it too?
Flags: needinfo?(clement.lefevre)
Whiteboard: [2.2-bug-bash]
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
Status: UNCONFIRMED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → INVALID
(Reporter)

Comment 5

4 years ago
(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.
Flags: needinfo?(jlorenzo)
Flags: needinfo?(felash)
Flags: needinfo?(clement.lefevre)
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.
Flags: needinfo?(felash)
(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.
Flags: needinfo?(jlorenzo)
I meant regular English.
(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.
(Reporter)

Comment 10

4 years ago
(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.
Flags: needinfo?(lebedel.delphine)
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
Status: RESOLVED → REOPENED
Ever confirmed: true
Flags: needinfo?(lebedel.delphine)
Resolution: INVALID → ---
(Reporter)

Comment 14

4 years ago
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.
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.
triage: agree with comment #15 that we should follow this pattern in the other apps.
feature-b2g: --- → 2.2+
Priority: -- → P2
See Also: → 1132238
(Assignee)

Updated

4 years ago
Assignee: nobody → augustin.trancart
(Assignee)

Comment 18

4 years ago
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.
Flags: needinfo?(clement.lefevre)
Attachment #8566496 - Flags: review?(jmcf)
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!
Attachment #8566496 - Flags: review?(jmcf)
(Assignee)

Updated

4 years ago
Flags: needinfo?(dflanagan)
(Assignee)

Updated

4 years ago
Attachment #8566496 - Flags: review?(dflanagan)
(Assignee)

Updated

4 years ago
Flags: needinfo?(dflanagan)
Comment on attachment 8566496 [details] [review]
[gaia] autra:bug-1126587 > mozilla-b2g:master

This is more for Pavel :)
Attachment #8566496 - Flags: review?(dflanagan) → review?(pivanov)
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.
Attachment #8566496 - Flags: review?(pivanov) → review-
+1 for David comment
(Assignee)

Comment 23

4 years ago
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!
Duplicate of this bug: 1126900
Augustin did you provide a new patch? In that case you should flag again for review.
Flags: needinfo?(augustin.trancart)
(Assignee)

Comment 26

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

Oh thanks, I forgot to flip the r flag again!
Flags: needinfo?(augustin.trancart)
Attachment #8566496 - Flags: review- → review?(pivanov)
Comment on attachment 8566496 [details] [review]
[gaia] autra:bug-1126587 > mozilla-b2g:master

I think David is more familiar with this code :)
Attachment #8566496 - Flags: review?(pivanov) → review?(dflanagan)
(Assignee)

Updated

4 years ago
Attachment #8566496 - Flags: review?(dflanagan)
(Assignee)

Comment 28

4 years ago
Actually no, it is not ready yet because there is still the title of contact detail that is not right.
(Assignee)

Comment 29

4 years ago
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 :-)
Attachment #8566496 - Flags: review?(dflanagan)
(Assignee)

Updated

4 years ago
Attachment #8566496 - Flags: review?(dflanagan) → review?(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.
Attachment #8566496 - Flags: review?(jrburke) → review?(francisco)
Comment on attachment 8566496 [details] [review]
[gaia] autra:bug-1126587 > mozilla-b2g:master

Tried and working perfectly, thanks!
Attachment #8566496 - Flags: review?(francisco) → review+
Please ask for approval 2.2 once this lands on master
Keywords: checkin-needed
Status: REOPENED → RESOLVED
Last Resolved: 4 years ago4 years ago
Resolution: --- → FIXED
(Assignee)

Comment 34

4 years ago
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
Flags: needinfo?(clement.lefevre)
Attachment #8566496 - Flags: approval-gaia-v2.2?(bbajaj)
The landing on master caused bug 1139185 so I don't know if you want to uplift this to 2.2.
Depends on: 1139185
(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..
Attachment #8566496 - Flags: approval-gaia-v2.2?(bbajaj) → approval-gaia-v2.2+
Posted image 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.
Flags: needinfo?(clement.lefevre)
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
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(ktucker)
Status: RESOLVED → VERIFIED
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(ktucker)
(Reporter)

Comment 40

4 years ago
@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.
Flags: needinfo?(jmercado)
Flags: needinfo?(fan.luo)
Flags: needinfo?(clement.lefevre)
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.
Posted image 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
Flags: needinfo?(fan.luo) → needinfo?(clement.lefevre)
(Reporter)

Comment 43

4 years ago
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.
Flags: needinfo?(clement.lefevre)
Comment 41 and 42 make the ni? on me irrelevant.
Flags: needinfo?(jmercado)
You need to log in before you can comment on or make changes to this bug.