Closed Bug 1130305 Opened 5 years ago Closed 5 years ago

[Messages][RTL] suggestions list is not showing the ellipsis in the correct location for LTR names

Categories

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

ARM
Gonk (Firefox OS)
defect

Tracking

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

VERIFIED FIXED
2.2 S8 (20mar)
feature-b2g 2.2+
tracking-b2g backlog
Tracking Status
b2g-v2.2 --- verified
b2g-master --- verified

People

(Reporter: julienw, Assigned: steveck)

References

Details

Attachments

(5 files)

See the second suggestion in the screenshot.

We should use dir=auto or <bdi> somewhere. See js/contact_renderer.js where we define the template we use, and see the template in index.html.
feature-b2g: --- → 2.2+
Priority: -- → P2
I don't think we handled this case properly all around the message app, in fact the suggestion list already applied bdi element. I am not sure if the ellipsis direction should be considered for bdi element. Hi Oleg, could you think of any other solution that might have the same effect as bdi element?
  
In these cases we still want the LTR content align right under RTL system language for consistency, but in the meantime we want the ellipsis displayed coorectly based on text content. These 2 requirements seems contradictory, but I'll try to find if there exists any conbination could satisfy these requirments.
Flags: needinfo?(azasypkin)
BTW this issue should be related to bug 883884, maybe Oleg could confirm this.
It's an experimental patch that tried other possible method to solve ellipsis direction issue. Seems like plaintext with forced text-align direction might work, but there is still a problem that ellipsis wont work when plaintext applied(simplified test case : http://jsfiddle.net/xubj4y1j/3/)
I think we can handle the 2 issues differently, because in the first screenshot, we have only 1 contact per line. So we can do something smarter.

In the second screenshot, I don't know if we can really do better..

Also let's not forget that in RTL documents we'll likely have more RTL contact names.
Julien, Steve, I've done exactly that in bug #1126587
We have every item in the contact list displayed to the right in RTL and to the left in RTL. Then, every bdi element have the ellipsis in the correct location (depending on their content).

The trick is to let the bdi element do the ellipsis, while relying on parent element's alignment.

And what I really do like in this patch (thanks to Kaze I need to say) is that you don't even have any right/left. So we don't need any specific RTL hack, everything is mirrorred automatically :-)
Steve, I tried patch from attachment 8562638 [details] [diff] [review], but with it I don't see ellipsis on long LTR names in RTL and on long RTL names in LTR. 

At the first look Augustin's suggestion should work as well, though I'm not sure if we can eliminate "rtl-specific" styles to correctly align LTR names in RTL and vice versa. Maybe we can give it a try and compare?
Flags: needinfo?(azasypkin) → needinfo?(schung)
Attached file Link to github
Hey Augustin, thanks for the advice and it works well :)

Oleg, I think we should apply this solution first since my previous testing patch has some flaw. But I'm not sure whether we should apply this to all the contact name which is wrapped by bdi element, any thought?
Flags: needinfo?(schung)
Attachment #8569000 - Flags: feedback?(azasypkin)
Attachment #8568999 - Flags: feedback?(azasypkin)
Attachment #8569000 - Flags: feedback?(azasypkin)
Assignee: nobody → schung
Comment on attachment 8568999 [details] [review]
[gaia] steveck-chung:message-RTL > mozilla-b2g:master

(In reply to Steve Chung [:steveck] from comment #8)
> Created attachment 8569000 [details] [review]
> Link to github
> 
> Hey Augustin, thanks for the advice and it works well :)
> 
> Oleg, I think we should apply this solution first since my previous testing
> patch has some flaw. But I'm not sure whether we should apply this to all
> the contact name which is wrapped by bdi element, any thought?

If you mean other cases where we display contact names, then IMO - yes, we should take care of this as well. But I think in this bug we can cover only cases related to contact_renderer, including contact prompt header that isn't covered by current patch yet (left comment on GitHub).

For the rest of cases, eg. ellipsis for contact name in thread list item and thread header (at least for 1-to-1 threads) we can file separate bug.

Thanks!
Attachment #8568999 - Flags: feedback?(azasypkin) → feedback+
Status: NEW → ASSIGNED
Dear Steven
Could you kindly provide ETA date (Target Milestone) for this issue? Thank you very much!
Flags: needinfo?(schung)
(In reply to Josh Cheng [:josh] from comment #10)
> Dear Steven
> Could you kindly provide ETA date (Target Milestone) for this issue? Thank
> you very much!

Since patch already created, we could set the milestone to this sprint.
Flags: needinfo?(schung)
Target Milestone: --- → 2.2 S7 (6mar)
Comment on attachment 8568999 [details] [review]
[gaia] steveck-chung:message-RTL > mozilla-b2g:master

Hi Oleg, the patch is updated based on your suggestions(not sure if this changes is what you want, but I think created another css file mainly for ellipsis hach is not that necessary), thanks!
Attachment #8568999 - Flags: review?(azasypkin)
Comment on attachment 8568999 [details] [review]
[gaia] steveck-chung:message-RTL > mozilla-b2g:master

Hey Steve, everything looks good in code and on the device! But I've just noticed that some Arabic names are cut in contact prompt (fine in master) and some other places (the same as in master though). I've left comment about it and attached some screenshots at GitHub.

Thanks!
Attachment #8568999 - Flags: review?(azasypkin)
Comment on attachment 8568999 [details] [review]
[gaia] steveck-chung:message-RTL > mozilla-b2g:master

Thanks for finding the truncation issue for Arabic letter, I also apply the fix for contact name in report view, but I didn't fix for thread list view becaue we also have bug 1138350 for ellipsis and maybe we can address this in bug 1138350 together, what do you think?
I suspect that truncate issue is a generic problem that might related to Arabic font metrics, because we didn't face this issue with other languaues beofre. Anyway I still adjust line height for bdi element to avoid the tuncation first, but the Arabic letter truncation issue might worth some investigation later.
Attachment #8568999 - Flags: review?(azasypkin)
Comment on attachment 8568999 [details] [review]
[gaia] steveck-chung:message-RTL > mozilla-b2g:master

Patch updaated per Julien's comment(Thanks!) that applies display:block and -moz-fit-content width instead. I adjust the original line-height and margin/padding to prevent chars been cut, basically the line-height/font-size ration might be too small to prevent the char cut. However, I still need to override the line-height and margin in shared list styling. Not sure if it's necessary to fix the list styling because I didn't see other bug report about this, wdyt?
Comment on attachment 8568999 [details] [review]
[gaia] steveck-chung:message-RTL > mozilla-b2g:master

Looks good to me!

I noticed three more cut issues in carrier label (left comment at GitHub), but we have the same in master, so I'm OK if we fix it later and in a separate bug.

Thanks for you work!
Attachment #8568999 - Flags: review?(azasypkin) → review+
(In reply to Oleg Zasypkin [:azasypkin] from comment #16)
> Comment on attachment 8568999 [details] [review]
> [gaia] steveck-chung:message-RTL > mozilla-b2g:master
> 
> Looks good to me!
> 
> I noticed three more cut issues in carrier label (left comment at GitHub),
> but we have the same in master, so I'm OK if we fix it later and in a
> separate bug.
> 
> Thanks for you work!

Thank you too! I will create a follow up for it.
In master: https://github.com/mozilla-b2g/gaia/commit/63a11f957233aeeb578433ef40560f1fcd0cd293
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
I create Bug 1140977 for tracking all the cut issues in message app.
Comment on attachment 8568999 [details] [review]
[gaia] steveck-chung:message-RTL > mozilla-b2g:master

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): Side effects of RTL feature request
[User impact] if declined: Contact name ellipsis in suggestion list/message report will be displayed in opposite direction if contact/system direction is different.
[Testing completed]: N/A (CSS changes only)
[Risk to taking this patch] (and alternatives if risky): Low
[String changes made]: N/A
Attachment #8568999 - Flags: approval-gaia-v2.2?(bbajaj)
Attachment #8568999 - Flags: approval-gaia-v2.2?(bbajaj) → approval-gaia-v2.2+
Julien I'm seeing your original issue as being fixed.  However the issue from comment 1 still seems to be occurring.  You mentioned that it should be handled separately, but I couldn't find another bug that was written up for  it.  Should that still be written up separately or should we wait to mark this as verified til that gets fixed as well.

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
Flags: needinfo?(ktucker)
Flags: needinfo?(felash)
QA Whiteboard: [QAnalyst-Triage-]
Flags: needinfo?(ktucker)
Test case has been added in moztrap:
https://moztrap.mozilla.org/manage/case/15374/
Flags: in-moztrap+
(In reply to Jayme Mercado [:JMercado] from comment #21)
> Julien I'm seeing your original issue as being fixed.  However the issue
> from comment 1 still seems to be occurring.  You mentioned that it should be
> handled separately, but I couldn't find another bug that was written up for 
> it.  Should that still be written up separately or should we wait to mark
> this as verified til that gets fixed as well.

This is handled in bug 1138350.

Thanks a lot for your checks :)
Flags: needinfo?(felash)
Per comment 23 and my own comment 21, this issue is verified fixed.

Actual Results: Suggestion list names have the ellipsis in the correct position.


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
Status: RESOLVED → VERIFIED
QA Whiteboard: [QAnalyst-Triage-] → [QAnalyst-Triage?]
Flags: needinfo?(ktucker)
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(ktucker)
You need to log in before you can comment on or make changes to this bug.