1159092 - [RTL][Notifications]The ellipsis of long LTR music name is located at wrong side in notification bar and lockscreen

Status: VERIFIED FIXED

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

Description

[Sue]

Attachment: ellipsis_in_notification_bar.png

[1.Description]:
[RTL][Flame v2.2 & v3.0][Nexus 5 2.2 & 3.0][Notifications]When user plays a music with long LTR name, the ellipsis is located at wrong side on the ongoing music process in notification bar. 
See attachment:ellipsis_in_notification_bar.png

[2.Testing Steps]: 
Prerequisite: Have a music with long LTR name in device. 
1. Set system language as Arabic.
2. Launch Music app.
3. Play the music which has the long LTR name.
4. Pull down the notification bar and observe the ongoing music process on notification bar.

[3.Expected Result]: 
4. The ellipsis should be located at right side of LTR name.

[4.Actual Result]: 
4. The ellipsis is located at left side of LTR name.

[5.Reproduction build]: 
Device: Flame 2.2 (affected)
Build ID               20150427002504
Gaia Revision          265ca0bc9408c21fc4b25a259fcee7fb642cd06b
Gaia Date              2015-04-24 19:13:28
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/1908685d798d
Gecko Version          37.0
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150427.040113
Firmware Date          Mon Apr 27 04:01:24 EDT 2015
Bootloader             L1TC000118D0

Device: Flame 3.0 (affected)
Build ID               20150427160201
Gaia Revision          0636405f0844bf32451a375b2d61a2b16fe33348
Gaia Date              2015-04-27 16:42:28
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/caf25344f73e
Gecko Version          40.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150427.192938
Firmware Date          Mon Apr 27 19:29:49 EDT 2015
Bootloader             L1TC000118D0

Device: Nexus 5 2.2 (affected)
Build ID               20150427002504
Gaia Revision          265ca0bc9408c21fc4b25a259fcee7fb642cd06b
Gaia Date              2015-04-24 19:13:28
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/1908685d798d
Gecko Version          37.0
Device Name            hammerhead
Firmware(Release)      5.1
Firmware(Incremental)  eng.cltbld.20150427.035747
Firmware Date          Mon Apr 27 03:58:04 EDT 2015
Bootloader             HHZ12f

Device: Nexus 5 3.0 (affected)
Build ID               20150427160201
Gaia Revision          0636405f0844bf32451a375b2d61a2b16fe33348
Gaia Date              2015-04-27 16:42:28
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/caf25344f73e
Gecko Version          40.0a1
Device Name            hammerhead
Firmware(Release)      5.1
Firmware(Incremental)  eng.cltbld.20150427.192534
Firmware Date          Mon Apr 27 19:25:51 EDT 2015
Bootloader             HHZ12f

[6.Reproduction Frequency]: 
Always Recurrence,5/5

[7.TCID]: 
Free Test

Comment 1

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

Triage -- P2

Comment 2

[David Flanagan [:djf]]

The same bug exists for the lockscreen, and we might as well fix them both in the same patch.

Comment 3

[David Flanagan [:djf]]

This bug is related to bug 1150749 which added <bdi> elements to the notification screen and lockscreen to hold the artist and track name so that parentheses would display correctly. 

So to fix the parenthesis bug, the markup is now like this:

  <div><bdi>title</bdi> - <bdi>artist</bdi></div>

The text-overflow:ellipsis style is on the div element, but because the title and artist strings are isolated in bdi tags, it just uses the global rtl to decide where to put the ellipsis.

If we could assume that the title and artist names would always be in the same language and could use a single bdi element, then we could just put text-overflow:ellipsis on the bdi element and the problem would be solved.

But bug 1150749 actually showed sample screenshots where one was in Arabic and one was in English, so I think we do need both bdi elements.

Comment 4

[David Flanagan [:djf]]

Attachment: test tracks with bidi metadata

This zip file contains 8 .ogg files. They are just short recordings of my voice, but each has different metadata for artist and track name.

ee.ogg has an english track name and english artist name.
ea.ogg has an english track name and arabic artist name.
ae.ogg has an arabic track name and an english artist name.
aa.ogg has both names in arabic.

The long_ variants of these files have longer track and artist names so that we can check whether the ellipsis is displayed on the right side.

In all of the files, the track name contains parentheses so we can test that those display correctly in RTL locales.

I have no idea what the Arabic text means. I cut-and-pasted it from http://generator.lorem-ipsum.info/_arabic

Comment 5

[David Flanagan [:djf]]

Comment on attachment 8598938 [details]
test tracks with bidi metadata

Delphine: do these sample tracks seem like they will be useful? Can you tell whether the Arabic is well-enough formed for them to be valid test cases?

Comment 6

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

They seem like they are valid, however I'm not a native speaker so can't tall if the Arabic is well-enough formed.  Flagging localizers for this. thanks

Comment 7

[David Flanagan [:djf]]

I think I can fix this bug by changing the markup from:

  <div><bdi>title</bdi> - <bdi>artist</bdi></div>

To

  <bdi>title - <bdi>artist</bdi></bdi>

With this layout, the language of the song determines the overall direction.  If the song title is LTR then we have one of these:

   song - artist
   song - tsitra

And in these cases, the ellipsis is on the right if it is needed.

If the song is RTL, then we have:

   artist - gnos
   tsitra - gnos

And the ellipsis is on the left if it is needed.

This may not be perfect if artist name is a different directionality than the song name and if it is the artist name that is being truncated with the ellipsis, but I don't know how else to fix it.

Comment 8

[David Flanagan [:djf]]

I suppose that a proper fix would be to redesign the UI so that the artist and song title are on separate lines and they can each have their own ellipsis on the apprpriate side based on their own direction.  But until we can make a change like that, I'll attach the patch that does what is described in comment #7

Comment 9

[GitHub Autolander Bot]

Attachment: [gaia] davidflanagan:bug1159092 > mozilla-b2g:master

Comment 10

[David Flanagan [:djf]]

Comment on attachment 8598970 [details] [review]
[gaia] davidflanagan:bug1159092 > mozilla-b2g:master

Delphine, Rami, Ahmed: Does this patch look like it does the right thing to you?  (If you can't run the patch, could you at least give feedback on what I describe in comment #7?)  Basically, this patch uses the directionality of the song name to decide on the position of the ellipsis, even if the artist name is the thing being truncated and the artist name has a different directionality than the song name.  This doesn't seem perfect, but it is the closest I can figure out how to get.

Ted: I'm asking for your feedback because this patch modifies what you ddid for bug 1150749

Dominic: I think you're probably the original author of these controls, so you're probably the right person to review this.

Comment 11

[Dominic Kuo [:dkuo]]

Comment on attachment 8598970 [details] [review]
[gaia] davidflanagan:bug1159092 > mozilla-b2g:master

David,

I think your patch does what you said in comment 7 so looks good to me, also I agree we might need to re-design the ui, or this problem seems no perfect solution for it, but the patch should be fine to handle most of the cases, thanks for working on this!

Comment 12

[David Flanagan [:djf]]

I updated the unit tests, but didn't get them quite right. I'll update the patch with test tweaks that should not affect Dominic's r+

Comment 13

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

Comment on attachment 8598970 [details] [review]
[gaia] davidflanagan:bug1159092 > mozilla-b2g:master

Sorry I'm not the right person for asking feedback and review here, so taking the flags off me. thanks

Comment 14

[David Flanagan [:djf]]

Delphine: you set this as a P2 bug. Now that the 2.2 FC deadline has passed, I don't know if RTL bugs should still be nominated for uplift or not.

Comment 15

[Ted Clancy [:tedders1]]

Comment on attachment 8598970 [details] [review]
[gaia] davidflanagan:bug1159092 > mozilla-b2g:master

David, I don't think removing the <bdi> tags around the track's title is the right thing to do. If the user interface is in English, and the artist name is in English, but the title is in Arabic, won't the dash end up on the wrong side? It won't be between the title and artist like we expect. (That was the problem in Bug 1150749.)

But don't worry, this bug will be fixed by Bug 883884, which I'm currently working on. (I wish I could give you a workaround in the meantime, but I don't think there is a good one.)

If you want, you can mark this bug as dependent on 883884.

Comment 16

[Ted Clancy [:tedders1]]

(In reply to Ted Clancy [:tedders1] from comment #15)
If the user interface is in English, and the artist name
> is in English, but the title is in Arabic, won't the dash end up on the
> wrong side? It won't be between the title and artist like we expect.

Oh wait, nevermind. I'm lying. The dash will end up on the left, but so will the Artist name, so it will still look okay. It's not in the order the user will be expecting, but I think that's okay.

Comment 17

[David Flanagan [:djf]]

Attachment: ltr locale, english artist, arabic song name

Ted: The mdash is still in the middle in the case you describe, though the song name is on the right of the dash instead of the left. Note (see comment #7) that there are still two bdi tags, but now one is nested inside the other rather than having them independent.

I suspect that by far the most common case will be ltr song name and ltr artist name displayed in an rtl locale, and I'm going to land this to get the ellipsis handled correctly in that case, even if the bidi handling is imperfect.

Comment 18

[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 19

[David Flanagan [:djf]]

I'm going to override autolander on that. dkuo is the appropriate reviewer for this patch.

Landed on master: https://github.com/mozilla-b2g/gaia/commit/4e2ff35414a885831fdd8932a5ccb226b99eb030

Comment 20

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

Hey David! I think that's more a question for RelMan (that said, I'm still seeing things getting approval for uplift on 2.2 today). thanks!

Comment 22

[bhavana bajaj [:bajaj]]

(In reply to David Flanagan [:djf] from comment #14)
> Delphine: you set this as a P2 bug. Now that the 2.2 FC deadline has passed,
> I don't know if RTL bugs should still be nominated for uplift or not.

I think it depends on user impact/risk, but I would highly avoid taking anything less critical on 2.2 at this time.

Comment 23

[Sue]

Attachment: verify_v3.0_pass.png

This issue has been verified passed on latest build of Flame 3.0 and Nexus 5 3.0 with the same steps in comment 0.
Result: The ellipsis of long LTR music name is shown correctly in notification bar and lockscreen.
See attachment:verify_v3.0_pass.png
Rate:0/5

Device: Flame 3.0 (pass)
Build ID               20150504160201
Gaia Revision          70077825aab2c7a79611befb40a5fe7e610d5443
Gaia Date              2015-05-04 18:09:33
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/102d0e9aa9e1
Gecko Version          40.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150504.193019
Firmware Date          Mon May  4 19:30:30 EDT 2015
Bootloader             L1TC000118D0

Device: Nexus 5 3.0 (pass)
Build ID               20150504160201
Gaia Revision          70077825aab2c7a79611befb40a5fe7e610d5443
Gaia Date              2015-05-04 18:09:33
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/102d0e9aa9e1
Gecko Version          40.0a1
Device Name            hammerhead
Firmware(Release)      5.1
Firmware(Incremental)  eng.cltbld.20150504.193136
Firmware Date          Mon May  4 19:31:54 EDT 2015
Bootloader             HHZ12f

Comment 24

[Rami Khader]

From image, I can see that the song name in Arabic (right aligned) and the artist name in English correctly aligned.
Looks ok from my side

Comment 25

[Sue]

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

Comment 26

[Sue]

Hi David,
Could you please help to confirm whether it will uplift to v2.2 or not? Thanks :)

Comment 27

[David Flanagan [:djf]]

Comment on attachment 8598970 [details] [review]
[gaia] davidflanagan:bug1159092 > mozilla-b2g:master

I didn't get a clear answer from Delphine or Bhavana about whether this patch should be uplifted, so I guess I'll go ahead and nominate it and let someone else decide, since the reporter has pinged me about uplift.

I honestly don't know how urgent RTL bugs like this are still considered for 2.2. This particular bug is kind of a corner case since it only affects where the ellipsis goes when the song name or artist name is too long. But it does affect text that appears on the lockscreen, so I suppose that could make it more visible and higher priority to fix.

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): not a regression, just a very tricky bidi corner case

[User impact] if declined: ellipsis may be on the wrong side for long song names or artist names when direction of locale and music metadata do not match

[Testing completed]: yes

[Risk to taking this patch] (and alternatives if risky): a relatively small patch that has been on master for a couple of weeks, so not very risky.

[String changes made]: none

Comment 28

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

(In reply to David Flanagan [:djf] from comment #27)
> Comment on attachment 8598970 [details] [review]
> [gaia] davidflanagan:bug1159092 > mozilla-b2g:master
> 
> I didn't get a clear answer from Delphine or Bhavana about whether this
> patch should be uplifted, so I guess I'll go ahead and nominate it and let
> someone else decide, since the reporter has pinged me about uplift.

Hey David - yeah I'm an l10n-driver so it's not my decision if this gets uplifted or not. Need info'ing Josh for decision since he's RelMan now is the right way to go forwards :) thanks!

Comment 29

[Norry.L.F(Leave from Mozilla)]

Hi Josh,

Could you help to confirm if it will uplift to v2.2? 

Thanks

Comment 30

[Josh Cheng [:josh]]

Comment on attachment 8598970 [details] [review]
[gaia] davidflanagan:bug1159092 > mozilla-b2g:master

Approving and please NI if causing any issue.

Hi Norry,
Please verify on 2.2 with LTR and RTL to make sure no regression.
Thanks.

Comment 31

[Ryan VanderMeulen [:RyanVM]]

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

Comment 32

[Sue]

Attachment: verify_v2.2_pass.png

This issue has been verified as pass on latest build of Flame 2.2 and Nexus 5 2.2 by STRs in comment 0.
Result:The ellipsis of long LTR music name is shown correctly in notification bar and lockscreen.
See attachment:verify_v2.2_pass.png
Rate:0/5

Device: Flame 2.2 (pass)
Build ID               20150524002504
Gaia Revision          de5942bd99b801d4d04b676f1e70a77e93115d95
Gaia Date              2015-05-22 19:38:55
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/1bd024f4e171
Gecko Version          37.0
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150524.040835
Firmware Date          Sun May 24 04:08:46 EDT 2015
Bootloader             L1TC000118D0

Device: Nexus 5 2.2 (pass)
Build ID               20150524002504
Gaia Revision          de5942bd99b801d4d04b676f1e70a77e93115d95
Gaia Date              2015-05-22 19:38:55
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/1bd024f4e171
Gecko Version          37.0
Device Name            hammerhead
Firmware(Release)      5.1
Firmware(Incremental)  eng.cltbld.20150524.043322
Firmware Date          Sun May 24 04:33:38 EDT 2015
Bootloader             HHZ12f