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

VERIFIED FIXED in 2.2 S11 (1may)

Status

defect
P2
normal
VERIFIED FIXED
4 years ago
4 years ago

People

(Reporter: pcheng, Assigned: hub)

Tracking

unspecified
2.2 S11 (1may)
ARM
Gonk (Firefox OS)
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

(3 attachments)

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
(Reporter)

Comment 1

4 years ago
Posted image 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.
(Reporter)

Updated

4 years ago
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(ktucker)
Whiteboard: [3.0-Daily-Testing]
(Reporter)

Updated

4 years ago
OS: Linux → Gonk (Firefox OS)
Hardware: x86 → ARM
(Reporter)

Updated

4 years ago
Blocks: music-rtl
QA Whiteboard: [QAnalyst-Triage?] → [rtl-impact][QAnalyst-Triage?]
This is a regression: it got fixed a month back in Bug 1140154
Triage: P2 -- nominating
blocking-b2g: --- → 2.2?
Keywords: regression
Priority: -- → P2
(Reporter)

Comment 3

4 years ago
Note: only the behavior observed at step 3 of the STR is a regression, the rest is not regression.
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
QA Whiteboard: [rtl-impact][QAnalyst-Triage?] → [rtl-impact][QAnalyst-Triage+]
Flags: needinfo?(ktucker)
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.
Assignee: nobody → hub

Comment 6

4 years ago
Blocking Reason: poor ux and a missed test case from previous attempt to fix.
blocking-b2g: 2.2? → 2.2+
(Assignee)

Comment 8

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

This should fix the issues.
Attachment #8596146 - Flags: review?(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.
Keywords: regression
(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 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.
Attachment #8596146 - Flags: review?(dkuo) → review+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
(Assignee)

Comment 13

4 years ago
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
Attachment #8596146 - Flags: approval-gaia-v2.2?(bbajaj)
Attachment #8596146 - Flags: approval-gaia-v2.2?(bbajaj) → approval-gaia-v2.2+
(Reporter)

Comment 15

4 years ago
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
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)

Comment 16

4 years ago
Test case has been added in moztrap:
https://moztrap.mozilla.org/manage/case/16088/
Flags: in-moztrap+
You need to log in before you can comment on or make changes to this bug.