77.24 KB, image/png
46 bytes, text/x-github-pull-request
|Details | Review|
113.40 KB, image/png
11.30 KB, image/png
33.70 KB, image/png
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
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 ) 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  https://github.com/mozilla-b2g/gaia/blob/70fb49fb147d237ce3de4654daef2976e23fbfba/apps/sms/js/errors.js
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)
(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.
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.
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.
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.
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+
I'll have to change some strings.
Comment on attachment 8580697 [details] [review] [gaia] julienw:944644-use-lists-for-numbers > mozilla-b2g:master Hey Steve, tell me what you think :)
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)
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.
Hey Fang, here is a modified version (I removed the padding). Tell me which one you prefer.
Attachment #8582404 - Flags: ui-review?(fshih)
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.
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?
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 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+
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?
Pull request has landed in master: https://github.com/mozilla-b2g/gaia/commit/465e03152de4ec2d49ca583473ae741169435b60
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+
Good, this is the one that's in the landed patch :) Thanks Fang !
Attachment #8580697 - Flags: approval-gaia-v2.2? → approval-gaia-v2.2+
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
You need to log in before you can comment on or make changes to this bug.