1096330 - [Messages][RTL] Correctly handle recipient pills

Status: VERIFIED FIXED

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

Description

[Julien Wajsberg [:julienw]]

International phone numbers inside recipient pills are not correctly displayed. This bug is to make sure we handle correctly this case, along with possibly other cases.

Since recipient pills are part of building blocks, we should make sure it still works fine in other apps too.

Comment 1

[Oleg Zasypkin [:azasypkin]]

Attachment: GitHub pull request URL

Hey Julien,

In this patch I've chosen less risky approach I had in mind, but described at GitHub the other option I was thinking about.

Thanks!

Comment 2

[Julien Wajsberg [:julienw]]

Comment on attachment 8522138 [details] [review]
GitHub pull request URL

I like the less risky approach but the selectors with "moz-dir" need some more comment.

We also need to wait for Eitan's answer in bug 1080832 for flex-grow.

And lastly I think we can try to simplify some more the selectors.

Comment 3

[Oleg Zasypkin [:azasypkin]]

Attachment: email_and_wrong_recipients.png

Hey Stephany,

Could you please advise if we need to change location of "!" sign for invalid recipient and "email" icon for email recipient when we're in RTL (see screenshot of what we have in LTR currently)?

Thanks!

Comment 4

[Stephany Wilkes]

I'm flagging Ahmed to advise further. I would think that the email icon (assuming you mean the envelope near the recipient email address field at top) would be at right, but defer to Ahmed on what he would expect to see here. Thanks!

Comment 5

[Ahmed Nefzaoui [:Nefzaoui]]

Yes, for RTL, the "!" sign should be at the far right in the pill.
Thanks!

Comment 6

[Ahmed Nefzaoui [:Nefzaoui]]

Though, I'm still not sure why are we using :-moz-dir() for this particular case?
I tend to use it only when I'm trying to handle mixed content (RTL & LTR) in a given direction since it's result (and correct me if I'm wrong) gets affected by the content of the element itself and not only the document dir attribute..

Comment 7

[Oleg Zasypkin [:azasypkin]]

Attachment: email_and_wrong_recipients_in_LTR.png

(In reply to Ahmed Nefzaoui [:Nefzaoui] (Please needinfo? | Away from 30 OCT to 18 NOV) from comment #6)
> Though, I'm still not sure why are we using :-moz-dir() for this particular
> case?
> I tend to use it only when I'm trying to handle mixed content (RTL & LTR) in
> a given direction since it's result (and correct me if I'm wrong) gets
> affected by the content of the element itself and not only the document dir
> attribute..

So, let me try to explain: 

We have "container" that contains recipient pills, every pill element has "dir=auto" because it can contain either LTR/RTL contact name or LTR international number (that's basically the reason for dir="auto"). Previously margins between pills were based on "-moz-margin-end" of the pill element. With "dir=auto" there is possible case when we'll have two sibling RTL and LTR pills and their "-moz-margin-end" will be on the opposite sides, thus we won't have any margin between these two pills :) So relying on container's direction we can define "fixed" margin that doesn't depend on text direction inside pill.

Maybe I'm missing something and there is a better way to handle this?

> Yes, for RTL, the "!" sign should be at the far right in the pill. Thanks!

Thanks! And what about "email" icon location? In LTR it's located on the left side, though I bit confused that location of "!" and "email" is different, let's ask Jenny :)

Jenny, is it intentional that location of "!" and "email" icons is different (see email_and_wrong_recipients_in_LTR.png)?

Thanks!

Comment 8

[Ahmed Nefzaoui [:Nefzaoui]]

Concerning the email icon, if in LTR it's located on the left side, then it's just usual mirroring: shifting it to the right for RTL :)

Comment 9

[Jenny Lee]

Hello Oleg! I don't mind which side as long as all the icons in pill are on the same side. ;)

Comment 10

[Julien Wajsberg [:julienw]]

Jenny, Fang, so you'd put both error and email icons on the right, or on the left in LTR?

Comment 11

[Fang Shih [:fang]]

Hi Julien, After we checked on RTL spec, We prefer put both error and email icons on the right in RTL. Thanks!

Comment 12

[Oleg Zasypkin [:azasypkin]]

Comment on attachment 8522138 [details] [review]
GitHub pull request URL

Hey Julien,

Since we've got all needed replies, can you please look at the updated PR again?

If you're ok, I think I'll need BB peer review too.

Thanks!

Comment 13

[Julien Wajsberg [:julienw]]

Fang, sorry, just to be as clear as possible.

LTR: both icons on the left
RTL: both icons on the right

is that it?

Comment 14

[Julien Wajsberg [:julienw]]

Attachment: 2014-11-21-15-32-41.png

Hey Fang,

I think the email icon should have less padding on the left (and right in RTL mode). What do you think?

Comment 15

[Julien Wajsberg [:julienw]]

Note: here is the spec we used for the email icon: attachment 8399982 [details] (in bug 840515).

Comment 16

[Julien Wajsberg [:julienw]]

Comment on attachment 8522138 [details] [review]
GitHub pull request URL

r=me with the added comments and one nit

Yep we need a review from our lovely BB peer :arnau :) Please ask him when you're ready!

Comment 17

[Oleg Zasypkin [:azasypkin]]

Comment on attachment 8522138 [details] [review]
GitHub pull request URL

(In reply to Julien Wajsberg [:julienw] from comment #16)
> Comment on attachment 8522138 [details] [review]
> GitHub pull request URL
> 
> r=me with the added comments and one nit
> 
> Yep we need a review from our lovely BB peer :arnau :) Please ask him when
> you're ready!

Thanks for review and let's wait for ui-review from Fang :) 

Hey Arnau, would you mind to review BB part in the meantime?

Thanks!

Comment 18

[Fang Shih [:fang]]

(In reply to Julien Wajsberg [:julienw] from comment #13)
> Fang, sorry, just to be as clear as possible.
> 
> LTR: both icons on the left
> RTL: both icons on the right
> 
> is that it?

Yes, That's correct! Thanks!

Comment 19

[Fang Shih [:fang]]

Comment on attachment 8526755 [details]
2014-11-21-15-32-41.png

Hey Julien,

According the visual space, I think the error icon is the one should have more padding on the left, and right in RTL mode. So it'll have the same padding for both left/ right side of this pill button. 

I was wondering can we also make the email icon have more padding on the right. Currently it looks a lot closer to the text. It should have the same padding like Error icon with text. 

Thanks!

Comment 20

[Oleg Zasypkin [:azasypkin]]

Hey Fang,

Thanks for the reply! Some notes/question below:

> According the visual space, I think the error icon is the one should have
> more padding on the left, and right in RTL mode.

Just to confirm :) That's true for both RTL and LTR mode, right?

> So it'll have the same padding for both left/ right side of this pill button. 

Please note, paddings for "error" icon are defined in BB and can be shared across other apps (though I don't see that anybody except Messages using it right now).

> I was wondering can we also make the email icon have more padding on the
> right. Currently it looks a lot closer to the text. It should have the same
> padding like Error icon with text. 

It would be nice if you share padding values that you have in mind :)

Thanks!

Comment 21

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

Comment on attachment 8522138 [details] [review]
GitHub pull request URL

LGTM

Comment 22

[Oleg Zasypkin [:azasypkin]]

Julien,

Maybe we can land this patch as is since MMS via email isn't enabled yet and file follow-up to unify look&feel of "email" and "error" icons if needed?

Thanks!

Comment 23

[Julien Wajsberg [:julienw]]

Yep Oleg, let's do it :)

Comment 24

[Fang Shih [:fang]]

Attachment: Messages_1096330.jpg

Hey Oleg,
I've attached the padding spec, Thanks for the help! Let me know if you need any other details. Thanks!

Comment 25

[Oleg Zasypkin [:azasypkin]]

(In reply to Julien Wajsberg [:julienw] from comment #23)
> Yep Oleg, let's do it :)

Okay!

(In reply to Fang Shih [:grasspizza] from comment #24)
> Created attachment 8529466 [details]
> Messages_1096330.jpg
> 
> Hey Oleg,
> I've attached the padding spec, Thanks for the help! Let me know if you need
> any other details. Thanks!

Thanks for spec Fang! I've filed separate bug 1105646 with the reference to this spec.

Treeherder is green, so landed.

Master: https://github.com/mozilla-b2g/gaia/commit/05d9637fb89c4d2d36f467559aa1959450e0ffad

Comment 26

[Lancy(Leave from Mozilla)]

Attachment: Verify1_Flame v2.2&3.0_N5 v2.2&3.0_Pass.png

This issue is verified pass on latest flame v2.2&3.0 and N5 v2.2&3.0 build.
The STR is as follow:
1.Set your phone language to Arabic.
2.Launch Message and tap the new message button.
3.Input a invalid recipient and a email address in recipient field.
**The error and email icons in pills are on the same side.

See attachment:Verify1_Flame v2.2&3.0_N5 v2.2&3.0_Pass.png
Reproducing rate:0/10

Device: Flame 2.2 build (Pass)
Build ID               20150526162504
Gaia Revision          15fa6e486b5c82956a8e8f8a99c39d297e8f0523
Gaia Date              2015-05-26 20:07:31
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/663bf4703588
Gecko Version          37.0
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150526.201446
Firmware Date          Tue May 26 20:14:55 EDT 2015
Bootloader             L1TC000118D0

Device: Flame 3.0 build (Pass)
Build ID               20150526160204
Gaia Revision          8ca93673869a64e09ed6153c5402896822dfb253
Gaia Date              2015-05-26 19:31:37
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/1e4e369822ac
Gecko Version          41.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150526.195035
Firmware Date          Tue May 26 19:50:45 EDT 2015
Bootloader             L1TC000118D0

Device: N5 2.2 build (Pass)
Build ID               20150526162504
Gaia Revision          15fa6e486b5c82956a8e8f8a99c39d297e8f0523
Gaia Date              2015-05-26 20:07:31
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/663bf4703588
Gecko Version          37.0
Device Name            hammerhead
Firmware(Release)      5.1
Firmware(Incremental)  eng.cltbld.20150526.201116
Firmware Date          Tue May 26 20:11:30 EDT 2015
Bootloader             HHZ12f

Device: N5 3.0 build (Pass)
Build ID               20150526160204
Gaia Revision          8ca93673869a64e09ed6153c5402896822dfb253
Gaia Date              2015-05-26 19:31:37
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/1e4e369822ac
Gecko Version          41.0a1
Device Name            hammerhead
Firmware(Release)      5.1
Firmware(Incremental)  eng.cltbld.20150526.195039
Firmware Date          Tue May 26 19:50:56 EDT 2015
Bootloader             HHZ12f