1217779 - [RTL][Music]The parenthesis in search result list are displayed incorrectly.

Status: VERIFIED FIXED

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

Description

[Lancy(Leave from Mozilla)]

Attachment: Music_parenthesis_v2.5.png

[1.Description]:
[RTL][AriesKK&FlameKK v2.5][Music]Search the song whose name contains parenthesis, the parenthesis in search result list are displayed incorrectly.
See attachments:Music_parenthesis_v2.5.png. 

[2.Testing Steps]: 
1. Have your phone set in a RTL language(Arabic) 
2. Go to the Music app with about 5~10 songs in the phone 
3. Go to the titles, Playlist, Artists, Albums or Songs view and tap the search field.
4. Input the keywords and the song with parenthesis is listed.

[3.Expected Result]: 
4.The texts should be shown in parenthesis.

[4.Actual Result]: 
4.The parenthesis displayed incorrectly.

[5.Reproduction build]: 
Device: Flame KK 2.5 build (Affected)
Build ID               20151022150207
Gaia Revision          29ce8ec8606e59f582375234440812b046346513
Gaia Date              2015-10-22 05:31:38
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/76bd0c01d72e64ca4f261ffdb2652a91f961e930
Gecko Version          44.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20151022.185000
Firmware Date          Thu Oct 22 18:50:13 EDT 2015
Firmware Version       v18D v4
Bootloader             L1TC000118D0

Device: Aries KK 2.5 build (Affected)
Build ID               20151023005002
Gaia Revision          29ce8ec8606e59f582375234440812b046346513
Gaia Date              2015-10-22 05:31:38
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/1f03a14106e59280761ac53904340f389674337f
Gecko Version          44.0a1
Device Name            aries
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.worker.20151023.001128
Firmware Date          Fri Oct 23 00:11:35 UTC 2015
Bootloader             s1

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

[7.TCID]: 
15337

Comment 1

[Augustin Trancart [:autra]]

This is a Music issue.

Comment 2

[Josh Cheng [:josh]]

[Tracking Requested - why for this release]:
Not in scope of 2.5

Comment 3

[Julien Wajsberg [:julienw]]

jim, I doubt this depends on that bug ? What I see is "simply" a missing <bdi>...

bug 1157727 would also fix this but I don't expect it in 2.5.

Comment 4

[Jim Porter (:squib)]

We could try to use <bdi>, but dir="auto" would work better (i.e. fix some of our other problems as well), and the easiest way to get that working properly in web components is to fix Gecko. It's possible I could work around the lack of proper CSS :dir() support by extending some of the web component hacks we have, but I'd rather just fix it right the first time.

Comment 5

[Julien Wajsberg [:julienw]]

<bdi> and dir=auto are essentially the same nowadays. (except you can put dir=auto on existing elements).

I really don't understand how :dir() would help here but maybe I miss something :)

Comment 6

[Jim Porter (:squib)]

It would help me to fix the padding on the text that depends on the directionality of the locale, not the actual text.

Comment 7

[Jim Porter (:squib)]

Oops, I hit enter too fast. padding-inline-start/end works great when the directionality of the text matches the locale (because Gecko's CSS engine tracks the direction correctly through the shadow DOM), but when the directionality *doesn't* match, the easiest way is to use :dir() in the selector.

In any case, the dep is because I plan to fix that bug first, and then use the changes there to fix this.

Comment 8

[GitHub Autolander Bot]

Attachment: [gaia] wilsonpage:1217779 > mozilla-b2g:master

Comment 9

[Wilson Page [:wilsonpage]]

Attachment: mixed-arabic-english-songs-view

Comment 10

[Wilson Page [:wilsonpage]]

VIDEO: https://www.youtube.com/watch?v=zLAxk-W_PNw

Comment 11

[Justin D'Arcangelo [:justindarc]]

Comment on attachment 8688464 [details] [review]
[gaia] wilsonpage:1217779 > mozilla-b2g:master

LGTM. Just address my nits in the PR before landing.

Comment 12

[Jim Porter (:squib)]

Thanks for fixing this (especially in a way that doesn't require me fixing :dir() first)! This is pretty much exactly what I was planning on doing. I just have one tiny comment on GitHub.

Comment 13

[Jim Porter (:squib)]

(In reply to Wilson Page [:wilsonpage] from comment #9)
> Created attachment 8688466 [details]
> mixed-arabic-english-songs-view

I just noticed this. Let's get UX to take a look at this, since I'm pretty sure they want all the song titles to be aligned to the same side (the right side in this case).

Comment 14

[Wilson Page [:wilsonpage]]

(In reply to Jim Porter (:squib) from comment #13)
> (In reply to Wilson Page [:wilsonpage] from comment #9)
> > Created attachment 8688466 [details]
> > mixed-arabic-english-songs-view
> 
> I just noticed this. Let's get UX to take a look at this, since I'm pretty
> sure they want all the song titles to be aligned to the same side (the right
> side in this case).

This bug concerns the display of parenthesis, which is now correct. Can we address this as a follow-up?

Comment 15

[Jim Porter (:squib)]

The behavior without this patch follows what I think the UX spec is (text alignment matches the user-locale's directionality), so landing this patch would change that behavior. If my understanding of the spec is correct, this patch would regress that part of the spec, so I think we need to address this before landing.

I could be wrong about the spec, of course, but the above is how I've handled LTR-in-RTL (and vice versa) in every other case.

Comment 16

[Wilson Page [:wilsonpage]]

(In reply to Jim Porter (:squib) from comment #15)
> The behavior without this patch follows what I think the UX spec is (text
> alignment matches the user-locale's directionality), so landing this patch
> would change that behavior. If my understanding of the spec is correct, this
> patch would regress that part of the spec, so I think we need to address
> this before landing.
> 
> I could be wrong about the spec, of course, but the above is how I've
> handled LTR-in-RTL (and vice versa) in every other case.

OK. How did you do it in OGA?

Comment 17

[Jim Porter (:squib)]

Poorly? :) Let me pull your patch and I'll see what I can do about it. I was tinkering with this last week and had something that mostly worked before I got sidetracked by :dir() behaving weirdly.

Comment 18

[Wilson Page [:wilsonpage]]

(In reply to Jim Porter (:squib) from comment #17)
> Poorly? :) Let me pull your patch and I'll see what I can do about it. I was
> tinkering with this last week and had something that mostly worked before I
> got sidetracked by :dir() behaving weirdly.

OK. I just pushed a patch addressing final comments.

Comment 19

[Jim Porter (:squib)]

Attachment: Option 1: the "thorough" way

For this patch and the next, I chose to handle this in gaia-fast-list, since the UX specs for these cases apply everywhere in Gaia, not just the Music app. I know it's a bit more inconvenient to cut another release of gaia-fast-list and import it here, but I think this will benefit other apps as well, so it should be worth it in the long run. If you don't want to update gaia-fast-list yet, then consult the third patch that I'll post shortly.

This is the most-thorough way I can think of to fix the issue. It would work no matter what kind of template a user provided to <gaia-fast-list>, even if dir="auto" were set in a parent of the h3/p element.

Comment 20

[Jim Porter (:squib)]

Attachment: Option 2: a simpler way

This version is a bit simpler and relies on text-align: match-parent to fix the alignment. It's not as flexible as the previous one, but it might be sufficient, depending on what kinds of templates you want to support for gaia-fast-list.

Comment 21

[Jim Porter (:squib)]

Attachment: Option 3: the music-specific way

This essentially applies the CSS rule from option 2 in the music app only. It's the simplest patch possible, but obviously, no other apps will benefit from this change, as it's specific to the music app.

Comment 22

[Sam Foster [:sfoster] (he/him)]

Drive-by comment just to confirm: when document.dir='rtl', we want lists to be aligned to the right margin, with icon then title if reading from right to left - even if they are english/ltr titles. This is addressed in the spec at https://mozilla.app.box.com/s/y60s1ngyza9jcltmpaqwfoye4f311n5r - specifically in the "Lists" section.

Comment 23

[Wilson Page [:wilsonpage]]

Are all these patches based on top of mine?

Ideally I would like to fix the issue in the component but the requirement of `dir="auto"` in the template means that the user will always have to do some work.

I think aesthetically it looks better to right align everything in RTL mode, so perhaps that is a sensible default for the component. Then I guess we don't have to rely on the user adding dir="auto", unless that is still required to fix the parenthesis issue?

Comment 24

[Wilson Page [:wilsonpage]]

(In reply to Sam Foster [:sfoster] from comment #22)
> Drive-by comment just to confirm: when document.dir='rtl', we want lists to
> be aligned to the right margin, with icon then title if reading from right
> to left - even if they are english/ltr titles. This is addressed in the spec
> at https://mozilla.app.box.com/s/y60s1ngyza9jcltmpaqwfoye4f311n5r -
> specifically in the "Lists" section.

Wow this is amazing info, thanks Sam!

Comment 25

[Sam Foster [:sfoster] (he/him)]

(In reply to Sam Foster [:sfoster] from comment #22)
> we want lists to
> be aligned to the right margin, with icon then title if reading from right
> to left 

Doh, sorry to confuse. In the LTR list we read title then icon going from left to right. In RTL, we should read title then icon going from right to left. In otherwords, it should be flipped from whatever it is in LTR.

Comment 26

[Jim Porter (:squib)]

(In reply to Wilson Page [:wilsonpage] from comment #23)
> Are all these patches based on top of mine?

Yes. I just edited gaia-fast-list in the music app too, since it was easier to show the change that way.

> Ideally I would like to fix the issue in the component but the requirement
> of `dir="auto"` in the template means that the user will always have to do
> some work.

There's no way to prevent that, unfortunately. It's up to the user of <gaia-fast-list> to declare the directionality of their content.

> I think aesthetically it looks better to right align everything in RTL mode,
> so perhaps that is a sensible default for the component. Then I guess we
> don't have to rely on the user adding dir="auto", unless that is still
> required to fix the parenthesis issue?

dir="auto" is still required for the parens and also to get the ellipsis to show up on the correct side. dir="auto" basically says "stuff in here might not match the directionality of its parent"; that lets Gecko know to isolate it from the parent and treat the directionality of neutral characters, like parens, according to the text inside the dir="auto" element.

If you have LTR-in-RTL, then without dir="auto", once Gecko sees the closing paren at the end of your text, it thinks you've already stopped being LTR and are back to RTL, so it mirrors the paren and puts it on the wrong side.

It's kind of confusing, so I just think of dir="auto" as a magic incantation that makes arbitrary text flow in the right direction. I asked the l10n experts on dev-fxos, and their advice was basically "use dir="auto" for text coming from the user, and fix the text-alignment as needed".

Comment 27

[Wilson Page [:wilsonpage]]

Comment on attachment 8688464 [details] [review]
[gaia] wilsonpage:1217779 > mozilla-b2g:master

https://github.com/mozilla-b2g/gaia/commit/a49a89add899b2a646422ab15906af023e291045

Comment 28

[Jim Porter (:squib)]

Should this be resolved now?

Comment 29

[Wilson Page [:wilsonpage]]

Comment on attachment 8688464 [details] [review]
[gaia] wilsonpage:1217779 > mozilla-b2g:master

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

Comment 30

[Mahendra Potharaju [:mahe]]

Josh, 

Can you confirm if this effects RTL on TV? Its a paper cut, so want to approve if this is required for TV

Comment 31

[Jim Porter (:squib)]

Huh? The music app doesn't exist on TV.

Comment 32

[Lancy(Leave from Mozilla)]

Attachment: Verify1_parenthesis_master.png

This issue is verified as pass on latest FlameKK&AriesKK master build by the same STR in comment 0.
Actual result:Search the song with parenthesis, the text is shown in parenthesis.
See attachment:Verify1_parenthesis_master.png
Reproducing rate:0/10

Device: Aries KK master build(Pass)
Build ID               20151207142341
Gaia Revision          24ed003a53a81f63367e265fa7117cbe7d23d4c8
Gaia Date              2015-12-07 03:36:55
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/59bc3c7a83de7ffb611203912a7da6ad84535a5a
Gecko Version          45.0a1
Device Name            aries
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.worker.20151207.134432
Firmware Date          Mon Dec  7 13:44:39 UTC 2015
Bootloader             s1

Device: Flame KK master build 512mb(Pass)
Build ID               20151207030216
Gaia Revision          24ed003a53a81f63367e265fa7117cbe7d23d4c8
Gaia Date              2015-12-07 03:36:55
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/528ea05671e9bd9ccb33d1558a20691a72c85f98
Gecko Version          45.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20151207.062956
Firmware Date          Mon Dec  7 06:30:09 EST 2015
Firmware Version       v18D v4
Bootloader             L1TC000118D0

Comment 33

[Josh Cheng [:josh]]

(In reply to Mahendra Potharaju [:mahe] from comment #30)
> Josh, 
> 
> Can you confirm if this effects RTL on TV? Its a paper cut, so want to
> approve if this is required for TV

Hi Mahe,
There is no plan for shipping TV with RTl feature for now. Thanks.

Comment 34

[Mahendra Potharaju [:mahe]]

Comment on attachment 8688464 [details] [review]
[gaia] wilsonpage:1217779 > mozilla-b2g:master

Approved for 2.5

Comment 35

[Carsten Book [:Tomcat]]

hi, this has problems to apply to 2.5 

Tomcats-MacBook-Pro-2:gaia Tomcat$ git cherry-pick 117a39b2c8fe2e5c7b1b06a14762f750fe009dbe
error: could not apply 117a39b... Bug 1217779 - [RTL][Music]The parenthesis in search result list are displayed incorrectly
hint: after resolving the conflicts, mark the corrected paths
hint: with 'git add <paths>' or 'git rm <paths>'
hint: and commit the result with 'git commit'
can you check this , thanks!

Comment 36

[GitHub Autolander Bot]

Attachment: [gaia] wilsonpage:1217779-v2.5 > mozilla-b2g:v2.5

Comment 37

[Wilson Page [:wilsonpage]]

For some reason the patch seemed to apply for me (attachment 8702514 [details] [review]).

Comment 38

[Carsten Book [:Tomcat]]

https://github.com/mozilla-b2g/gaia/commit/c1498a6256c0bab2d4b33d6af47d02ece78dd546

thanks Wilson did the merge/uplift now

Comment 39

[Lance（leave from Mozilla）]

This bug has been verified as "pass" on the latest build of Aries KK v2.5 and Flame KK v2.5 512mb by the STR in comment 0.

Actual result: Search the song with parenthesis, the text is shown in parenthesis.
 
See attachment: Verify2_parenthesis_v2.5.png
Reproduce rate: 0/10

Device: Aries KK v2.5 (Pass)
Build ID               20151231123040
Gaia Revision          37f0119c1c0767962060b25c898c6caaf7f08c04
Gaia Date              2015-12-31 10:21:04
Gecko Revision         http://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/392d484c5d78a50360aa92e96bb5737e8e558889
Gecko Version          44.0
Device Name            aries
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.worker.20151231.113833
Firmware Date          Thu Dec 31 11:38:41 UTC 2015
Bootloader             s1

Device: Flame KK v2.5 512mb(Pass)
Build ID               20151230174512
Gaia Revision          7a016dc992b82e3700ca18382c3b3f57be178c07
Gaia Date              2015-12-30 15:34:29
Gecko Revision         http://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/b96c539dffb4f94ec52a6704805c0444269c1f83
Gecko Version          44.0
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.worker.20151230.165528
Firmware Date          Wed Dec 30 16:55:38 UTC 2015
Firmware Version       v18D v4
Bootloader             L1TC000118D0

Comment 40

[Lance（leave from Mozilla）]

Attachment: Verify2_parenthesis_v2.5.png