Closed Bug 1096330 Opened 5 years ago Closed 5 years ago

[Messages][RTL] Correctly handle recipient pills

Categories

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

defect
Not set

Tracking

(Not tracked)

VERIFIED FIXED
2.2 S1 (5dec)

People

(Reporter: julienw, Assigned: azasypkin)

References

Details

(Whiteboard: [p=1])

Attachments

(6 files)

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.
Status: NEW → ASSIGNED
QA Contact: azasypkin
Assignee: nobody → azasypkin
QA Contact: azasypkin
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!
Attachment #8522138 - Flags: review?(felash)
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.
Attachment #8522138 - Flags: review?(felash)
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!
Flags: needinfo?(swilkes)
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!
Flags: needinfo?(swilkes) → needinfo?(nefzaoui.ahmed)
Yes, for RTL, the "!" sign should be at the far right in the pill.
Thanks!
Flags: needinfo?(nefzaoui.ahmed)
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..
(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!
Flags: needinfo?(jelee)
Flags: needinfo?(nefzaoui.ahmed)
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 :)
Flags: needinfo?(nefzaoui.ahmed)
Hello Oleg! I don't mind which side as long as all the icons in pill are on the same side. ;)
Flags: needinfo?(jelee)
Jenny, Fang, so you'd put both error and email icons on the right, or on the left in LTR?
Flags: needinfo?(jelee)
Flags: needinfo?(fshih)
Hi Julien, After we checked on RTL spec, We prefer put both error and email icons on the right in RTL. Thanks!
Flags: needinfo?(jelee)
Flags: needinfo?(fshih)
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!
Attachment #8522138 - Flags: review?(felash)
Fang, sorry, just to be as clear as possible.

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

is that it?
Flags: needinfo?(fshih)
Attached image 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?
Attachment #8526755 - Flags: ui-review?(fshih)
Note: here is the spec we used for the email icon: attachment 8399982 [details] (in bug 840515).
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!
Attachment #8522138 - Flags: review?(felash) → review+
See Also: → 1103011
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!
Attachment #8522138 - Flags: review?(rnowmrch)
(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!
Flags: needinfo?(fshih)
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!
Attachment #8526755 - Flags: ui-review?(fshih) → ui-review-
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!
Flags: needinfo?(fshih)
Comment on attachment 8522138 [details] [review]
GitHub pull request URL

LGTM
Attachment #8522138 - Flags: review?(rnowmrch) → review+
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!
Flags: needinfo?(felash)
Yep Oleg, let's do it :)
Flags: needinfo?(felash)
Attached image 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!
Flags: needinfo?(fshih)
(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
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: 2.1 S9 (21Nov) → 2.2 S1 (5dec)
See Also: → 1105646
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
Status: RESOLVED → VERIFIED
QA Whiteboard: [MGSEI-Triage+]
You need to log in before you can comment on or make changes to this bug.