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

VERIFIED FIXED in 2.2 S9 (3apr)

Status

defect
P2
normal
VERIFIED FIXED
4 years ago
4 years ago

People

(Reporter: pcheng, Assigned: dkuo)

Tracking

unspecified
2.2 S9 (3apr)
ARM
Gonk (Firefox OS)
Dependency tree / graph
Bug Flags:
in-moztrap +

Firefox Tracking Flags

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

Details

(Whiteboard: [3.0-Daily-Testing])

Attachments

(2 attachments)

Posted image 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
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
Blocks: music-rtl
QA Whiteboard: [rtl-impact][QAnalyst-Triage?]
Flags: needinfo?(ktucker)
Whiteboard: [3.0-Daily-Testing]
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.
QA Whiteboard: [rtl-impact][QAnalyst-Triage?] → [rtl-impact][QAnalyst-Triage+]
Flags: needinfo?(ktucker)
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!
No longer blocks: music-rtl
Component: Gaia::Music → ar / Arabic
Flags: needinfo?(rami223)
Product: Firefox OS → Mozilla Localizations

Comment 4

4 years ago
Is it possible to know what's the original string that will help me to fix it much faster.
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).
Flags: needinfo?(rami223)
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.
Blocks: music-rtl
Component: ar / Arabic → Gaia::Music
Product: Mozilla Localizations → Firefox OS
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
blocking-b2g: --- → 2.2?
Priority: -- → P2

Comment 8

4 years ago
Dominic or Jim,

Could one of you please investigate this one? 

Thanks
Hema
Flags: needinfo?(squibblyflabbetydoo)
Flags: needinfo?(dkuo)
(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.
Flags: needinfo?(squibblyflabbetydoo)
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.)
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...
Will a <bdi> element around the song titles fix this?
Assignee

Comment 14

4 years ago
(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!
Flags: needinfo?(dkuo)
Assignee

Comment 15

4 years ago
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.
Attachment #8575893 - Flags: review?(squibblyflabbetydoo)
blocking-b2g: 2.2? → 2.2+
Assignee

Updated

4 years ago
Blocks: 1141950

Comment 16

4 years ago
put Dominic as assignee
Assignee: nobody → dkuo
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.
Attachment #8575893 - Flags: review?(squibblyflabbetydoo) → review+
Test case has been added in moztrap:
https://moztrap.mozilla.org/manage/case/16088/
Flags: in-moztrap+
Dear Dominic,
Do you have any update? Thanks!
Flags: needinfo?(dkuo)
Assignee

Comment 20

4 years ago
(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.
Flags: needinfo?(dkuo)
Assignee

Comment 21

4 years ago
(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.
Assignee

Updated

4 years ago
Keywords: checkin-needed
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Assignee

Comment 23

4 years ago
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.
Attachment #8575893 - Flags: approval-gaia-v2.2?
Attachment #8575893 - Flags: approval-gaia-v2.2? → approval-gaia-v2.2+
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
Test disabled by bug 1148392
See Also: → 1148392
Depends on: 1149886
See Also: → 1150749
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
Status: RESOLVED → VERIFIED
QA Whiteboard: [rtl-impact][QAnalyst-Triage+] → [rtl-impact][QAnalyst-Triage?]
Flags: needinfo?(ktucker)
QA Whiteboard: [rtl-impact][QAnalyst-Triage?] → [rtl-impact][QAnalyst-Triage+]
Flags: needinfo?(ktucker)
Assignee

Comment 28

4 years ago
(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.
You need to log in before you can comment on or make changes to this bug.