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

Status: VERIFIED FIXED

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

Description

[Steve Chung [:steveck]]

Attachment: 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?

Comment 1

[Wilson Page [:wilsonpage]]

This is an issue for SMS team as they are doing some RTL magic with the content inside `gaia-header h1`.

Comment 2

[Julien Wajsberg [:julienw]]

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?

Comment 3

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

triage: major issue from new feature

Comment 4

[Wilson Page [:wilsonpage]]

I'm confused can someone clarify how a device can get into this state and what the proposed fix is?

Comment 5

[Julien Wajsberg [:julienw]]

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 6

[GitHub Autolander Bot]

Attachment: [gaia] steveck-chung:message-header-RTL > mozilla-b2g:master

Comment 7

[Steve Chung [:steveck]]

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.

Comment 8

[Augustin Trancart [:autra]]

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?

Comment 9

[Steve Chung [:steveck]]

(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.

Comment 10

[Augustin Trancart [:autra]]

(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 11

[Julien Wajsberg [:julienw]]

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.

Comment 12

[GitHub Autolander Bot]

Attachment: [gaia] steveck-chung:message-RTL > mozilla-b2g:master

Comment 13

[Steve Chung [:steveck]]

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)

Comment 14

[Oleg Zasypkin [: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!

Comment 15

[Steve Chung [:steveck]]

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!

Comment 16

[Oleg Zasypkin [: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!

Comment 17

[Steve Chung [:steveck]]

The simplified solution is really brilliant, thanks!

In master: https://github.com/mozilla-b2g/gaia/commit/1e9ae9379b1d62cdbc4c3d7fda12bb54fb841c8d

Comment 18

[Steve Chung [:steveck]]

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

Comment 19

[Ryan VanderMeulen [:RyanVM]]

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

Comment 20

[Sue]

Attachment: 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

Comment 21

[Lancy(Leave from Mozilla)]

Test case has been added in moztrap:
https://moztrap.mozilla.org/manage/case/15397/