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

VERIFIED FIXED in 2.2 S9 (3apr)

Status

defect
P2
normal
VERIFIED FIXED
6 years ago
4 years ago

People

(Reporter: steveck, Assigned: julienw)

Tracking

({late-l10n})

unspecified
2.2 S9 (3apr)
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:2.2+, b2g-v2.2 verified, b2g-master verified)

Details

Attachments

(5 attachments)

(Reporter)

Description

6 years ago
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.
Summary: [Gaia] Replacing <br> with proper lists while displaying error messages with the recipients case → [Messages][Refactoring] Replacing <br> with proper lists while displaying error messages with the recipients case
(Assignee)

Comment 1

4 years ago
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
Flags: needinfo?(fabien)
(Assignee)

Updated

4 years ago
Blocks: 964033
(Assignee)

Updated

4 years ago
Blocks: messages-rtl
No longer blocks: 964033
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?
Flags: needinfo?(fabien) → needinfo?(azasypkin)
Posted image 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.
Flags: needinfo?(azasypkin)
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.
Flags: needinfo?(jlorenzo)
Priority: -- → P3
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.
Flags: needinfo?(jlorenzo)
Keywords: qawanted
(Assignee)

Comment 6

4 years ago
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.
Keywords: qawanted
(Assignee)

Updated

4 years ago
blocking-b2g: --- → 2.2?
Triage: Already marked as P3 by the RTL owners, not blocking 2.2 then.
blocking-b2g: 2.2? → ---
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.
blocking-b2g: --- → 2.2?
Priority: P3 → P2
Triage: Agreed on comment 8.
blocking-b2g: 2.2? → 2.2+
(Assignee)

Updated

4 years ago
Assignee: nobody → felash
(Assignee)

Comment 10

4 years ago
I'll have to change some strings.
Keywords: late-l10n
(Assignee)

Comment 12

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

Hey Steve,

tell me what you think :)
Attachment #8580697 - Flags: review?(schung)
(Assignee)

Comment 13

4 years ago
Posted image 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 :) )
Attachment #8580842 - Flags: ui-review?(fshih)
(Reporter)

Comment 14

4 years ago
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.
Attachment #8580697 - Flags: review?(schung)
(Assignee)

Comment 15

4 years ago
Posted image fdn-error.png
Hey Fang, here is a modified version (I removed the padding). Tell me which one you prefer.
Attachment #8582404 - Flags: ui-review?(fshih)
(Assignee)

Comment 16

4 years ago
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.
Attachment #8580697 - Flags: review?(schung)
(Reporter)

Comment 17

4 years ago
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?
Flags: needinfo?(felash)
(Assignee)

Comment 18

4 years ago
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).
Flags: needinfo?(felash)
(Reporter)

Comment 19

4 years ago
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.
Attachment #8580697 - Flags: review?(schung) → review+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
(Assignee)

Comment 20

4 years ago
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
Attachment #8580697 - Flags: approval-gaia-v2.2?
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
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 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!
Attachment #8582404 - Flags: ui-review?(fshih) → ui-review+

Updated

4 years ago
Attachment #8580842 - Flags: ui-review?(fshih) → ui-review-
(Assignee)

Comment 24

4 years ago
Good, this is the one that's in the landed patch :) Thanks Fang !
Target Milestone: --- → 2.2 S9 (3apr)
Attachment #8580697 - Flags: approval-gaia-v2.2? → approval-gaia-v2.2+

Comment 26

4 years ago
Posted image 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

Updated

4 years ago
Status: RESOLVED → VERIFIED
QA Whiteboard: [MGSEI-Triage+]
You need to log in before you can comment on or make changes to this bug.