Closed Bug 1138340 Opened 5 years ago Closed 5 years ago

[Messages][RTL] String in gaia-header is not showing the ellipsis in the correct location for LTR names

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

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

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

People

(Reporter: steveck, Assigned: steveck)

References

Details

Attachments

(3 files, 1 obsolete file)

Attached image Screen Shot.png
Please ref the screenshot. the ellipsis should be in right hand side for LTR text, but ellipsis displayd at left when system lang is RTL. 

We proposed some hack in bug 1130305 before root cause fixed in bug 883884, but I'm not sure to apply this in gaia-header module directly. Hi Wilson, do you think it's fine to apply this solution right in header component, or we should leave this workaround only in message app?
Flags: needinfo?(wilsonpage)
This is an issue for SMS team as they are doing some RTL magic with the content inside `gaia-header h1`.
Flags: needinfo?(wilsonpage) → needinfo?(azasypkin)
Wilson, just so that you know that Steve is actually part of the SMS team :)

Steve, maybe you can try to do both solutions, if they're both easy? And then we can see which one is preferable?
Flags: needinfo?(azasypkin)
blocking-b2g: --- → 2.2?
triage: major issue from new feature
blocking-b2g: 2.2? → 2.2+
I'm confused can someone clarify how a device can get into this state and what the proposed fix is?
Wilson, you need a thread with a contact name that is big enough to overflow the header title. Then use Mirrored English and open that thread.

This comes from [1].

[1] https://github.com/gaia-components/gaia-header/blob/master/gaia-header.js#L815

I think the solution is to add the ellipsis on the inner <bdi> we have. Something like:

  gaia-header h1 bdi {
    text-overflow: ellipsis;
    display: block;
    overflow: hidden;
    white-space: nowrap;
  }

I'm not sure gaia-header can do anything generic here... until we're not obliged to force it to LTR.
Comment on attachment 8572395 [details] [review]
[gaia] steveck-chung:message-header-RTL > mozilla-b2g:master

Hi Julien, it's a header ellipsis fix patch based on other rtl fixing from bug 1130305. I tried to find some generic way to fix for gaia header, but I still kept this changes in message app because I think it's reasonable to force the direction in header as same as the system. Only message app utilized bdi in header, which means the text's direction in bdi is not related to system and app need to take care of this. That's why I moved this to message app only.
Attachment #8572395 - Flags: feedback?(felash)
I'm taking advantage of the NI on Julien to say that it could be a good idea to make gaia_header RTL friendly... 
I guess every apps need this fix anyway right?
(In reply to Augustin Trancart [:autra] from comment #8)
> I'm taking advantage of the NI on Julien to say that it could be a good idea
> to make gaia_header RTL friendly... 
> I guess every apps need this fix anyway right?

Well, not really. Because most of the header title is set with translated string, aligning the title's direction with system seems reasonable. It makes sense that title content does not warp with bdi element by default and apps should control it by themselves for my POV. I can send this reminder to dev-gaia that app might need to beware of this if your header title content might have different direction with system(like sms/contect as far as I know). But I'm fine if you insist gaia-header should applys this by default.
(In reply to Steve Chung [:steveck] from comment #9)
> Well, not really. Because most of the header title is set with translated
> string, aligning the title's direction with system seems reasonable. 

Actually, I think header should always be aligned with system language direction (or centered). I was only speaking about the ellipsis direction. I meant imho gaia-header should by default :
- be aligned to the language direction (or centered)
- but show ellipsis according to content.
Actually, I've seen quite a few bugs about right-aligned content showing ellipsis on the wrong side for LTR string in RTL language (and also RTL string in LTR language), so that's why I thought this should be done in shared... This seems to me a reasonable default, because you hardly never want ellipsis at the beginning of a word (ie right for RTL word, and left for LTR) in title.

That being said:
- It is true that having title in another direction than system language is not a common case... So it might be overkill if only 2 apps are affected.
- when you say bdi should not warp, you mean that it should not do any ellipsis by default? In this case, you can also forget what I said :-)
Comment on attachment 8572395 [details] [review]
[gaia] steveck-chung:message-header-RTL > mozilla-b2g:master

I left 2 questions but this looks good. I agree that this seems to be the right solution.
Attachment #8572395 - Flags: feedback?(felash) → feedback+
Assignee: nobody → schung
Attachment #8572395 - Attachment is obsolete: true
Comment on attachment 8575116 [details] [review]
[gaia] steveck-chung:message-RTL > mozilla-b2g:master

Hi Oleg, I create a patch based on Bug 1130305 that applys the display:block for thte header title. For the group header I add another span with flex display to show the group title and participant correctly(otherwise the participant number will be pushed out of the title container)
Attachment #8575116 - Flags: review?(azasypkin)
Comment on attachment 8575116 [details] [review]
[gaia] steveck-chung:message-RTL > mozilla-b2g:master

Hey Steve, 

Everything looks good in both RTL and LTR, except for the one issue with group header (commented on GitHub) + I've left one question as well.

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

Sorry that some cases was ignored while testing... I updated the patch per your comments and gave some thought for your question, thanks!
Attachment #8575116 - Flags: review?(azasypkin)
Comment on attachment 8575116 [details] [review]
[gaia] steveck-chung:message-RTL > mozilla-b2g:master

Now it looks perfect! 

I just left one suggestion at GitHub that can make CSS part a bit simpler. And don't forget to fix thread_ui unit tests - they seem broken because of new header markup.

Thanks!
Attachment #8575116 - Flags: review?(azasypkin) → review+
The simplified solution is really brilliant, thanks!

In master: https://github.com/mozilla-b2g/gaia/commit/1e9ae9379b1d62cdbc4c3d7fda12bb54fb841c8d
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Comment on attachment 8575116 [details] [review]
[gaia] steveck-chung:message-RTL > mozilla-b2g:master

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): 2.2 RTL feature request
[User impact] if declined: The ellipsis direction for gaia-header title might be wrong in certain conditions.
[Testing completed]: Yes
[Risk to taking this patch] (and alternatives if risky): Low
[String changes made]: N/A
Attachment #8575116 - Flags: approval-gaia-v2.2?(bbajaj)
Attachment #8575116 - Flags: approval-gaia-v2.2?(bbajaj) → approval-gaia-v2.2+
Attached image massage_long_name.png
This issue has been verified succeffully on Flame 2.2/3.0, the ellipsis is in right hand side for LTR text.
See attachment:massage_long_name.png
Rate:0/5

Flame 2.2 build: pass
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

Flame 3.0 build: pass
Build ID               20150315160203
Gaia Revision          d4177902b04b8fedcb7df9a30ae6e9677e03d2d4
Gaia Date              2015-03-13 15:58:35
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/af68c9c0e903
Gecko Version          39.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150315.192711
Firmware Date          Sun Mar 15 19:27:22 EDT 2015
Bootloader             L1TC000118D0
Status: RESOLVED → VERIFIED
QA Whiteboard: [MGSEI-Triage+]
Test case has been added in moztrap:
https://moztrap.mozilla.org/manage/case/15397/
Flags: in-moztrap+
See Also: → 1157610
You need to log in before you can comment on or make changes to this bug.