944644 - [Messages][Refactoring] Replacing <br> with proper lists while displaying error messages with the recipients case

Status: VERIFIED FIXED

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

Description

[Steve Chung [:steveck]]

It's a follow-up bug from bug 928330. Error dialog has been modularized but the message content for showing recipients could be better instead of layout with <br> elements.

Comment 1

[Julien Wajsberg [:julienw]]

see https://github.com/mozilla-b2g/gaia/blob/70fb49fb147d237ce3de4654daef2976e23fbfba/apps/sms/js/error_dialog.js#L41-L44

I think we'll have a RTL issue here.

Hey kaze, maybe you could check this?

"showRecipient" is true for FDN errors (see [1]) and I know you worked on this some time ago, so maybe you're the right engineer for this? At least you know how to trigger FDN errors :D

[1] https://github.com/mozilla-b2g/gaia/blob/70fb49fb147d237ce3de4654daef2976e23fbfba/apps/sms/js/errors.js

Comment 2

[Dylan Oliver [:doliver]]

I think Kaze is unavailable  -- Oleg, if you understand the RTL issue that Julien is referencing, can you help illustrate it so we can judge the severity?

Comment 3

[Oleg Zasypkin [:azasypkin]]

Attachment: fdn-check-issue.png

(In reply to Dylan Oliver [:doliver] from comment #2)
> I think Kaze is unavailable  -- Oleg, if you understand the RTL issue that
> Julien is referencing, can you help illustrate it so we can judge the
> severity?

Sure, see screenshot of FDN error in LTR and RTL (via pseudo locale). Numbers should always be in LTR, so "+" sign in international number should always be at the beginning. I also see some weird "<br />" in RTL, but it may be related to pseudo-locale, not sure.

Comment 4

[Dylan Oliver [:doliver]]

Thanks Oleg. Yes, I believe the <br/> problem shown here is related to pseudo-locale so the issue is main issue is the + character, but that is likely a different bug that what Steve is reporting here and we should probably follow up with a new bug if we don't have one already. Johan, do you know if this is filed?

Since this one seems to be mostly code cleanup then, marking as P3.

Comment 5

[Johan Lorenzo [:jlorenzo]]

I don't think any bug for this panel has ever been filed. Most of the work regarding the position of the + in a phone number was handled in bug 1080828. We need to create a new bug. QA wanted to file it.

Comment 6

[Julien Wajsberg [:julienw]]

I don't think we need a new bug for this.

The root cause for the "+" badly positioned is this bug, the fix is what this bug is proposing, there is IMO no really different possible fix.

Comment 7

[Johan Lorenzo [:jlorenzo]]

Triage: Already marked as P3 by the RTL owners, not blocking 2.2 then.

Comment 8

[Dylan Oliver [:doliver]]

P3 decision was because I read this more about code cleanup than user facing -- if it's about the position of the + char then we've been marking those P1/P2 depending on visibility. Upgrading this one for P2 and re-nominating for triage group to consider. I don't know how common FDN is in the target markets.

Comment 9

[Johan Lorenzo [:jlorenzo]]

Triage: Agreed on comment 8.

Comment 10

[Julien Wajsberg [:julienw]]

I'll have to change some strings.

Comment 11

[GitHub Autolander Bot]

Attachment: [gaia] julienw:944644-use-lists-for-numbers > mozilla-b2g:master

Comment 12

[Julien Wajsberg [:julienw]]

Comment on attachment 8580697 [details] [review]
[gaia] julienw:944644-use-lists-for-numbers > mozilla-b2g:master

Hey Steve,

tell me what you think :)

Comment 13

[Julien Wajsberg [:julienw]]

Attachment: fdn-error.png

Hey Fang, what do you think of this?

Especially for the list of phone numbers.


(Note this is a 2.2+ bug :) )

Comment 14

[Steve Chung [:steveck]]

Comment on attachment 8580697 [details] [review]
[gaia] julienw:944644-use-lists-for-numbers > mozilla-b2g:master

Overall looks good except a title l10n key change is miising. I also have some minor questions on github, and we need visual's comment about this changes because the new dialog body is quite different than previous verion.

Comment 15

[Julien Wajsberg [:julienw]]

Attachment: fdn-error.png

Hey Fang, here is a modified version (I removed the padding). Tell me which one you prefer.

Comment 16

[Julien Wajsberg [:julienw]]

Comment on attachment 8580697 [details] [review]
[gaia] julienw:944644-use-lists-for-numbers > mozilla-b2g:master

Hey Steve,

ready for the next review :) I've put the changes in a separate commit.

Comment 17

[Steve Chung [:steveck]]

It looks good now, but I have 2 small questions on github. And I'm not sure if we need to wait for Fang's ui-review, wdyt?

Comment 18

[Julien Wajsberg [:julienw]]

I answered both questions; tell me what you think :)

I'd prefer to have Fang's review but I'm fine landing this without her review (especially that the previous version likely landed without ui-review too.. I think the new version is nicer).

Comment 19

[Steve Chung [:steveck]]

Comment on attachment 8580697 [details] [review]
[gaia] julienw:944644-use-lists-for-numbers > mozilla-b2g:master

I left a comment on github and overall looks good. Since we don't have ui-review or previous version and the new version follows the shared comfirm dialog style, I'm fine if you prefer to land without ui-review because of deadline.

Comment 20

[Julien Wajsberg [:julienw]]

Comment on attachment 8580697 [details] [review]
[gaia] julienw:944644-use-lists-for-numbers > mozilla-b2g:master

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): bug 913421
[User impact] if declined: The phone numbers are not correctly displayed in RTL in this error dialog.
[Testing completed]: yes (manually + some unit tests)
[Risk to taking this patch] (and alternatives if risky): low
[String changes made]: yes

Comment 21

[GitHub Autolander Bot]

Pull request has landed in master: https://github.com/mozilla-b2g/gaia/commit/465e03152de4ec2d49ca583473ae741169435b60

Comment 22

[bhavana bajaj [:bajaj]]

I would like to have ui-review+, to avoid any back-forth later on. I've seen this in other areas where we had to make more changes to accommodate the UX feedback and want to avoid the mess. Will ping fang to speed this up.

Comment 23

[Fang Shih [:fang]]

Comment on attachment 8582404 [details]
fdn-error.png

I think I prefer this one better, so it won't looks so separated when there are only two list of numbers, Thanks!

Comment 24

[Julien Wajsberg [:julienw]]

Good, this is the one that's in the landed patch :) Thanks Fang !

Comment 25

[Ryan VanderMeulen [:RyanVM]]

v2.2: https://github.com/mozilla-b2g/gaia/commit/666c89c8ee9c8a104f7c724a6447126a6d9f916b

Comment 26

[Sue]

Attachment: verify_pass.png

This issue has been verified as pass on latest build of Flame 2.2/3.0 and Nexus 5 2.2/3.0 by STRs:
1. Enable FDN in Settings.
2. Launch Message.
3. Send a message to a number with "+" symbol that is not an Authorized number of FDN.
4. Observe the error message.
Result: The "+" symbol is shown at right side of number as expected.
See attachment:verify_pass.png
Rate:0/5

Device: Flame 2.2 (pass)
Build ID               20150512002502
Gaia Revision          c4c1bf443f2b01c2ba918780510fd4c639a3c360
Gaia Date              2015-05-11 14:12:24
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/70782f19acbf
Gecko Version          37.0
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150512.041644
Firmware Date          Tue May 12 04:16:55 EDT 2015
Bootloader             L1TC000118D0

Device: Flame 3.0 (pass)
Build ID               20150512160203
Gaia Revision          3654ec1d7ce1e0a56a34d5c3b06f6a9b33ff79ad
Gaia Date              2015-05-12 05:13:33
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/42db79f3cd6b
Gecko Version          41.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150512.192015
Firmware Date          Tue May 12 19:20:26 EDT 2015
Bootloader             L1TC000118D0

Device: Nexus 5 2.2 (pass)
Build ID               20150512162502
Gaia Revision          e048df68f6f4853b5826a8816e143d95258149de
Gaia Date              2015-05-12 19:10:26
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/9edadb35caca
Gecko Version          37.0
Device Name            hammerhead
Firmware(Release)      5.1
Firmware(Incremental)  eng.cltbld.20150512.202258
Firmware Date          Tue May 12 20:23:14 EDT 2015
Bootloader             HHZ12f

Device: Nexus 5 3.0 (pass)
Build ID               20150512160203
Gaia Revision          3654ec1d7ce1e0a56a34d5c3b06f6a9b33ff79ad
Gaia Date              2015-05-12 05:13:33
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/42db79f3cd6b
Gecko Version          41.0a1
Device Name            hammerhead
Firmware(Release)      5.1
Firmware(Incremental)  eng.cltbld.20150512.191625
Firmware Date          Tue May 12 19:16:41 EDT 2015
Bootloader             HHZ12f