If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

[Messages][RTL] Correctly handle recipient pills

VERIFIED FIXED in 2.2 S1 (5dec)

Status

Firefox OS
Gaia::SMS
VERIFIED FIXED
3 years ago
2 years ago

People

(Reporter: julienw, Assigned: azasypkin)

Tracking

unspecified
2.2 S1 (5dec)
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [p=1])

Attachments

(6 attachments)

(Reporter)

Description

3 years ago
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.
(Assignee)

Updated

3 years ago
Status: NEW → ASSIGNED
QA Contact: azasypkin
(Assignee)

Updated

3 years ago
Assignee: nobody → azasypkin
QA Contact: azasypkin
(Assignee)

Comment 1

3 years ago
Created attachment 8522138 [details] [review]
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!
Attachment #8522138 - Flags: review?(felash)
(Reporter)

Comment 2

3 years ago
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.
(Reporter)

Updated

3 years ago
Attachment #8522138 - Flags: review?(felash)
(Assignee)

Comment 3

3 years ago
Created attachment 8524559 [details]
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!
Flags: needinfo?(swilkes)

Comment 4

3 years ago
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..
(Assignee)

Comment 7

3 years ago
Created attachment 8526190 [details]
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!
Flags: needinfo?(jelee)
(Assignee)

Updated

3 years ago
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)

Comment 9

3 years ago
Hello Oleg! I don't mind which side as long as all the icons in pill are on the same side. ;)
Flags: needinfo?(jelee)
(Reporter)

Comment 10

3 years ago
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)

Comment 11

3 years ago
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)
(Reporter)

Comment 13

3 years ago
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)
(Reporter)

Comment 14

3 years ago
Created attachment 8526755 [details]
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)
(Reporter)

Comment 15

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

Comment 16

3 years ago
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+
(Assignee)

Updated

3 years ago
See Also: → bug 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)

Comment 18

3 years ago
(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 19

3 years ago
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)
(Reporter)

Comment 23

3 years ago
Yep Oleg, let's do it :)
Flags: needinfo?(felash)

Comment 24

3 years ago
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!
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
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: 2.1 S9 (21Nov) → 2.2 S1 (5dec)
(Assignee)

Updated

3 years ago
See Also: → bug 1105646
Created attachment 8611048 [details]
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
Status: RESOLVED → VERIFIED
QA Whiteboard: [MGSEI-Triage+]
You need to log in before you can comment on or make changes to this bug.