1053708 - Make SMS main User Interface RTL-friendly

Status: RESOLVED FIXED

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

Comment 1

[Ahmed Nefzaoui [:Nefzaoui]]

Attachment: Github pull-request

This part is ready for review. :)

Comment 2

[Julien Wajsberg [:julienw]]

Comment on attachment 8472928 [details] [review]
Github pull-request

I think we should try to use -moz-padding-start and friends as much as possible, instead of overriding all rules like this.

Comment 3

[Julien Wajsberg [:julienw]]

Hey Ahmed, we (as "the SMS team) would like to take this bug in our sprint, is it fine for you?

Comment 4

[Ahmed Nefzaoui [:Nefzaoui]]

Hey Julien, Yeah fine.

Comment 5

[Ahmed Nefzaoui [:Nefzaoui]]

Back on track with a new device. :)

(In reply to Julien Wajsberg [:julienw] from comment #2)
> Comment on attachment 8472928 [details] [review]
> Github pull-request
> 
> I think we should try to use -moz-padding-start and friends as much as
> possible, instead of overriding all rules like this.

So, from the PR:
> how do you feel about using "unset" instead of "auto" here and in all other properties that you reset ?
Yes actually seems to be more effective!! Already started using it more often in other patches :)

> how about using -moz-padding-start and -moz-padding-end instead of rewriting everything like this?
Also don't mind but since every other single CSS file had its properties rewritten manually I think it's just a matter of consistency ? Also if my understand for them is right then they implicitly does the same thing as explicitly overriding things, only a little fancier? :)

Comment 6

[Ahmed Nefzaoui [:Nefzaoui]]

(In reply to Julien Wajsberg [:julienw] from comment #3)
> Hey Ahmed, we (as "the SMS team) would like to take this bug in our sprint,
> is it fine for you?

Just to be sure, this means the SMS team will work on it ?

Comment 7

[Julien Wajsberg [:julienw]]

Yeah, this means we (the SMS team) committed to fixing this bug before Tuesday 30th :)

Comment 8

[Julien Wajsberg [:julienw]]

We'll ask you feedback so feel free to give advice as you have more experience in this :)

Comment 9

[Julien Wajsberg [:julienw]]

(In reply to Ahmed Nefzaoui [:Nefzaoui] from comment #5)
> Back on track with a new device. :)
> 
> (In reply to Julien Wajsberg [:julienw] from comment #2)
> > Comment on attachment 8472928 [details] [review]
> > Github pull-request
> > 
> > I think we should try to use -moz-padding-start and friends as much as
> > possible, instead of overriding all rules like this.
> 
> So, from the PR:
> > how do you feel about using "unset" instead of "auto" here and in all other properties that you reset ?
> Yes actually seems to be more effective!! Already started using it more
> often in other patches :)
> 
> > how about using -moz-padding-start and -moz-padding-end instead of rewriting everything like this?
> Also don't mind but since every other single CSS file had its properties
> rewritten manually I think it's just a matter of consistency ? Also if my
> understand for them is right then they implicitly does the same thing as
> explicitly overriding things, only a little fancier? :)

This is one way to look at it.

The other way to look at it is that if take the habit to use -moz-padding-start and its friends, we'll be RTL-compatible for free. The issue is not about being RTL-compatible now, but to be RTL-compatible in the long run. If we'd override things manually, then as a developer it's difficult to think of it for every patches when you're not using a RTL language frequently.

As a result, RTL support will break for sure.

That's why I want to use -moz-padding-start and such: so that we don't need to think about it: when we'll do CSS changes in the future, we'll change the values in one place only.

Moreover it helps testing a feature that's not so used, and as a result we'll make the web better :)

Comment 10

[Julien Wajsberg [:julienw]]

Attachment: github PR

WIP, built up on top of Ahmed's useful work

Comment 11

[Julien Wajsberg [:julienw]]

Pushed a nearly finished patch, I just need to also change what landed in bug 1067228.

Comment 12

[Julien Wajsberg [:julienw]]

Comment on attachment 8493220 [details] [review]
github PR

Hey Oleg,

I think this is ready for a first review.

Known issues or questions:
* the "(Contact name (+1" title; I'll file a separate bug for this, this may be a gecko issue.
* should the "new message" icon be reversed or done differently? I don't know how we write arabic :) Ahmed, what do you think?
* the incoming/outgoing messages position are not changed in this bug per IRC discussion with Ahmed.

I decided to minimize the amount of RTL-specific overrides by using standard or 
"soon-standard" properties that Gecko support. Notably:
* -moz-(padding/margin/border)-(start/end)
* text-align: (start/end)
* when using both left/right for absolutely positioned elements and there is a conflict (because width is set too), then either left or right is privileged depending on the container's text direction
* converted one float into flex, which supports RTL from the start

Where possible I also simply removed some unused CSS code.

I don't think we need to be perfect here. We obviously need that LTR does not regress, and if we can catch some RTL issues now it's better, but let's not be overstrict :)

Tell me what you think!

Comment 13

[Oleg Zasypkin [:azasypkin]]

(In reply to Julien Wajsberg [:julienw] from comment #12)
> * should the "new message" icon be reversed or done differently? I don't
> know how we write arabic :) Ahmed, what do you think?

Good point! Ni? Ahmed to not miss this :)

Comment 14

[Ahmed Nefzaoui [:Nefzaoui]]

> * should the "new message" icon be reversed or done differently? I don't
> know how we write arabic :) Ahmed, what do you think?

Seems fine as it is, I'm not sure if the icon belongs to the icon font or currently lives as a PNG image, but either way either creating flipped PNG asset or using scaleX(-1) to show that the pen is pointed to the right (as if it was right-to-left writing) would be awesome to have too :)

> * the "(Contact name (+1" title; I'll file a separate bug for this, this may be a gecko issue.
I'm not sure I understand this issue ? Screenshot please? :)

Also, another concern that I have is the fact that in RTL view the "..." are shown behind the message in the main UI and the message is quoted only from it's last part as if it was read from the right.. (adding a screenshot)
Now I'm not sure what property can control that however if that's possible, I think combining it with -moz-dir(rtl) and -moz-dir(ltr) to handle the cases where:

* It's a non-RTL text so we show the first words of the text message from the left and the "..." are on the far right of the <span>
and
* When it's a RTL text (e.g. Arabic) so we show the first words from the right and have the "..." to the far left.

Comment 15

[Ahmed Nefzaoui [:Nefzaoui]]

Attachment: Screenshot for Comment 14

If we can control the position of the "..." depending on the language of the message.. :)

Comment 16

[Julien Wajsberg [:julienw]]

(In reply to Ahmed Nefzaoui [:Nefzaoui] from comment #14)
> > * should the "new message" icon be reversed or done differently? I don't
> > know how we write arabic :) Ahmed, what do you think?
> 
> Seems fine as it is, I'm not sure if the icon belongs to the icon font or
> currently lives as a PNG image, but either way either creating flipped PNG
> asset or using scaleX(-1) to show that the pen is pointed to the right (as
> if it was right-to-left writing) would be awesome to have too :)

The "problem" I see here is that the fact that the pen is pointed to the left is (IMO) more because the writer is right-handed. As a right-handed person, I don't think I'd point the pen to the right to write RTL characters. What do you think?

> 
> > * the "(Contact name (+1" title; I'll file a separate bug for this, this may be a gecko issue.
> I'm not sure I understand this issue ? Screenshot please? :)

Sure, one moment :)

> 
> Also, another concern that I have is the fact that in RTL view the "..." are
> shown behind the message in the main UI and the message is quoted only from
> it's last part as if it was read from the right.. (adding a screenshot)
> Now I'm not sure what property can control that however if that's possible,
> I think combining it with -moz-dir(rtl) and -moz-dir(ltr) to handle the
> cases where:
> 
> * It's a non-RTL text so we show the first words of the text message from
> the left and the "..." are on the far right of the <span>
> and
> * When it's a RTL text (e.g. Arabic) so we show the first words from the
> right and have the "..." to the far left.

It might make sense to do simple testcases in a simple HTML file, it may be gecko issues. Anyway, this is more a matter for bug 1053709 IMO.

Comment 17

[Julien Wajsberg [:julienw]]

Attachment: Issue with group MMS threads

Comment 18

[Ahmed Nefzaoui [:Nefzaoui]]

(In reply to Julien Wajsberg [:julienw] from comment #16)
> (In reply to Ahmed Nefzaoui [:Nefzaoui] from comment #14)
> > > * should the "new message" icon be reversed or done differently? I don't
> > > know how we write arabic :) Ahmed, what do you think?
> > 
> > Seems fine as it is, I'm not sure if the icon belongs to the icon font or
> > currently lives as a PNG image, but either way either creating flipped PNG
> > asset or using scaleX(-1) to show that the pen is pointed to the right (as
> > if it was right-to-left writing) would be awesome to have too :)
> 
> The "problem" I see here is that the fact that the pen is pointed to the
> left is (IMO) more because the writer is right-handed. As a right-handed
> person, I don't think I'd point the pen to the right to write RTL
> characters. What do you think?

I just happen to be left-handed ..
Well.. Haven't thought of it that way TBH. :) But yes, makes sense!!

> > > * the "(Contact name (+1" title; I'll file a separate bug for this, this may be a gecko issue.
> > I'm not sure I understand this issue ? Screenshot please? :)
>Sure, one moment :)

Thanks :)

Comment 19

[Oleg Zasypkin [:azasypkin]]

Comment on attachment 8493220 [details] [review]
github PR

I've left some questions and few nits on Github, but otherwise SMS part looks good to me! IMO it's better to land this base RTL patch sooner and iterate through follow-ups if we have other issues.

Comment 20

[Julien Wajsberg [:julienw]]

Attachment: Comparison before/after change

Hey Fang,

In this patch I adjust how the DSDS indicators look like. I think this is closer to the spec in attachment 8475084 [details], but please tell me.

On this image, you have a screenshot for the current master, a screenshot for the patch, and a screenshot for the patch in Right-to-Left mode. Please tell me if it looks fine for you !

You can directly test the patch from the github pull request, it's uptodate.

Comment 21

[Julien Wajsberg [:julienw]]

Comment on attachment 8493220 [details] [review]
github PR

Asking for a review from Arnau for the changes in BB.

Comment 22

[Fang Shih [:fang]]

Comment on attachment 8501738 [details]
Comparison before/after change

It looks good!! Thanks Julien!

Comment 23

[Arnau March [:arnau] ( not working in Firefox OS anymore :( )]

Comment on attachment 8493220 [details] [review]
github PR

Overall, the changed properties in BB looks good to me.
But RTL stuff is quite confusing to me, not really sure how things should be positioned, or icons rotated... I guess you have a better idea how the layout should fit :)

Comment 24

[Julien Wajsberg [:julienw]]

master: f849fc9505b8466bc0ab4aca58891e0b268c21be

Comment 25

[Ahmed Nefzaoui [:Nefzaoui]]

\o/