1150749 - [RTL][Systems] Parentheses in song title are not displayed correctly on music widget screens

Status: VERIFIED FIXED

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

Description

[Yeojin Chung [:YeojinC]]

Attachment: Parenthesis.png

Description:
Song title that contains parentheses is displayed incorrectly on lockscreen and notification tray. This issue also reproduces on Create Ringtone screen and Settings > Sound screen.

Pre-requisite: Have at least 1 song containing parentheses in the title on the device.

Repro Steps:
1) Update a Flame to 20150402063750.
2) Set the device language in Arabic under Settings > Language.
3) Open Music app.
4) Play the song with the parentheses in the title.
5) Pull down the notification tray, or bring the lockscreen by pressing the power button twice.
6) Observe the parentheses in the song title.

Actual:
The parentheses are not located correctly.

Expected:
The parentheses are located correctly.

Note:
- The parentheses are also mis-located on "Create Ringtone" page, and on Settings >  Sound page when the song is set as ringtone.
- The parentheses are displayed correctly within the Music app. See bug 1140154.

Environmental Variables:
Device: Flame Master (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

Repro frequency: 5/5
See attached: screenshot

Comment 1

[Yeojin Chung [:YeojinC]]

This issue also reproduces on Flame 2.2.

Result: The parentheses are not located correctly on music widget and ringtone-related screens.

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 2

[Gregor Wagner [:gwagner]]

Created a new bug for ringtone screen issues.
Ted, can you take a look at the music widget?

Comment 3

[Ted Clancy [:tedders1]]

Hi Yeojin,

Could you please attach the song you were listening to when you took those pictures? Thanks.

Comment 4

[Ted Clancy [:tedders1]]

Note to testers:

The trick to reproducing this is that you need a song where the artist's name is in a right-to-left script while the song name is in a left-to-right script. And the song name needs to end with a bracket (parenthesis).

Comment 5

[Yeojin Chung [:YeojinC]]

Attachment: Death Grips - The Fever (Aye Aye).mp3

Sorry, I do not have the exact same file anymore, but the attached file should trigger the issue as well.

I was unable to reproduce this issue on the music widget on today's Flame master. However, I still saw the issue on the ringtone-related screens, such as "Create ringtone" screen, and Settings > Sound.

Device: Flame 3.0 (KK, 319mb, full flash)
Build ID: 20150406010204
Gaia: ef61ebbe5de8c2c9fc2a8f74a12455044c3b82e9
Gecko: 4fe763cbe196
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 6

[Dominic Kuo [:dkuo]]

(In reply to Ted Clancy [:tedders1] from comment #3)
> Hi Yeojin,
> 
> Could you please attach the song you were listening to when you took those
> pictures? Thanks.

Hey Ted, actually you can choose any mp3 file and edit the tags with tag editor, like [1]. It should be able to modify any fields then you can test them with your patch. Hope this helps :)

[1] http://kid3.sourceforge.net/

Comment 7

[GitHub Autolander Bot]

Attachment: [gaia] tedders1:bug-1150749-fix > mozilla-b2g:master

Comment 8

[Ted Clancy [:tedders1]]

(In reply to Yeojin Chung [:YeojinC] from comment #5)

> Sorry, I do not have the exact same file anymore, but the attached file
> should trigger the issue as well.
> 
> I was unable to reproduce this issue on the music widget on today's Flame
> master. 

Yeojin, this bug only occurs when the Artist name is in a right-to-left script. The file you attached won't trigger the issue.

Comment 9

[David Flanagan [:djf]]

Comment on attachment 8588966 [details] [review]
[gaia] tedders1:bug-1150749-fix > mozilla-b2g:master

The basic approach looks solid here, but some nits, and r- because of innerHtml which isn't even a thing, and would have been the wrong thing even if it was :-)

Please as Jim (:squib) or Dominic (:dkuo) for the next review, since they are more familiar with this code than I am.

Comment 10

[Jim Porter (:squib)]

Comment on attachment 8588966 [details] [review]
[gaia] tedders1:bug-1150749-fix > mozilla-b2g:master

This looks basically good to me, but I'm not a system app peer (I did write this code originally, though). I'd recommend having someone on the system app take a look at this too.

Comment 11

[Ted Clancy [:tedders1]]

Comment on attachment 8588966 [details] [review]
[gaia] tedders1:bug-1150749-fix > mozilla-b2g:master

Adding :etienne to the review since he's a system peer.

Comment 12

[Etienne Segonzac (:etienne)]

Comment on attachment 8588966 [details] [review]
[gaia] tedders1:bug-1150749-fix > mozilla-b2g:master

Can you add a small test in apps/system/test/unit/media_playback_test.js to make sure the bdi nodes are created correctly?
r=me with that

Also adding Greg for a quick look at the lockscreen part since the files aren't exactly the same :)

Comment 13

[Ted Clancy [:tedders1]]

Okay, I added a test.

Comment 14

[Greg Weng [:snowmantw][:gweng][:λ]]

Well I've found the patch is to make them as (almost) the same, so it' good. However, since in System app we need to test every file you patched, so please add one unit test for lockscreen_media_playback.js, thanks.

Comment 15

[Ted Clancy [:tedders1]]

Hi Greg. I've added a test to lockscreen_media_playback.js.

Comment 16

[Greg Weng [:snowmantw][:gweng][:λ]]

Comment on attachment 8588966 [details] [review]
[gaia] tedders1:bug-1150749-fix > mozilla-b2g:master

OK thanks.

Comment 17

[GitHub Autolander Bot]

Autolander could not locate a review from a user within the suggested reviewer list. Either the patch author or the reviewer should be in the suggested reviewer list.

Comment 18

[Ted Clancy [:tedders1]]

Can someone tell me what Autolander's problem is? I've got r+'s from :squib, :etienne, and :gweng. 

:squib is a peer on the Music app, and :etienne is a peer on the system app.

Isn't that enough?

Comment 19

[Greg Weng [:snowmantw][:gweng][:λ]]

Etienne, could you help with this? Since I'm not owner, and I'm not the recommended reviewer.

Comment 20

[Kevin Grandon :kgrandon]

(In reply to Ted Clancy [:tedders1] from comment #18)
> Can someone tell me what Autolander's problem is? I've got r+'s from :squib,
> :etienne, and :gweng. 
> 
> :squib is a peer on the Music app, and :etienne is a peer on the system app.
> 
> Isn't that enough?

You removed the review+ from Jim here in comment 11. I recommend getting another review, or landing manually.

Comment 21

[Kevin Grandon :kgrandon]

Manually landing for now: https://github.com/mozilla-b2g/gaia/commit/a68db278aa7f792c7e9f22b20ca6f160214d6360

Comment 22

[Ted Clancy [:tedders1]]

Thanks, Kevin.

Comment 23

[Ted Clancy [:tedders1]]

Comment on attachment 8588966 [details] [review]
[gaia] tedders1:bug-1150749-fix > mozilla-b2g:master

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #):
RTL support (Bug 1064539)

[User impact] if declined:
The text displayed to the use will be jumbled.

[Testing completed]:
Green try run: https://treeherder.mozilla.org/#/jobs?repo=gaia&revision=3f78524835d223023589b7378d1f121d8f585dd3

[Risk to taking this patch] (and alternatives if risky):
None forseen. This is a cosmetic change to two screens.

[String changes made]:
None.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

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

Comment 25

[Sue]

Attachment: screenshot.png

This issue has been verified fail on latest build of Flame 2.2/3.0 with the same steps in comment 0.
After step 4, the parentheses of the music title in headers are not located correctly.
But after step 5, the parentheses of the music title are located correctly. 
See attachment:screenshot.png
Rate:5/5

Device: Flame 2.2 (fail)
Build ID               20150419002502
Gaia Revision          c15a2b6d3a783813959c2b3bffd2a131f4270b9e
Gaia Date              2015-04-17 17:49:32
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/cc02ee38b252
Gecko Version          37.0
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150419.040848
Firmware Date          Sun Apr 19 04:08:59 EDT 2015
Bootloader             L1TC000118D0

Device: Flame 3.0 (fail)
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 26

[Sue]

Hi Josh,
According to comment 25, the parentheses of the music title in headers are not located correctly. Shall we open a new bug to trace it? I think this issue is used for tracing the bug that the parentheses of the music title on music widget screens is shown wrongly in notification tray and lockscreen. And now, the parentheses of the music title in notification tray and lockscreen are located correctly.

Comment 27

[Dominic Kuo [:dkuo]]

(In reply to Sue from comment #26)
> Hi Josh,
> According to comment 25, the parentheses of the music title in headers are
> not located correctly. Shall we open a new bug to trace it? I think this
> issue is used for tracing the bug that the parentheses of the music title on
> music widget screens is shown wrongly in notification tray and lockscreen.
> And now, the parentheses of the music title in notification tray and
> lockscreen are located correctly.

Lockscreen music widget, utility music widget and Music app are different apps/modules(System and Music), attachment 8594643 [details] is actually showing an issue in Music app, not System app which we have already fixed in this bug, so it's better to file a new bug in Gaia::Music component instead of reopening this one, thanks.

Comment 28

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

This issue is verified fixed on Flame 2.2 and 3.0 for the music widget on lockscreen and utility tray.

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

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

-----

There are still two outstanding issues, one occurs on Settings app > Sound > ringtone (if the song with parentheses is set as ringtone) - this is being covered in bug 1151496.

Another is a partial re-occurrence since bug 1140154 - that parentheses are not displayed correctly while viewing the song in Music app. I have filed bug 1156571 for follow-up on that issue and hope that this covers all scenarios in Music app.

Comment 29

[Sue]

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