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

Status: RESOLVED FIXED

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

Description

[Rachel Pribble [:rpribble]]

Attachment: 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

Comment 1

[Francisco Jordano [:arcturus] [:francisco]]

Thanks Julien, taking care of this

Comment 2

[Francisco Jordano [:arcturus] [:francisco]]

Thanks Julien, taking care of this

Comment 3

[Montoya Clemmons (MClemmons)]

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

Comment 4

[Taichi Matsuura]

Attachment: 2014-04-08-11-05-41.png

Tap "cancel" -> close selector screen.
Tap "item" -> close selector screen.

Comment 5

[Taichi Matsuura]

Hi Francisco,

I am working for this bug.
Could you assign me if you don't mind.
Attached file is draft Im making.

Comment 6

[Julien Wajsberg [:julienw]]

(don't know if Francisco reads the bugmail without a needinfo :) )

Comment 7

[Francisco Jordano [:arcturus] [:francisco]]

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!

Comment 8

[Taichi Matsuura]

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

Comment 9

[Francisco Jordano [:arcturus] [:francisco]]

Sure,

I'll be glad of reviewing it :)

Comment 10

[Francisco Jordano [:arcturus] [:francisco]]

Attachment: Pointer to PR 18200

Comment 11

[Francisco Jordano [:arcturus] [:francisco]]

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!

Comment 12

[Taichi Matsuura]

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?

Comment 13

[Francisco Jordano [:arcturus] [:francisco]]

(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!

Comment 14

[Taichi Matsuura]

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

Comment 15

[Francisco Jordano [:arcturus] [:francisco]]

Hi Taichi,

so are we ready for a second review round?

When ever you are, just setup the flag review? to myself.

Comment 16

[Taichi Matsuura]

Attachment: PR review : Bug 972573 - Add cancel button on the selection screen

There is a couple of errors in travis test, but I dont know how to re-run travis.

Comment 17

[Francisco Jordano [:arcturus] [:francisco]]

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.

Comment 18

[Michał Budzyński (:michalbe)]

I'm on it now. 
Taichi, I reruned Travis tests and start to reviewing your patch now.

Comment 19

[Michał Budzyński (:michalbe)]

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

Comment 20

[Francisco Jordano [:arcturus] [:francisco]]

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.

Comment 21

[Taichi Matsuura]

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!

Comment 23

[Kotaro Oki]

Attachment: PR review: Bug972573

Hi, Michal.
I have modified to use the Action menu.
Please review it.

https://github.com/mozilla-b2g/gaia/pull/20051

Comment 24

[Michał Budzyński (:michalbe)]

Thanks Kotaro,
I'm traveling today so I'll review the patch at the beginning of next week. Cheers.

Comment 25

[Kotaro Oki]

(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.

Comment 26

[Michał Budzyński (:michalbe)]

Hey Kotaro, 
I'm on it now.
Thanks

Comment 27

[Michał Budzyński (:michalbe)]

Comment on attachment 8434835 [details] [review]
PR review: Bug972573

Comments in the pull request on Github.

Comment 28

[Kotaro Oki]

(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.

Comment 29

[Michał Budzyński (:michalbe)]

Kotaro,

I made some comments on GH. Thanks for your work!

Comment 30

[Kotaro Oki]

Attachment: 2nd PR review: Bug972573

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.

Comment 31

[Michał Budzyński (:michalbe)]

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 32

[Michał Budzyński (:michalbe)]

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!

Comment 33

[Kotaro Oki]

(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.

Comment 34

[Michał Budzyński (:michalbe)]

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!

Comment 35

[Francisco Jordano [:arcturus] [: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!

Comment 36

[Kotaro Oki]

(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.

Comment 37

[Francisco Jordano [:arcturus] [:francisco]]

Hi Kotaro,

thanks for the work, I'll assing the review flags on the PR to myself.

Comment 38

[Francisco Jordano [:arcturus] [: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

Comment 39

[Kotaro Oki]

Hi Francisco,

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

Thank you a lot.

Comment 40

[Francisco Jordano [:arcturus] [: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!

Comment 41

[Francisco Jordano [:arcturus] [:francisco]]

Landed in gaia:

https://github.com/mozilla-b2g/gaia/commit/b3d25641cfeb7ab34e7294185ced6fa61ba727a9

Great work folks!