Closed Bug 972573 Opened 6 years ago Closed 6 years ago

[B2G][SMS] The selection screen for adding a contact with multiple phone numbers to a SMS message lacks a cancel, back, or 'x' button

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(b2g-v1.2 affected, b2g-v1.3 affected, b2g-v1.4 affected)

RESOLVED FIXED
2.0 S4 (20june)
Tracking Status
b2g-v1.2 --- affected
b2g-v1.3 --- affected
b2g-v1.4 --- affected

People

(Reporter: rpribble, Assigned: ta-matsuura)

References

Details

(Whiteboard: permafail)

Attachments

(6 files)

Attached image NoCancelOption
Description:
When adding a contact with multiple phone numbers listed to a SMS message, there is no cancel, back, or 'x' button on the phone number selection screen.  Tapping 'okay' without selecting a number will take the user back to the previous screen, but it is not an immediately obvious selection to the user.

Repro Steps:
1) Updated buri to BuildID: 20140212004003
2) Tap the contacts icon
3) Tap the + sign to create a new contact
4) Create and save a new contact with two phone numbers
5) Tap the home button
6) Tap the SMS icon
7) Tap the button in the upper right to create a new message
8) Tap the + sign to add a contact to the sms
9) Tap the new created contact with two numbers
10) Observe the lack of cancel option on the phone number selection screen that appears

Actual:
No cancel option on the phone number selection screen.

Expected:
No confusing UI.

Environmental Variables:
Device: buri v13 moz ril
BuildID: 20140212004003
Gaia: ce17d5eae7b1893ae4397c814b10ae598fcbdb58
Gecko: ab07e61c2eb0
Version: 28.0
Firmware Version: v1.2-device.cfg

Notes:

Repro frequency: 100%
Link to failed test case: https://moztrap.mozilla.org/manage/cases/?filter-id=5009
See attached: Screenshot
Component: Gaia::SMS → Gaia::Contacts
Flags: needinfo?(francisco.jordano)
Thanks Julien, taking care of this
Assignee: nobody → francisco.jordano
Flags: needinfo?(francisco.jordano)
Thanks Julien, taking care of this
Whiteboard: burirun1.3-3 → burirun1.3-3, burirun1.4-1
Whiteboard: burirun1.3-3, burirun1.4-1 → permafail
No cancel option on the phone number selection screen appears on 1.4 Buri following the STR in Comment 0.

Environmental Variables: 
Device: Buri 1.4 MOZ 
BuildID: 20140324000202 
Gaia: 730670951e40b2317a167fcd07c398bb662d6e87 
Gecko: a44f8b39c2c8 
Version: 30.0a2
Base image: v1.2-device.cfg
Attached image 2014-04-08-11-05-41.png
Tap "cancel" -> close selector screen.
Tap "item" -> close selector screen.
Hi Francisco,

I am working for this bug.
Could you assign me if you don't mind.
Attached file is draft Im making.
(don't know if Francisco reads the bugmail without a needinfo :) )
Flags: needinfo?(francisco.jordano)
Thanks Julien!

Definitely the needinfo helps a lot!

Thanks Taichi for taking care of this, I'm assigning to you, if you need help ping me (flag me with needinfo). Will be happy to help!
Assignee: francisco.jordano → ta-matsuura
Flags: needinfo?(francisco.jordano)
Blocks: 974864
Hi Francisco,

I have sent PR, if you are not proper person to review,
could you announce this to someone else?
https://github.com/mozilla-b2g/gaia/pull/18200

Thanks,
Taichi
Flags: needinfo?(francisco.jordano)
Sure,

I'll be glad of reviewing it :)
Flags: needinfo?(francisco.jordano)
Attached file Pointer to PR 18200
Attachment #8405438 - Flags: review?(francisco.jordano)
Comment on attachment 8405438 [details] [review]
Pointer to PR 18200

Hi,

Great work here!

The patch is a bit bigger than I expected, but I'm amazed by the work done.

Left some comments on the github PR, mainly:

- No need to add a new web activity
- If using value selector, try to lazyload from a template
- Reuse shared style for value selector in shared

Thanks a lot!
Attachment #8405438 - Flags: review?(francisco.jordano) → review-
Big thanks for reviewing Francisco.
I have quick question and some comments.

> - No need to add a new web activity
We need new activity for massage app. It'll support "mms to email address" and contacts app need to return what user choose (phone number or email address) back to message app.
In this case message app does not know which value selected, thus "result" contains an exact user selected value using new web activity.

We also consider other app which already using "webcontacts/tel", "webcontacts/email" so on.
Im afraid they will get the impact from my changes.
I dont mind changing the naming, please tell me.

> - If using value selector, try to lazyload from a template
Do you mean that we prepare value_selector.html, then do what details.js does.

> - Reuse shared style for value selector in shared
I loaded value_selector.css in shared but it made the broken screen. We need own value selector right?
Flags: needinfo?(francisco.jordano)
(In reply to Taichi Matsuura from comment #12)
> Big thanks for reviewing Francisco.
> I have quick question and some comments.
> 
> > - No need to add a new web activity
> We need new activity for massage app. It'll support "mms to email address"
> and contacts app need to return what user choose (phone number or email
> address) back to message app.
> In this case message app does not know which value selected, thus "result"
> contains an exact user selected value using new web activity.

My point is that MMS doesn't support sending to email yet, so it shouldn't be in contacts, until we have the feature in message app.

> 
> We also consider other app which already using "webcontacts/tel",
> "webcontacts/email" so on.
> Im afraid they will get the impact from my changes.
> I dont mind changing the naming, please tell me.
> 
> > - If using value selector, try to lazyload from a template
> Do you mean that we prepare value_selector.html, then do what details.js
> does.

Exactly, have a specific html view that is lazy loaded as details.html + details.js

> 
> > - Reuse shared style for value selector in shared
> I loaded value_selector.css in shared but it made the broken screen. We need
> own value selector right?

Oh, damn it! If it's not possible to reuse I'm ok then.

In general we are adding a lot of complexity, this bug was for adding a cancel option, not for adding extra functionality like adding emails. That's my concern, I would like to have that functionality once sms supports sending to email.

Thanks a lot!
Flags: needinfo?(francisco.jordano)
Hi Francisco,

Please review https://github.com/mozilla-b2g/gaia/pull/18582.

(In reply to Francisco Jordano [:arcturus] from comment #13)
> My point is that MMS doesn't support sending to email yet, so it shouldn't
> be in contacts, until we have the feature in message app.

I understood and deleted some code with regard to MMS. 
 
> Exactly, have a specific html view that is lazy loaded as details.html +
> details.js

Yes, I used lazy load.
Hope this PR will be merged quickly.

Thanks,
Taichi
Flags: needinfo?(francisco.jordano)
Hi Taichi,

so are we ready for a second review round?

When ever you are, just setup the flag review? to myself.
Flags: needinfo?(francisco.jordano)
There is a couple of errors in travis test, but I dont know how to re-run travis.
Attachment #8412357 - Flags: review?(francisco.jordano)
Comment on attachment 8412357 [details] [review]
PR review : Bug 972573 - Add cancel button on the selection screen

Hi Michal, could you take a look to this as well? Would appreciate your feedback here.
Attachment #8412357 - Flags: feedback?(mbudzynski)
I'm on it now. 
Taichi, I reruned Travis tests and start to reviewing your patch now.
Comment on attachment 8412357 [details] [review]
PR review : Bug 972573 - Add cancel button on the selection screen

Taichi, great work, thanks for your patch. However, I have one main concern - Why do you need out own Value Selector? Can't we use the Action menu [1] from building blocks? It has everything we need - list of elements & cancel button. Using it will simplify your changes in the code and we will keep consistency among the apps.


[1] http://buildingfirefoxos.com/building-blocks/action-menu.html
Attachment #8412357 - Flags: feedback?(mbudzynski) → feedback-
Flags: needinfo?(ta-matsuura)
Comment on attachment 8412357 [details] [review]
PR review : Bug 972573 - Add cancel button on the selection screen

Hi,

we are still waiting for feedback requested by Michal, he did a feedback round and looking for solving some problems.

Thanks,
F.
Attachment #8412357 - Flags: review?(francisco.jordano)
Thanks Michal for your feedback and sorry to have kept you waiting.
I was bit busy for other stuff and I will start this again!
Flags: needinfo?(ta-matsuura)
Target Milestone: --- → 2.0 S4 (20june)
Duplicate of this bug: 1020454
Attached file PR review: Bug972573
Hi, Michal.
I have modified to use the Action menu.
Please review it.

https://github.com/mozilla-b2g/gaia/pull/20051
Attachment #8434835 - Flags: review?(mbudzynski)
Thanks Kotaro,
I'm traveling today so I'll review the patch at the beginning of next week. Cheers.
(In reply to Michał Budzyński (:michalbe) from comment #24)
> Thanks Kotaro,
> I'm traveling today so I'll review the patch at the beginning of next week.
> Cheers.

Hi Michał.
> I'm traveling today so I'll review the patch at the beginning of next week.
I see.
This Bug has blocking bug(Bug974864).
I want to fix it, so please review ASAP.
Thanks in advance.
Hey Kotaro, 
I'm on it now.
Thanks
Comment on attachment 8434835 [details] [review]
PR review: Bug972573

Comments in the pull request on Github.
Attachment #8434835 - Flags: review?(mbudzynski)
(In reply to Michał Budzyński (:michalbe) from comment #27)
> Comment on attachment 8434835 [details] [review]
> PR review: Bug972573
> 
> Comments in the pull request on Github.

Thank you for review.
I have left some comments on GitHub.
Please check it.
Flags: needinfo?(mbudzynski)
Kotaro,

I made some comments on GH. Thanks for your work!
Flags: needinfo?(mbudzynski)
Hi Michał,

Thank you for review.
I have fixed all issues in https://github.com/mozilla-b2g/gaia/pull/20051.
Please review new PR.

Thanks.
Attachment #8439167 - Flags: review?(mbudzynski)
I'm siting to this now. 

For the record - next time pls just update the old pull request, I'll be able to see my comments on your changes, and your responses to them if you'll have any. Opening a new PR & new branch for every proposal is painful.
Comment on attachment 8439167 [details] [review]
2nd PR review: Bug972573

Small, mostly cosmetic changes requested on Github. Please update the same PR with changes and ask for review again. Thanks Kotaro, great job!
Attachment #8439167 - Flags: review?(mbudzynski)
(In reply to Michał Budzyński (:michalbe) from comment #31)
> I'm siting to this now. 
> 
> For the record - next time pls just update the old pull request, I'll be
> able to see my comments on your changes, and your responses to them if
> you'll have any. Opening a new PR & new branch for every proposal is painful.

Sorry for inconvenient.
I will do the next time.

(In reply to Michał Budzyński (:michalbe) from comment #32)
> Comment on attachment 8439167 [details] [review]
> 2nd PR review: Bug972573
> 
> Small, mostly cosmetic changes requested on Github. Please update the same
> PR with changes and ask for review again. Thanks Kotaro, great job!

I have confirmed and fixed.
And update the same PR(https://bugzilla.mozilla.org/attachment.cgi?id=8439167).
Please review again.

Thanks.
Attachment #8439167 - Flags: review?(mbudzynski)
Comment on attachment 8439167 [details] [review]
2nd PR review: Bug972573

r+ from me, thanks Kotaro, it was my pleasure to work with you on this. Please stash your commits into one (we have one commit per feature policy at Mozilla), and even if we put big effort in reviewing & fixing this patch, I would like :arcturus to take a look on it as well since he is the mighty owner of the Contacts App.

Thanks one more time!
Attachment #8439167 - Flags: review?(mbudzynski)
Attachment #8439167 - Flags: review+
Attachment #8439167 - Flags: feedback?(francisco)
Comment on attachment 8439167 [details] [review]
2nd PR review: Bug972573

Left some comments that would like to be addressed before landing, specially the lack of unit tests for the action_menu.js.

Also some other nit.

Thanks for the great work!
Attachment #8439167 - Flags: feedback?(francisco)
(In reply to Francisco Jordano [:arcturus] [:francisco] from comment #35)
> Comment on attachment 8439167 [details] [review]
> 2nd PR review: Bug972573
> 
> Left some comments that would like to be addressed before landing, specially
> the lack of unit tests for the action_menu.js.
> 
> Also some other nit.
> 
> Thanks for the great work!

Hi Francisco,

I have confirmed and fixed.
Please review again.

I do not know what the Flags do I set.
So I check the NI.

Thanks.
Flags: needinfo?(francisco)
Hi Kotaro,

thanks for the work, I'll assing the review flags on the PR to myself.
Flags: needinfo?(francisco)
Attachment #8439167 - Flags: review?(francisco)
Comment on attachment 8439167 [details] [review]
2nd PR review: Bug972573

Great work!

Such an amazing work here, tested and working like charm, code wise all the suggestions are implemented, so I'm pretty happy with this.

This is almost ready, just left a tiny tiny comment regarding style of the l10n strings, once fixed, please ping me back againg for reviewing and we merge, after that change we won't do any more code review, we will merge the feature :D
Attachment #8439167 - Flags: review?(francisco)
Hi Francisco,

I have confirmed and fixed.
Please review it.
I hope this is last review request.

Thank you a lot.
Attachment #8439167 - Flags: review?(francisco)
Comment on attachment 8439167 [details] [review]
2nd PR review: Bug972573

Yeah!

Great work, thanks a lot for adding all the changes, I'm r+ and merging this functionality!
Attachment #8439167 - Flags: review?(francisco) → review+
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Duplicate of this bug: 1043626
You need to log in before you can comment on or make changes to this bug.