1140154 - [RTL][Music] Songs with parentheses are not displayed correctly

Status: VERIFIED FIXED

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

Description

[Pi Wei Cheng [:piwei] (inactive)]

Attachment: screenshot of issue

Description:
Music app displays songs with parentheses incorrectly.

Prerequisite:
In RTL language (Arabic)

STR:
1) Put a song with () in its title onto Flame
2) Open Music app and display the song in any of the tabs

Expected: () are displayed correctly. For example a song titled "Stricken (Live)" should be displayed as typed.

Actual: For the above example it displays as "(Stricken (Live". See screenshot.

This issue doesn't occur if you actually go into the song to view it though.

Device: Flame 3.0 Master (full flash 319MB)
BuildID: 20150305010212
Gaia: eff3321ab4e65da3f906688ebb55ddf1e93d9452
Gecko: 56492f7244a9
Gonk: e7c90613521145db090dd24147afd5ceb5703190
Version: 39.0a1 (3.0 Master)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:39.0) Gecko/39.0 Firefox/39.0

Comment 1

[Pi Wei Cheng [:piwei] (inactive)]

2.2 is also affected.

Device: Flame 2.2
BuildID: 20150305002528
Gaia: 89af288bad6751248ff84504fa898206fee127fe
Gecko: 6d8d294aa8f3
Gonk: e7c90613521145db090dd24147afd5ceb5703190
Version: 37.0 (2.2) 
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0

Comment 2

[Pi Wei Cheng [:piwei] (inactive)]

Actually looking into it further I found issue only occurs if there's parenthesis at the end of the title. For the attached screenshot you can see the parentheses in the middle are unaffected.

Comment 3

[Delphine Lebédel [:delphine - Use needinfo]]

I think this is the same issue as we have seen before, and it can be fixed on l10n side. Rami, if I'm mistaken, just let me know and I'll get this bug filtered out correctly again. Thanks!

Comment 4

[Rami Khader]

Is it possible to know what's the original string that will help me to fix it much faster.

Comment 5

[Anas Husseini [:linostar]]

I would say this isn't a bug, but rather an issue with bidirectionality. If the song name was in Arabic, the paranthesis at the end of the line (or at its beginning) will have the correct orientation. In the case where you see Arabic song names (with paranthesis) in an English interface, or English song names in Arabic interface (like your snapshot), the paranthesis at the edges of sentences will be reversed.

To solve this issue, you need to detect, per song name, the language it is written with (take note that an Arabic user may have both English and Arabic song names in his list), an enclose it with the corresponding RTL or LTR embeddings. So this is an RTL design issue rather than a localization issue (not to mention that song names are not localizable in the first place).

Comment 6

[Delphine Lebédel [:delphine - Use needinfo]]

Aha, thanks Linostar. We had similar bugs that could actually be fixed in the actual strings, but I understand here that won't work. I'll revert this bug back to RTL issues.

Comment 7

[Delphine Lebédel [:delphine - Use needinfo]]

Triage P2 - nominating as this is in the main Music screen. Since LTR written songs are popular, this is going to be a frequent bug

Comment 8

[Hema Koka [:hema]]

Dominic or Jim,

Could one of you please investigate this one? 

Thanks
Hema

Comment 9

[Jim Porter (:squib)]

(In reply to Anas Husseini [:linostar] from comment #5)
> To solve this issue, you need to detect, per song name, the language it is
> written with (take note that an Arabic user may have both English and Arabic
> song names in his list), an enclose it with the corresponding RTL or LTR
> embeddings. So this is an RTL design issue rather than a localization issue
> (not to mention that song names are not localizable in the first place).

Why doesn't Gecko do this itself? We don't actually know what language (or languages) are used in a given field. We can guess, but we don't have any information that Gecko wouldn't.

Comment 10

[Jim Porter (:squib)]

Here's (apparently) an algorithm that can handle this in general, but given that we've already branched for 2.2, I'm not sure we could land something this complex: <http://blogs.msdn.com/b/murrays/archive/2010/05/07/bidi-paragraph-with-parenthesized-text.aspx>

(It's also a bit surprising to me that the Unicode spec didn't come up with a smarter way to handle this for the simple case.)

Comment 11

[Jim Porter (:squib)]

Actually, it seems Unicode 6.3 incorporates some changes to make parens work right: <http://www.unicode.org/reports/tr9/#Paired_Brackets>. I don't know much about Unicode specs though, so we should probably find someone who works on Gecko and knows Unicode really well. I'm guessing this is a Gecko bug...

Comment 12

[David Flanagan [:djf]]

Will a <bdi> element around the song titles fix this?

Comment 13

[GitHub Autolander Bot]

Attachment: [gaia] dominickuo:bug-1140154 > mozilla-b2g:master

Comment 14

[Dominic Kuo [:dkuo]]

(In reply to David Flanagan [:djf] from comment #12)
> Will a <bdi> element around the song titles fix this?

Yes, I just tried it and it works! thanks David!

Comment 15

[Dominic Kuo [:dkuo]]

Comment on attachment 8575893 [details] [review]
[gaia] dominickuo:bug-1140154 > mozilla-b2g:master

I have fixed the paranthesis issue with the <bdi> elements and made some changes on the list elements when they are rtl, but not sure they are correct or not, set review to Jim first and I will probably update more after I read the spec again.

Comment 16

[howie [:howie]]

put Dominic as assignee

Comment 17

[Jim Porter (:squib)]

Comment on attachment 8575893 [details] [review]
[gaia] dominickuo:bug-1140154 > mozilla-b2g:master

I haven't run with this patch, but it looks sane.

If you like, you could simplify it by using CSS "unicode-bidi: isolate;" instead of using the <bdi> element and renaming variables like you did. It's up to you, though. The <bdi> element might be a better choice since it's more "semantic HTML".

I think this will still break for more complex cases (e.g. "ARABIC TEXT (English text)"), but that probably requires the Unicode paren-matching code I mentioned. If you'd like me to test the patch out with cases like that, just let me know.

Comment 18

[Lancy(Leave from Mozilla)]

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

Comment 19

[Josh Cheng [:josh]]

Dear Dominic,
Do you have any update? Thanks!

Comment 20

[Dominic Kuo [:dkuo]]

(In reply to Josh Cheng [:josh] from comment #19)
> Dear Dominic,
> Do you have any update? Thanks!

I have several bugs ongoing and hope back to this one today, and will need some time to address the issues Jim mentioned in comment 17.

Comment 21

[Dominic Kuo [:dkuo]]

(In reply to Jim Porter (:squib) from comment #17)
> If you like, you could simplify it by using CSS "unicode-bidi: isolate;"
> instead of using the <bdi> element and renaming variables like you did. It's
> up to you, though. The <bdi> element might be a better choice since it's
> more "semantic HTML".

I tried to use "unicode-bidi: isolate;" on the elements but seems not work for me, so I kept the original approach that use <bdi> to replace <span> elements, also with the necessary changes in CSS and HTML.

Comment 22

[GitHub Autolander Bot]

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

Comment 23

[Dominic Kuo [:dkuo]]

Comment on attachment 8575893 [details] [review]
[gaia] dominickuo:bug-1140154 > mozilla-b2g:master

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): RTL feature.
[User impact] if declined: bad ux if using music app in RTL.
[Testing completed]: css changes only for RTL styles, tests are not needed.(though modified js file but it's about html, not js logic).
[Risk to taking this patch] (and alternatives if risky): low.
[String changes made]: none.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

v2.2: https://github.com/mozilla-b2g/gaia/commit/116362f2c0048a5253d6a6a029913bd13812e5ac

Comment 25

[Kevin Grandon :kgrandon]

I pushed a hotfix to try to address the integration test issues on v2.2 after uplift. The test is currently disabled on master due to the TC switch, and is being investigated in another bug.

Failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-b2g37_v2_2&revision=98f70d16f582

Master hotfix: https://github.com/mozilla-b2g/gaia/commit/9aef68c5b9ea7b8215d6769827dcd91d2aff0922
v2.2 hotfix: https://github.com/mozilla-b2g/gaia/commit/283d966df6aae1627961850ebf98c58624a703bf

Comment 26

[Hubert Figuiere [:hub]]

Test disabled by bug 1148392

Comment 27

[Yeojin Chung [:YeojinC]]

This issue is verified fixed on Flame Master and 2.2.

Result: Songs with parentheses are displayed correctly in the Music app. 
(However, the parentheses in song titles are still located incorrectly in music widget and ringtone-related screens. Filed a separate bug 1150749)

Environmental Variables:
Device: Flame 3.0 (KK, 319mb, full flash)
Build ID: 20150402063750
Gaia: f37be8b44cb7c3a147b9615ab76743b760f08eeb
Gecko: 35046df9df1f
Gonk: b83fc73de7b64594cd74b33e498bf08332b5d87b
Version: 40.0a1 (Master)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:40.0) Gecko/40.0 Firefox/40.0

Environmental Variables:
Device: Flame 2.2 (KK, 319mb, full flash)
Build ID: 20150401162503
Gaia: 1ceca464053dee4a8bf10ea5abeef724d68c2ff2
Gecko: 427b4da96714
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

Comment 28

[Dominic Kuo [:dkuo]]

(In reply to Yeojin Chung [:YeojinC] from comment #27)
> Result: Songs with parentheses are displayed correctly in the Music app. 
> (However, the parentheses in song titles are still located incorrectly in
> music widget and ringtone-related screens. Filed a separate bug 1150749)

Those issues you saw are actually in different apps/modules(System, Settings and Ringtones) so they are expected, though we didn't found them before and they are not related to this patch.