1156571 - [RTL][Music] Songs with parentheses are not displayed correctly in Music app

Status: VERIFIED FIXED

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

Description

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

Attachment: song used for testing with parentheses

Description:
In Music app tile view, as well as active song playing view, parentheses are not displayed correctly in Arabic.

STR:
0) Set device language to Arabic
1) Put the attached song to the device
2) Go to Music app observe the song on tile view
3) Start playing the song, and observe the song playing screen
4) Tap on screen to invoke the share menu

Expected on step 2, 3, and 4: Parentheses are displayed correctly.

Actual on step 2, 3, and 4: Parentheses are mismatched. See screenshots on next comment.

Device: Flame 3.0
BuildID: 20150420010204
Gaia: cb41d8421da5dc4f16ea566ea2917a9b7f828154
Gecko: 50b95032152c
Gonk: b83fc73de7b64594cd74b33e498bf08332b5d87b
Version: 40.0a1 (3.0) 
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:40.0) Gecko/40.0 Firefox/40.0

Comment 1

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

Attachment: screenshots.png

2.2 is also affected.

Device: Flame 2.2
BuildID: 20150420002502
Gaia: c15a2b6d3a783813959c2b3bffd2a131f4270b9e
Gecko: cc02ee38b252
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

Attaching screenshots of this issue.

Comment 2

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

This is a regression: it got fixed a month back in Bug 1140154
Triage: P2 -- nominating

Comment 3

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

Note: only the behavior observed at step 3 of the STR is a regression, the rest is not regression.

Comment 4

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

Also, I realize I must have been looking at screenshots from Bug 1140154 instead of this one, so I thought this was exactly the same issue on the same screen. Looking once more at the screenshots here, I realize they are not from exactly the same screen

Comment 5

[David Flanagan [:djf]]

I'm disappointed that we didn't get these cases handled as part of bug 1140154.  I suspect the fix here is just to add <bdi> elements around the song name in the gaia-header and in the overlay where the artist name is displayed.
 
I've asked Hub to take this one.

Comment 6

[Hema Koka [:hema]]

Blocking Reason: poor ux and a missed test case from previous attempt to fix.

Comment 7

[GitHub Autolander Bot]

Attachment: [gaia] hfiguiere:bug1156571-bidi-fixes > mozilla-b2g:master

Comment 8

[Hubert Figuiere [:hub]]

Comment on attachment 8596146 [details] [review]
[gaia] hfiguiere:bug1156571-bidi-fixes > mozilla-b2g:master

This should fix the issues.

Comment 9

[Dominic Kuo [:dkuo]]

(In reply to Delphine Lebédel [:delphine - use need info] from comment #2)
> This is a regression: it got fixed a month back in Bug 1140154
> Triage: P2 -- nominating

Bug 1140154 only fixed the part in the album detail view(sublist view), Music app didn't handle bdi parentheses before(it's reproducible on v2.1 already) so this is something we missed to implement.

Comment 10

[Dominic Kuo [:dkuo]]

(In reply to David Flanagan [:djf] from comment #5)
> I'm disappointed that we didn't get these cases handled as part of bug
> 1140154.  I suspect the fix here is just to add <bdi> elements around the
> song name in the gaia-header and in the overlay where the artist name is
> displayed.
>  
> I've asked Hub to take this one.

Sorry about bug 1140154, I worked on it and my patch was mainly about adjusting styles of the list elements and sublist view in RTL, but didn't pay attention to the other views also needed to handle the parentheses in RTL. I guess there are some similar issues among the other gaia apps(Settings and Ringtones for example), and we should probably notify the owners that, parentheses should be handled by bdi elements.

Comment 11

[Dominic Kuo [:dkuo]]

Comment on attachment 8596146 [details] [review]
[gaia] hfiguiere:bug1156571-bidi-fixes > mozilla-b2g:master

Hub, thanks for working on this, overall the changes looks good to me, note that because the html of player view is in both index.html and open.html, so be sure to make the same changes into open.html and keep the open activity working as usual before landing it.

Comment 12

[GitHub Autolander Bot]

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

Comment 13

[Hubert Figuiere [:hub]]

Comment on attachment 8596146 [details] [review]
[gaia] hfiguiere:bug1156571-bidi-fixes > mozilla-b2g:master

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): RTL blocker
[User impact] if declined: RTL bugs
[Testing completed]: manual testing
[Risk to taking this patch] (and alternatives if risky):
[String changes made]: none

Comment 14

[Ryan VanderMeulen [:RyanVM]]

v2.2: https://github.com/mozilla-b2g/gaia/commit/071c5d08419e5eb0209caea3526ff5ec729fe6ba

Comment 15

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

This issue is verified fixed on Flame 3.0 and 2.2. All the areas that were mentioned in bug write-up now display correctly formatted parentheses in Music app.

Device: Flame 3.0 Master (full flashed 319MB KK)
BuildID: 20150427010202
Gaia: b4c949cdc780893897c9b45c1adea46e2eb694ff
Gecko: 37d60e3b8be6
Gonk: b83fc73de7b64594cd74b33e498bf08332b5d87b
Version: 40.0a1 (3.0 Master)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:40.0) Gecko/40.0 Firefox/40.0

Device: Flame 2.2 (full flashed 319MB KK)
BuildID: 20150427002504
Gaia: 265ca0bc9408c21fc4b25a259fcee7fb642cd06b
Gecko: 1908685d798d
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 16

[Sue]

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