1158043 - [RTL][Music]The ellipsis is located at wrong side in headers of music app

Status: VERIFIED FIXED

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

Description

[Sue]

Attachment: ellipsis_in_music.png

[1.Description]:
[RTL][Flame v2.2 & v3.0][Nexus5 2.2 & 3.0][Music]The ellipsis is located at wrong side in headers of music app and the ongoing music process on notification bar.
See attachment:ellipsis_in_music.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. Observe the headers.
5. Pull down the notification bar and observe the ongoing music process on notification bar.

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

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

[5.Reproduction build]: 
Device: Flame 2.2 (affected)
Build ID               20150423162502
Gaia Revision          b838d0e7c163e66660dcb6e387d8339944a7a30e
Gaia Date              2015-04-23 02:32:46
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/5fe76b26e55f
Gecko Version          37.0
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150423.195827
Firmware Date          Thu Apr 23 19:58:39 EDT 2015
Bootloader             L1TC000118D0

Device: Flame 3.0 (affected)
Build ID               20150423160207
Gaia Revision          0c5e2ee1173f3c53379ef3cd10de714836258fe8
Gaia Date              2015-04-23 16:10:10
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/22a157f7feb7
Gecko Version          40.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150423.193607
Firmware Date          Thu Apr 23 19:36:18 EDT 2015
Bootloader             L1TC000118D0

Device: Nexus 5 2.2 (affected)
Build ID               20150423002502
Gaia Revision          b838d0e7c163e66660dcb6e387d8339944a7a30e
Gaia Date              2015-04-23 02:32:46
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/8dce56574f28
Gecko Version          37.0
Device Name            hammerhead
Firmware(Release)      5.1
Firmware(Incremental)  eng.cltbld.20150423.035409
Firmware Date          Thu Apr 23 03:54:27 EDT 2015
Bootloader             HHZ12f

Device: Nexus 5 3.0 (affected)
Build ID               20150423160207
Gaia Revision          0c5e2ee1173f3c53379ef3cd10de714836258fe8
Gaia Date              2015-04-23 16:10:10
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/22a157f7feb7
Gecko Version          40.0a1
Device Name            hammerhead
Firmware(Release)      5.1
Firmware(Incremental)  eng.cltbld.20150423.192918
Firmware Date          Thu Apr 23 19:29:36 EDT 2015
Bootloader             HHZ12f

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

[7.TCID]: 
Free Test

Comment 1

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

These are 2 separate issues. Sue: please file another bug for the 2nd one. Renaming this one
Triage: P2 -- putting ni on me to investigate if this is a regression today

Comment 2

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

I just tried on an April 2nd build and this was fixed. Trying on a build from the 8th, will report results once that's done.
Regression, so nominating and adding QA wanted keyword for further investigation.

Comment 3

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

Attachment: 2015-04-25-13-35-22.png

This is an April 8th build, the ellipsis was correctly placed in an LTR title.

Comment 4

[Josh Cheng [:josh]]

Sue,
Please open another bug per comment 1 from Delphine.

Comment 5

[Josh Cheng [:josh]]

Dear Dominic,
Could you please help to check the problem? Thanks!

Comment 6

[David Flanagan [:djf]]

Hub: is htis fixed by (or caused by?) your recent bdi patch? If not, could you see if you can fix it today?

Comment 7

[Hubert Figuiere [:hub]]

It is clearly *not* caused by my fix because the gaia revision used to test (and allegedly affected) is actually the parent commit of my change....

commit 42fd6933c82796870eaaacbed667f86217243d75
Merge: 0c5e2ee 38a52b4
Author: autolander <[e-mail]>
Date:   Thu Apr 23 10:00:05 2015 -0700

    Bug 1156571 - merge pull request #29671 from hfiguiere:bug1156571-bidi-fixes to mozilla-b2g:master

commit 0c5e2ee1173f3c53379ef3cd10de714836258fe8
Merge: 4177209 411ea56
Author: autolander <[e-mail]>
Date:   Thu Apr 23 09:10:10 2015 -0700

    Bug 1148342 - merge pull request #29672 from davidflanagan:bug1148342 to mozilla-b2g:master


0c5e2ee1173f3c53379ef3cd10de714836258fe8 is the revision tested.

Comment 8

[David Flanagan [:djf]]

If we're lucky, then, maybe your bdi patch will resolve this issue, too.  If not, I know someone on our team (maybe Punam?) has fixed ellipsis on the wrong side bugs, so there is probably someone who can tell us how to most easily fix this.

Comment 9

[Hubert Figuiere [:hub]]

FWIW, I don't have a sample to test it :-/

Comment 10

[David Flanagan [:djf]]

Hub: you can use test_media/samples/Music/treasure_island_01-02_stevenson.ogg as a sample. It has a very long LTR name

Comment 11

[Hubert Figuiere [:hub]]

This is happening with the sample mentionned above.

Comment 12

[David Flanagan [:djf]]

I also see it in the tiles view... "Robert Lewis Stevenson" is coming out as "...is Stevenson" instead of "Robert Lewis..." or something.  But it does not appear in the list of all songs. The ellipsis is correctly on the right there.

The <bdi> element seems to have no effect on the position of the ellipsis.

It is not obvious to me how to change the side that the ellipsis goes on, but I know that this is a bug that has been solved before, so we can probably search for it and find an example. Punam thought that maybe she'd see something like that for the SMS app...

Comment 13

[KTucker [:KTucker][Inactive 3/4/2016]]

Based on Comment 3 let's get a regression window here.

Comment 14

[David Flanagan [:djf]]

Ah. Bug 1157610 solved this problem. Looks like this code will fix the header:

#title-text bdi {
  display:block;
  overflow: hidden;
  text-overflow: ellipsis;
}

Presumably something similar in the header for the individual tiles will fix those. 

Jim: you just fixed a blocker by adding bdi elements, too. Setting needinfo for you because you might want to check for ellipsis issues with those bdi tags.

Comment 15

[David Flanagan [:djf]]

Oops. Looks like I accidentally reset KTucker's regressionwindow-wanted flag. Putting that back.

I'd check bug 1154025 first. That's where I'd bet the regression lies.

Comment 16

[Brogan Zumwalt [Inactive]]

Last working B2G-Inbound build:
Device: Flame 3.0
Build ID: 20150416191748
Gaia: 29d3a84bbe41d4a3d0496660ab6e97d2352cc8c2
Gecko: f7f06b74b198
Version: 40.0a1 (3.0)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:40.0) Gecko/40.0 Firefox/40.0

First broken B2G-Inbound build:
Device:  Flame 3.0
BuildID: 20150416230239
Gaia: ae7527a6e6892b92b67925b3c892e03c6d920ff3
Gecko: 8d663ed59536
Version: 40.0a1 (3.0)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:40.0) Gecko/40.0 Firefox/40.0


Working Gaia with Broken Gecko issue does NOT reproduce:
Gaia: 29d3a84bbe41d4a3d0496660ab6e97d2352cc8c2
Gecko: 8d663ed59536

Working Gecko with Broken Gaia issue DOES reproduce:
Gaia: ae7527a6e6892b92b67925b3c892e03c6d920ff3
Gecko: f7f06b74b198


B2G-Inbound pushlog:
https://github.com/mozilla-b2g/gaia/compare/29d3a84bbe41d4a3d0496660ab6e97d2352cc8c2...ae7527a6e6892b92b67925b3c892e03c6d920ff3


Issue appears to occur due to changes made in bug 1154025

Comment 17

[KTucker [:KTucker][Inactive 3/4/2016]]

David, can you take a look at this please? Looks like the work done for bug 1154025 might be the culprit here.

Comment 18

[Sue]

(In reply to Josh Cheng [:josh] from comment #4)
> Sue,
> Please open another bug per comment 1 from Delphine.

Sorry for the delay, I have opened a new bug about the ellipsis locating at wrong side in notification bar, see Bug 1159092

Comment 19

[David Flanagan [:djf]]

(In reply to KTucker [:KTucker] from comment #17)
> David, can you take a look at this please? Looks like the work done for bug
> 1154025 might be the culprit here.

Yeah. The music app was using a severely out-of-date version of the gaia-header component. I'm not sure why this bug didn't manifest in that. We updated to the current version and the bug came back. Backing out bug 1154025 is not an option, so we just need to fix this here.

Comment 20

[David Flanagan [:djf]]

(In reply to Sue from comment #18)
> Sorry for the delay, I have opened a new bug about the ellipsis locating at
> wrong side in notification bar, see Bug 1159092

I've taken that bug.

Comment 21

[Hubert Figuiere [:hub]]

It seems that the gaia-header set "direction: rtl" for "gaia-header h1". In an "inline.css". This is definitely not right in that case. Not sure why, and I can't find where.

Comment 22

[Hubert Figuiere [:hub]]

It is in build_stage/music/shared/elements/gaia-header/dist/gaia-header.js:1406.

Comment 23

[David Flanagan [:djf]]

In case they are helpful, I've created some test files with bidi metadata. See https://bugzilla.mozilla.org/show_bug.cgi?id=1159092#c4

Comment 24

[Hubert Figuiere [:hub]]

This is the gaia-header commit that introduced it:

https://github.com/gaia-components/gaia-header/commit/f1fcc81bc3957e0671e045924aa76d2efbfbfbc1

Comment 25

[Hubert Figuiere [:hub]]

Wilson, any idea about what's above? Looks like the rule at https://github.com/gaia-components/gaia-header/commit/f1fcc81bc3957e0671e045924aa76d2efbfbfbc1#diff-c724cebc352c5f85a50270d4af6309d0R1244 is breaking music because it forces RTL if the locale is RTL.

Comment 26

[David Flanagan [:djf]]

Hub: even if there are bugs to be fixed in gaia-header, I don't think it would be safe to uplift them to 2.2.

I think the approach in comment #14 (putting the text-overflow:ellipsis and other properties) on the bdi element works to solve this.  (Though I didn't check to see if that messes up the font sizing code).

Since today is the FC deadline, so it would be great if we could land a simple fix and mark this resolved!

Comment 27

[Wilson Page [:wilsonpage]]

(In reply to Hubert Figuiere [:hub] from comment #25)
> Wilson, any idea about what's above? Looks like the rule at
> https://github.com/gaia-components/gaia-header/commit/
> f1fcc81bc3957e0671e045924aa76d2efbfbfbc1#diff-
> c724cebc352c5f85a50270d4af6309d0R1244 is breaking music because it forces
> RTL if the locale is RTL.

You can override any default styling in your app:

gaia-header h1 {
  direction: ltr !important;
}

Comment 28

[Wilson Page [:wilsonpage]]

You can also nest other elements inside your <h1> to give you more control:

<gaia-header>
   <h1>foo <span>bar</span></h1>
</gaia-header>

gaia-header h1 span {
  direction: ltr;
}

Comment 29

[Hubert Figuiere [:hub]]

Wilson: Thank! it is the !important I was missing to override direction back to "inherit".

David: yes fixing gaia-component was worrying me. Overriding will do. I tried comment #14 and it was not working.

Comment 30

[Hubert Figuiere [:hub]]

Actually the solution in comment #14 do work. No need to override the gaia-header styles.

Comment 31

[GitHub Autolander Bot]

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

Comment 32

[David Flanagan [:djf]]

Comment on attachment 8599397 [details] [review]
[gaia] hfiguiere:bug1158043-bidi > mozilla-b2g:master

Looks good!

I tested this with the samples mentioned in comment #10 and comment #23. The ellipsis comes out on the right of english-language song titles in both LTR and RTL locales. And the ellipsis comes out on the left of fake arabic-language song titles in both locales. Parentheses still work. And gaia-header does still reduce the font size before resorting to an ellipsis

Please file a followup bug for the related (but probably not blocking) issue where we see "...is Stevenson" in the Treasure Island tile when in an RTL locale. I think we should see "Robert Lewis S..." instead

Comment 33

[David Flanagan [:djf]]

Comment on attachment 8599397 [details] [review]
[gaia] hfiguiere:bug1158043-bidi > mozilla-b2g:master

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): not a regression. Just a trick RTL ellipsis issue that we didn't anticipate soon enough.

[User impact] if declined: If a song has a long LTR title, the ellipsis will be prominently displayed on the wrong side in the music player for RTL locales.

[Testing completed]: yes

[Risk to taking this patch] (and alternatives if risky): not risky

[String changes made]: none

Comment 34

[Hubert Figuiere [:hub]]

(In reply to David Flanagan [:djf] from comment #32)
> Comment on attachment 8599397 [details] [review]

> Please file a followup bug for the related (but probably not blocking) issue
> where we see "...is Stevenson" in the Treasure Island tile when in an RTL
> locale. I think we should see "Robert Lewis S..." instead


Didn't notice this one. Will do.

Comment 35

[GitHub Autolander Bot]

https://github.com/mozilla-b2g/gaia/pull/29798

Autolander could not land the pull request due to not having collaborator rights. This is possibly due to a tree closure. Please check the tree status and request checkin again once the tree is open.

Comment 36

[GitHub Autolander Bot]

http://docs.taskcluster.net/tools/task-graph-inspector/#4hen9ro_Sv-1becrrW3jcg

The pull request failed to pass integration tests. It could not be landed, please try again.

Comment 37

[GitHub Autolander Bot]

http://docs.taskcluster.net/tools/task-graph-inspector/#jUcWw412QcWdAGLNT16RSA

The pull request failed to pass integration tests. It could not be landed, please try again.

Comment 38

[Hubert Figuiere [:hub]]

Infra failure. Summoning the autolander one more time.

Comment 39

[GitHub Autolander Bot]

http://docs.taskcluster.net/tools/task-graph-inspector/#7Qsvg5RHTLmdPCKHdUndTQ

The pull request failed to pass integration tests. It could not be landed, please try again.

Comment 40

[Dominic Kuo [:dkuo]]

Hub is working on this so cancelling the ni.

Comment 41

[GitHub Autolander Bot]

http://docs.taskcluster.net/tools/task-graph-inspector/#JEt9U84ZTpmODsyBLj4IYw

The pull request failed to pass integration tests. It could not be landed, please try again.

Comment 42

[Hubert Figuiere [:hub]]

Rebasing just to be sure.

Comment 43

[Hubert Figuiere [:hub]]

Rebased and all green on Gaia-try. Summoning autolander one more time.

Comment 44

[GitHub Autolander Bot]

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

Comment 45

[Ryan VanderMeulen [:RyanVM]]

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

Comment 46

[Brogan Zumwalt [Inactive]]

Issue verified fixed on Flame 2.2 and 3.0

When language set to Arabic a long music file name has ellipsis on right in header of music app.

Device: Flame 2.2
Build ID: 20150501002503
Gaia: 209bf4d6fcb16ea6834b8bd86976c012e5914fe6
Gecko: 79e7065ceefa
Gonk: a9f3f8fb8b0844724de32426b7bcc4e6dc4fa2ed
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
Build ID: 20150501010203
Gaia: 759a1f935a6a81c32ad66e39a6353b334dfa4f91
Gecko: 7723b15ea695
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 47

[Sue]

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