964034 - (gaia-rtl) Support RTL for Music App

Status: RESOLVED WONTFIX

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

Description

[Ahmed Nefzaoui [:Nefzaoui]]

This bug is set for tracking and fixing everything related to Right-To-Left issues in Music App.

Comment 1

[Ahmed Nefzaoui [:Nefzaoui]]

Attachment: dir pseudo class applied on Music Player View

This bit of combination exciting me, however I'm seeking advice concerning UX ?
Since share button is set to be on the left in both cases showcased in attachment, it takes some of the original space of Artist & Album names when they are non-RTL.

Comment 2

[Ahmed Nefzaoui [:Nefzaoui]]

Attachment: PR to Github

Kaze, may I get your feedback about this decision please? :)

Comment 3

[Ahmed Nefzaoui [:Nefzaoui]]

Attachment: Screenshot - Tiles View

Notice the two main tiles

Comment 4

[Fabien Cazenave [:kaze]]

Comment on attachment 8371091 [details] [review]
PR to Github

LGTM, let’s get David’s review on this patch.

FTR, I wonder if we should mirror the rew/play/fwd buttons, the “Now playing” one and the progressbar. Please open another bug for that and we’ll ask for UX help.

Comment 5

[David Flanagan [:djf]]

Comment on attachment 8371091 [details] [review]
PR to Github

Reassigning the review to Dominic who knows more about Music than I do and probably has a shorter review queue.

Comment 6

[Dominic Kuo [:dkuo]]

Currently I got several reviews so I need to prioritize the them.

Ahmed, if this patch target for 1.4? and I am not familiar with Right-To-Left language so probably it will take me some time to review it. Also cc'ing Jacqueline and Peter to let them know about this, thanks.

Comment 7

[Ahmed Nefzaoui [:Nefzaoui]]

Since this bug is one of many blockers for RTL usage in Firefox OS, there's no targeted version with this patch, just master in general, but in the same time, the sooner it gets merged, the sooner we can move on and integrate RTL-based languages in Firefox OS.
Of course, Please, take your time. But I'd more than glad to get this reviewed the soonest possible :)
Thanks.

Comment 8

[Ahmed Nefzaoui [:Nefzaoui]]

Any progress on this ?
Anything I can help with ? Maybe concerning some clarifying some RTL information ? :)

Comment 9

[Dominic Kuo [:dkuo]]

Hey Ahmed, apologize for keep you waiting, I still got several blockers but I should be able to review this patch tomorrow, I will let you know if I need any help, thanks for your patient!

Comment 10

[Ahmed Nefzaoui [:Nefzaoui]]

Thanks, In the meantime I can provide a MP3 song sample with RTL information in it to help you test what would it be like to Play it or have it listed in a LTR view? Shall I? :)

Comment 11

[Dominic Kuo [:dkuo]]

(In reply to Ahmed Nefzaoui [:Nefzaoui] from comment #10)
> Thanks, In the meantime I can provide a MP3 song sample with RTL information
> in it to help you test what would it be like to Play it or have it listed in
> a LTR view? Shall I? :)

Sure, that will be great!

Comment 12

[Dominic Kuo [:dkuo]]

Attachment: Music_RTL_issues.png

The screenshots after applied attachment 8371091 [details] [review]

Comment 13

[Dominic Kuo [:dkuo]]

Comment on attachment 8371091 [details] [review]
PR to Github

Ahmed,

First of all, I applied your patch then tested it before I actually look into the modified code, and noticed issues are easily found which means we should address them and check the patch again before a detail review. I have found 6 of them(maybe more) and probably some of them need discussion with ux/visual that they are issues or not. Please reference to attachment 8391132 [details]:

1. For the first big tile, since you are mirroring almost all the ui, to be sync with the LTR ui, we should also mirror the first big tile, that means, the first big tile should start from the right side.
2. When the artist, album or title are longer than the list area, the ellipsis shouldn't on the left side if it's LTR text.
3. The similar issue in 2., the ellipsis should on the right side on both the album/artist and song title if it's LTR text.
4. The similar issue in 2., the ellipsis should on the right side when it's not RTL text.
5. The album and artist text should be aligned to right for both RTL and LTR text.
6. If we are mirroring all the ui, then we should also mirror the page transitions. In RTL ui, when enter the next page, do LTR transition. When back to previous page, do RTL transition.

I think it's better to discuss with Jacqueline(ux) and Peter(visual) before you make further changes, and probably come out some simple wireframes so that we can fix this and I can review base on the results, and if you need helps on the code, feel free to ping me, thanks.

Comment 14

[Ahmed Nefzaoui [:Nefzaoui]]

So few explanations :)

Picture 1:
That's how exactly it should be, RTL stands for Right To Left, thus, everything written in an RTL-based language should be on the Right side. (Please see Setting App, which has full RTL support right now)
And it's already on the right in the picture. Where's the problem?

Picture 2 and Picture 3:
OK

Picture 4:
That was the (logical) approach I chose to work on in Headers (/shared/headers.css bug 883673), which were accepted by :Kaze back then and this is how competitors does (did) too.. And for Music App there's bug 868549 which tend to see how to get over such cases.. Typically now our problem in this bug.

Picture 5:
Attempted to make the Music App look smarter so when There's a non-RTL (e.g. English/French Song) song being played, it doesn't align it to the right, but to the left instead. As exactly you requested for Picture 2 and 3

For transitions: OK I'll see what I can do, but to be honest I didn't notice any problem with having transitions that way

Finally let me refer to this wiki page: https://wiki.mozilla.org/L10n:B2G/RTL

Further details please?
Thanks so much

Comment 15

[Ahmed Nefzaoui [:Nefzaoui]]

I just updated the PR with
- Fixed transition as requested
- RTL icons for Album view, and "Currently Playing" icon
Please take a look at it now and tell me if that's how transitions are supposed to work, before I proceed to fixing what is in Picture 2 and 3.
Also I'd like to know what do you think about my comments concerning Screenshot 1,4 and 5 in your attachment.

Thanks so much

Comment 16

[Ahmed Nefzaoui [:Nefzaoui]]

Sorry for spamming but I just forgot to mention that icons I just added are indeed compressed with png_recompress.sh

Comment 17

[Dominic Kuo [:dkuo]]

(In reply to Ahmed Nefzaoui [:Nefzaoui] from comment #14)
> So few explanations :)

Thanks for making these changes :)

> Picture 1:
> That's how exactly it should be, RTL stands for Right To Left, thus,
> everything written in an RTL-based language should be on the Right side.
> (Please see Setting App, which has full RTL support right now)
> And it's already on the right in the picture. Where's the problem?

Actually I was talking about the layout of the tiles, the "first" big tile was designed to be place on the left side when it's LTR language. When it's RTL, the first big tile should be started from the right side, that will sync to all the other ui elements.

> Picture 2 and Picture 3:
> OK
> 
> Picture 4:
> That was the (logical) approach I chose to work on in Headers
> (/shared/headers.css bug 883673), which were accepted by :Kaze back then and
> this is how competitors does (did) too.. And for Music App there's bug
> 868549 which tend to see how to get over such cases.. Typically now our
> problem in this bug.

If you have already addressed the header for RTL in /shared/headers.css, then I am fine with that change because all the apps use the shared css should have the same rules. But I don't think we should depend on bug 868549 to fix this issue, cause if some text is truncated imply it should use marquee, then do we need to also use marquee on all the truncated text? for music the answer is probably no because it's easy to get truncated text, like the screenshot in attachment 8371091 [details] [review].

> Picture 5:
> Attempted to make the Music App look smarter so when There's a non-RTL (e.g.
> English/French Song) song being played, it doesn't align it to the right,
> but to the left instead. As exactly you requested for Picture 2 and 3

Okay, this looks good to me as you mentioned above.

> For transitions: OK I'll see what I can do, but to be honest I didn't notice
> any problem with having transitions that way

Every time when the pages do left-to-right transition, it means music app enters the next level, that's, the transition actually indicates the users that they are going to the next page. And base on this point, when it's RTL ui, the page transition should be opposite to indicate the same thing. I believe for the other apps, like settings or homescreen, we should also make similar changes or the ux won't be sync among all the apps.

> Finally let me refer to this wiki page: https://wiki.mozilla.org/L10n:B2G/RTL
> 
> Further details please?
> Thanks so much

I just comment these from my point of view, and I am sure ux and visual folks will have different opinions, let's needinfo Jacqueline and Peter to look on this first.

Jacqueline and Peter, would you please check attachment 8371091 [details] [review] and see if Ahmed and me are doing right on the RTL ui for music? thanks.

Comment 18

[Dominic Kuo [:dkuo]]

(In reply to Dominic Kuo [:dkuo] from comment #17)
> Jacqueline and Peter, would you please check attachment 8371091 [details] [review]
> and see if Ahmed and me are doing right on the RTL ui for music? thanks.

Actually I mean attachment 8391132 [details], it would be better for ux and visual to look on the screenshots instead of the patch, sorry about that :P

Comment 19

[Ahmed Nefzaoui [:Nefzaoui]]

Haven't done a lot concerning this bug lately, but it turned out that the comment on Picture 4 from attachment 8391132 [details] can be actually addressed too, Bug 993089. Silly me.
Also took a slight look and tried to hack on layouts as you suggested regarding Picture 1 in Comment 17.. Seems like it needs more event listeners which means lower performance and slower App loading.. Still haven't deeply looked into it though, what do you think? :)

Comment 20

[Dominic Kuo [:dkuo]]

(In reply to Ahmed Nefzaoui [:Nefzaoui] from comment #19)
> Haven't done a lot concerning this bug lately, but it turned out that the
> comment on Picture 4 from attachment 8391132 [details] can be actually
> addressed too, Bug 993089. Silly me.
> Also took a slight look and tried to hack on layouts as you suggested
> regarding Picture 1 in Comment 17.. Seems like it needs more event listeners
> which means lower performance and slower App loading.. Still haven't deeply
> looked into it though, what do you think? :)

Bug 993089 looks good to me and that should address Picture 4 in attachment 8391132 [details].

For Picture 1 what do you mean by more event listeners?
(Sorry about the inactive response, I am working on 1.3t blockers...)

Comment 22

[Hung Nguyen]

Hi Ahmed/Dominic 

Can you guys confirm what's been asked of UX? Is it the truncation issue and font usage? Or the whole RTL implementation in general? 

Thanks!
Hung

Comment 23

[Stephany Wilkes]

I apologize for the confusion. Hung is not working on RTL UX.

Hung, this was done without any specs. This would just be for a patch review on music fixes.

Today is merge day for 2.1 but this is not a 2.1 blocker, so it will push to 2.2 if it doesn't land and merge.

Comment 24

[Dominic Kuo [:dkuo]]

I think it's nice to have both UX and visual people to review the RTL patch, since the are mostly visually changes so that's why I needinfo Jacqueline and Hung/Peter.

Ahmed, recently the gaia-header component is landed in music app, so I think you will probably need to update your patch due to the conflicts, would you please address it and let's bring this back to the progress? thank you so much!

Comment 26

[Ahmed Nefzaoui [:Nefzaoui]]

Sure, I'm just waiting for my flame device to arrive (with my Peak currently having a almost-dead battery), couple of days and will kick off the work again :)
Sorry for the huge delay..

Comment 27

[Dominic Kuo [:dkuo]]

(In reply to Ahmed Nefzaoui [:Nefzaoui] from comment #26)
> Sure, I'm just waiting for my flame device to arrive (with my Peak currently
> having a almost-dead battery), couple of days and will kick off the work
> again :)
> Sorry for the huge delay..

Ahmed, please take your time cause this is not a blocker(for now) and I am sure we will need reviews(visual and code) back and forth, also thanks again for working on this :)

Comment 28

[Stephany Wilkes]

Exactly. This is not a blocker and will likely take a bit. :) Thanks all!

Comment 29

[Stephany Wilkes]

Resolving as WONTFIX as this will be replaced by a new 2.2 meta bug for RTL in Music, with child bugs attached.

Comment 30

[Stephany Wilkes]

Resolving as WONTFIX as this will be replaced by a new 2.2 meta bug for RTL in Music, with child bugs attached.

Comment 31

[Jacqueline Savory [:jsavory] UX]

Removing flag

Comment 32

[Peter Bylenga [:PBylenga]]

Mass Edit: adding the [rtl-meta]