1132666 - [RTL][Music] Translated text on music tiles is not right aligned in Arabic

Status: VERIFIED FIXED

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

Description

[Brogan Zumwalt [Inactive]]

Attachment: Screenshot

Description:
When language is set to Arabic, the text on the tiles in the music app that appears in Arabic (e.g. "Unknown Artist", "Unknown Album", etc.) is not right aligned. 

Repro Steps:
1) Update a Flame to 20150212010213
2) Set language to Arabic
3) Have a song with an unknown/blank artist and/or album title
4) Launch Music app


Actual:
Translated text on tiles in music app are not right aligned when language is set to Arabic.

Expected:
All translated text in music app is right aligned when language is set to Arabic.

Environmental Variables:
Device: Flame 3.0 Master
Build ID: 20150212010213
Gaia: d5a71cedb37dd45f439f672489db3994b349ac43
Gecko: 3094601af679
Gonk: e7c90613521145db090dd24147afd5ceb5703190
Version: 38.0a1 (3.0)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:38.0) Gecko/38.0 Firefox/38.0

Repro frequency: 3/3, 100%
See attached: screenshot

Comment 1

[Brogan Zumwalt [Inactive]]

Issue DOES occur on Flame 2.2

Translated text on tiles in music app are not right aligned when language is set to Arabic.

Device: Flame 2.2
Build ID: 20150212002504
Gaia: 791e53728cd8018f1d7cf7efe06bbeb1179f0370
Gecko: 5dec207fcbeb
Gonk: e7c90613521145db090dd24147afd5ceb5703190
Version: 37.0a2 (2.2)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0

Comment 2

[GitHub Autolander Bot]

Attachment: [gaia] anefzaoui:bug-1132666 > mozilla-b2g:master

Comment 3

[Hema Koka [:hema]]

Ahmed,

Dominic is probably out because of new year. Please route review request to :squib (Jim Porter)

Thanks
Hema

Comment 4

[Ahmed Nefzaoui [:Nefzaoui]]

Comment on attachment 8565376 [details] [review]
[gaia] anefzaoui:bug-1132666 > mozilla-b2g:master

(In reply to Hema Koka [:hema] from comment #3)
> Ahmed,
> 
> Dominic is probably out because of new year. Please route review request to
> :squib (Jim Porter)
> 
> Thanks
> Hema

Thanks for letting me know!

Comment 5

[Jim Porter (:squib)]

Comment on attachment 8565376 [details] [review]
[gaia] anefzaoui:bug-1132666 > mozilla-b2g:master

This looks sane, I guess. I'm not really a good person to ask about RTL issues. Is it normal for Latin text to be left-aligned in the same UI as Arabic text is right-aligned, or should we pick an alignment based on the user's locale?

Comment 6

[Ahmed Nefzaoui [:Nefzaoui]]

Comment on attachment 8565376 [details] [review]
[gaia] anefzaoui:bug-1132666 > mozilla-b2g:master

Hey Stephany,
Sorry to add even more load to your work but could you please give quick insight on this or assign someone to do ?
Thanks :)

Comment 7

[Stephany Wilkes]

Jim, according to the pattern, Latin character text is right-aligned in instances where the rest of the Arabic UI is RTL.

Comment 8

[Stephany Wilkes]

Comment on attachment 8565376 [details] [review]
[gaia] anefzaoui:bug-1132666 > mozilla-b2g:master

Seeing if Tif can review this patch on our big, busy FL day.

Comment 9

[Tiffanie Shakespeare [:tif]]

Comment on attachment 8565376 [details] [review]
[gaia] anefzaoui:bug-1132666 > mozilla-b2g:master

Per Steph's comment 7  - Arabic is right aligned but Latin is still left-aligned.

Comment 10

[Tiffanie Shakespeare [:tif]]

Attachment: latin characters are left-aligned

See my previous comment

Comment 11

[Ahmed Nefzaoui [:Nefzaoui]]

I guess then that makes the purpose of the bug is invalid, and then so does the bug itself :)

Comment 12

[Tiffanie Shakespeare [:tif]]

no no, the bug is still very valid - if you look at the original screenshot and STR, everything was left aligned. The patch fixes the alignment of items translated to Arabic (e.g. "unknown album") but Latin characters (e.g. Crystal Method) are still left-aligned when the should also be right aligned (Steph's comment)

Comment 13

[Dylan Oliver [:doliver]]

(In reply to Tiffanie Shakespeare from comment #12)
> no no, the bug is still very valid ... but Latin characters (e.g.
> Crystal Method) are still left-aligned when they should also be right aligned
> (Steph's comment)

Right, you can see the inconsistency if you go into the now playing screen. There you will find that the Latin chars are right-aligned as expected. 

Jim, Ahmed is no longer working on bugs for 2.2. Are you available to take over on this one to fix the alignment?

Comment 14

[Jim Porter (:squib)]

I can look at this.

Tif: Just to be clear, *all* the artist/album info - no matter the language of each item - should be left-aligned when the UI's locale is LTR, and right-aligned when the UI's locale is RTL, correct?

Comment 15

[Tiffanie Shakespeare [:tif]]

Yes, we want to consistently align the text. So everything should be right-aligned, including Latin strings, when the locale is a RTL language.

In looking at the patch, the Arabic was being right-aligned, but the English was not.

Thanks Jim!

Comment 16

[Josh Cheng [:josh]]

Dear Jim,
Could you kindly provide ETA date (Target Milestone) for this issue? Thank you very much!

Comment 17

[Jim Porter (:squib)]

I'll try to get to this by the end of the week, but I'm looking at other 2.2 blockers at the moment.

Comment 18

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

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

Comment 19

[GitHub Autolander Bot]

Attachment: [gaia] jimporter:music-rtl-tiles > mozilla-b2g:master

Comment 20

[Jim Porter (:squib)]

Comment on attachment 8575060 [details] [review]
[gaia] jimporter:music-rtl-tiles > mozilla-b2g:master

Hub: You know a little about RTL, I think. Could you look at this?

Comment 21

[Hubert Figuiere [:hub]]

Comment on attachment 8575060 [details] [review]
[gaia] jimporter:music-rtl-tiles > mozilla-b2g:master

Looks good to me.

Comment 22

[Jim Porter (:squib)]

Comment on attachment 8575060 [details] [review]
[gaia] jimporter:music-rtl-tiles > mozilla-b2g:master

Tif: Just wanted to make sure this looks right for you. Unfortunately, I don't have any MP3s with Arabic metadata to test...

Comment 23

[Tiffanie Shakespeare [:tif]]

Hey Jim I'm having issues testing the patch. Arabic disappears from the language list when I install your patch :-/

I also found another bug while trying to get this working - when English is truncated it's truncated on the left (e.g. ...stal Method) I don't know if you want to file a new bug for this or tackle in this one since it's related to switching the alignment to the right.

Comment 24

[Jim Porter (:squib)]

(In reply to Tiffanie Shakespeare from comment #23)
> Hey Jim I'm having issues testing the patch. Arabic disappears from the
> language list when I install your patch :-/

Can you use Mirrored English instead? That should be available in debug builds.

> I also found another bug while trying to get this working - when English is
> truncated it's truncated on the left (e.g. ...stal Method) I don't know if
> you want to file a new bug for this or tackle in this one since it's related
> to switching the alignment to the right.

That's probably a different bug; I'd guess it's related to bug 1140154.

Comment 25

[Tiffanie Shakespeare [:tif]]

Mirrored English works and looks like the bug is solved.

I checked out the patch in bug 1140154 and it doesn't seem to fix the truncation issue. So IDK if you want that patch to update with a fix or update it here.

Thanks Jim!

Comment 26

[Jim Porter (:squib)]

Well, then I don't know. I'm not really good with RTL stuff, but this might be a bug in Gecko.

Comment 27

[Josh Cheng [:josh]]

Hi Lancy,
Can you verify whether the patch https://bugzilla.mozilla.org/attachment.cgi?id=8575060 fix the issue? Thanks!

Comment 28

[Tiffanie Shakespeare [:tif]]

Hey Jim - can you send my a zip file of the music app with the patch? I want to see if I can get Arabic working with the changes you made. 

Also, who can we check with to nail down where the weird truncation issue is occurring? This definitely needs to be fixed.

Thanks!

Comment 29

[Tiffanie Shakespeare [:tif]]

Jim - n/m on the second issue. Dylan helped me track done a bug about this issue occurring in other apps too.

https://bugzilla.mozilla.org/show_bug.cgi?id=1143599

Comment 30

[Jim Porter (:squib)]

I have no idea how to generate a zip file for a given app. It *should* work if you just check out my branch and run

  APP=music make install-gaia

from the terminal. That would only update the music app, and not the rest of the system.

Comment 31

[Dylan Oliver [:doliver]]

Jim, we resolved to just land this one based on hub's r+ and Tif will do her UI review afterward. Can you go ahead and land this on master?

Comment 32

[GitHub Autolander Bot]

http://docs.taskcluster.net/tools/task-graph-inspector/#TRhCD7tlSJy1HgM-9TxBQw

The pull request failed to pass integration tests. It could not be landed, please try again.

Comment 33

[GitHub Autolander Bot]

http://docs.taskcluster.net/tools/task-graph-inspector/#_1jUKHY0RaWtfdWXbIMmOQ

The pull request failed to pass integration tests. It could not be landed, please try again.

Comment 34

[Dylan Oliver [:doliver]]

Can you provide an update on this bug?

Comment 35

[Jim Porter (:squib)]

I'm trying to figure out why the tests are failing in a way that prevents landing. I'm not sure if it's my fault or just the inherent instability of our tests, though.

Comment 36

[Josh Cheng [:josh]]

Hi Jim,
DO you have any update for this issue?
Thanks!

Comment 37

[Lancy(Leave from Mozilla)]

(In reply to Josh Cheng [:josh] from comment #27)
> Hi Lancy,
> Can you verify whether the patch
> https://bugzilla.mozilla.org/attachment.cgi?id=8575060 fix the issue? Thanks!

Hi Josh,

After building the patch with flame 3.0 latest gaia, this issue dose not exist.
See the attachment:Verify1_Translated text.png
Reproducing rate:0/10

Flame 3.0 build:
Build ID               20150412160203
Gaia Revision          6414097adfb054260e47cf72a9d1b80eaecef408
Gaia Date              2015-04-13 06:24:39
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/0a46652bd992
Gecko Version          40.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150412.192808
Firmware Date          Sun Apr 12 19:28:21 EDT 2015
Bootloader             L1TC000118D0

Comment 38

[Lancy(Leave from Mozilla)]

Attachment: Verify1_Translated text.png

Comment 39

[Tiffanie Shakespeare [:tif]]

Lancy, can you verify that english is also right-aligned? Thanks!

Comment 40

[Lancy(Leave from Mozilla)]

Hi Tiffanie,
For Arabic language will only exist via building gaia with the patch, we tried this method, but something is wrong with en-US language package. 
Instead, if we build the image with the patch, the Arabic language does not exist. Can we verify this issue in English via building the image with the patch?

Comment 41

[Tiffanie Shakespeare [:tif]]

Sorry Lancy I'm not quite following and re-reading my question I apologize for not being more clear. :-/

At one point (see comment 15) artists in Arabic or the translated word "unknown" was getting right-aligned but artists in English (e.g. Daft Punk) were being left-aligned.

In the screenshot you posted in comment 38, I only see Arabic and some other language or characters - I'm not sure what it is. 

I wanted to double check that an artist in English was still being right-aligned. Thanks :)

Comment 42

[GitHub Autolander Bot]

http://docs.taskcluster.net/tools/task-graph-inspector/#jLcYtyoRRBe5vcqj4HbXVw

The pull request failed to pass integration tests. It could not be landed, please try again.

Comment 43

[GitHub Autolander Bot]

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

Comment 44

[Lancy(Leave from Mozilla)]

Attachment: Verify2_Flame3.0_Pass.png

According to the steps in comment 0, this issue is verified pass on latest Flame 3.0 build.
See attachment: Verify2_Flame3.0_Pass.png
Reproducing rate:0/10

Flame 3.0 build:
Build ID               20150419160202
Gaia Revision          c6b04efa0f31a584e6ee0a46dd2b64c1e3c29adc
Gaia Date              2015-04-17 21:10:53
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/a32e3b93c8d8
Gecko Version          40.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150419.193329
Firmware Date          Sun Apr 19 19:33:40 EDT 2015
Bootloader             L1TC000118D0

Comment 45

[Josh Cheng [:josh]]

Hi Jim,
Can you also raise approval and pull request for 2.2?
Thanks!

Comment 46

[Lancy(Leave from Mozilla)]

(In reply to Tiffanie Shakespeare from comment #41)
> Sorry Lancy I'm not quite following and re-reading my question I apologize
> for not being more clear. :-/
> 
> At one point (see comment 15) artists in Arabic or the translated word
> "unknown" was getting right-aligned but artists in English (e.g. Daft Punk)
> were being left-aligned.
> 
> In the screenshot you posted in comment 38, I only see Arabic and some other
> language or characters - I'm not sure what it is. 
> 
> I wanted to double check that an artist in English was still being
> right-aligned. Thanks :)

Hi Tiffanie,
 Sorry for the late reply, could you please review the attachment in comment 44? Translated text is left-aligned in English, and this issue has been fixed in latest Flame 3.0 build.

Comment 47

[Tiffanie Shakespeare [:tif]]

Comment on attachment 8575060 [details] [review]
[gaia] jimporter:music-rtl-tiles > mozilla-b2g:master

Cool and thanks Lancy!

Comment 48

[Jim Porter (:squib)]

Comment on attachment 8575060 [details] [review]
[gaia] jimporter:music-rtl-tiles > mozilla-b2g:master

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): N/A
[User impact] if declined: RTL locales will look bad if there is LTR artist/album names with parens in them
[Testing completed]: Manually tested
[Risk to taking this patch] (and alternatives if risky): Low risk, just a small CSS change
[String changes made]: None

Comment 49

[Jim Porter (:squib)]

(In reply to Jim Porter (:squib) from comment #48)
> Comment on attachment 8575060 [details] [review]
> [gaia] jimporter:music-rtl-tiles > mozilla-b2g:master

Er, I'm confused. This just changes the alignment of text so that it's consistent in RTL

Comment 50

[Ryan VanderMeulen [:RyanVM]]

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

Comment 51

[Sue]

Attachment: verified_v2.2_pass.png

This issue has been verified passed on latest build of Flame 2.2 with the same steps in comment 0.
See attachment:verified_v2.2_pass.png
Rate:0/3

Device: Flame 3.0 (pass)
Build ID               20150423160207
Gaia Revision          0c5e2ee1173f3c53379ef3cd10de714836258fe8
Gaia Date              2015-04-23 16:10:10
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/22a157f7feb7
Gecko Version          40.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150423.193607
Firmware Date          Thu Apr 23 19:36:18 EDT 2015
Bootloader             L1TC000118D0

Comment 52

[Sue]

Sorry for the wrong device information in comment 51, update it:

Device: Flame 2.2 (pass)
Build ID               20150423162502
Gaia Revision          b838d0e7c163e66660dcb6e387d8339944a7a30e
Gaia Date              2015-04-23 02:32:46
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/5fe76b26e55f
Gecko Version          37.0
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150423.195827
Firmware Date          Thu Apr 23 19:58:39 EDT 2015
Bootloader             L1TC000118D0