880624 - [MMS/SMS] The dialog when tapping on a contact into ‘To’ field is not correct

Status: VERIFIED FIXED

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

Description

[isabelrios]

Attachment: Removing a recipient dialog

Bug seen on Unagi v1-train
Gecko-65bbcee
Gaia-13c6246

PROCEDURE
1.Start creating a SMS or MMS.
2.Tap on ‘+’ to add one recipient from contacts
3.Once the contact is into ‘To’ field, tap on it
4.Check the dialog shown

EXPECTED
The dialog shown should be the specified on page 9 of wireframe pack HTML5_SMS-MMSUserStorySpecifications_20130503_V8.0. 

ACTUAL
Current dialog message does not show info about the contact’s number and it is not matching the specs. Please see screenshot attached

For a better understanding of the impact of this screen here is the UX feedback:
1)	Disambiguation #1. If the user has more than one ‘Aaaa’ in their address book it gives them the opportunity to understand which ‘Noe’ the message is being sent to
2)	Disambiguation  #2. (this is the most important disambiguation) If the ‘Aaaa’ contact has more than one phone associated to it, this screen should allow the user to know which ‘Aaaa‘ phone has been added to the ‘to’ field. If the phone information is not added to this screen there is absolutely no opportunity for the end user to disambiguate which ‘Aaaa’ phone has been added to the ‘to’ field

Comment 1

[Stephany Wilkes]

Ayman, can you please confirm the spec and expected behavior? Thanks!

Comment 2

[ayman maat :maat]

(In reply to Stephany Wilkes from comment #1)
> Ayman, can you please confirm the spec and expected behavior? Thanks!

I can confirm Isabel is correct in her description that this functionality should be implemented as specified in the wireframes for reasons of disambiguation and reduction of end user cognitive loading.

Comment 3

[Rick Waldron [:rwaldron]]

Sorry about that, my initial understanding was incorrect.

Comment 4

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

Hi,

Ayman I was doing the proper changes, propagating the information we need (we were just missing the phone type, and were not using the phone number).

But realised that we are using a confirm to send the message. In that case we can setup just a title, so the buttons wont be 'Remove' or 'Cancel', but the standards 'Cancel' and 'OK'.

As well we can specify one line (ok, we can break the line) but is a single text.

You can take a look to the poc:

http://gyazo.com/f1b75e9befd6d1e3eeee16060ea79c55

Also in the branch:

https://github.com/arcturus/gaia/tree/bug-880624

I would suggest to do a wording to fit in a confirm, but as you comment add the extra information to disambiguate some cases.

Thanks!

Comment 5

[Rick Waldron [:rwaldron]]

@Francisco, ugh that is a serious can of worms.

Comment 6

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

@Rick, yup, we will need a clear confirmation of what we can do.

Thx!

Comment 7

[bov]

This issue tested on Leo 1.1 (Test Run Firefox OS 1.1.0 Full – Leo)
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18/rev/8e3f39363c54
Gaia: ea18de80fd04110756becaed214656642388401d
Platform Version: 18.0

STEPS:
1.Start creating a SMS or MMS.
2.Tap on ‘+’ to add one recipient from contacts
3.Once the contact is into ‘To’ field, tap on it
4.Check the dialog shown

EXPECTED
The dialog shown should be the specified on page 9 of wireframe pack HTML5_SMS-MMSUserStorySpecifications_20130503_V8.0. : 
Contact info:
- first name
- last name
- type
- phone number
Options:
- Remove
- Cancel

ACTUAL
Current dialog message does not show correct options and labels as defined in the corresponding specs. Ask the user about removing the contact. And the two only options are "Cancel" and "OK"

Link to failed test case:
https://moztrap.mozilla.org/manage/case/7916/
https://moztrap.mozilla.org/manage/case/7917/
https://moztrap.mozilla.org/manage/case/7918/
https://moztrap.mozilla.org/manage/case/7969/

Comment 8

[Dietrich Ayala (:dietrich)]

The spec looks clear. Blocking+ for now.

If carrier/hardware think we should not block, can renominate.

Comment 9

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

Not so sure about the hard dependency on bug 883911, it's true that we need to be clear on the exact way of display all those fields.

I'm more worry about the fact that the spec is a disambiguation screen (you can have acontact with several numbers and that helps you to discern which one are you texting) that doesn't fit the 'confirm' model.

Anyway I'll try to figure out with Ayman if we can fit that 'disambiguation' on a confirm dialog or not.

Cheers!
F.

Comment 10

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

After talking with Ayman and seeing what we can do with the CustomDialog class coming from the shared components, that is already being used, I totally agree with Rick (not like my previous comment), that we will have the dependency on bug 883911 to be sure we display the same information across all parts of the application.

Comment 11

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

Attachment: Pointer to PR 10491

Disambiguation screen as suggested by Ayman

As we are using the CustomDialog coming from shared, we have two main components:
title: We used the display name
message: We are using the text showed to disambiguate the contact in the contacts suggestions

Also the semantic of the CustomDialog forces us to put a 'negative' option on the right hand side (instead of the left) as was in the wf.

All this has been reviewed and approved by Ayman.

Victoria I'm adding you as needinfo cause the 'negative' color for the 'Remove' button is dark orange, and would like to be sure that's the way we do it here.

Thanks.

Comment 12

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

Comment on attachment 764847 [details]
Pointer to PR 10491

Not needed anymore since we will stop using custom dialog to implement the original specs

Comment 13

[Rick Waldron [:rwaldron]]

https://github.com/mozilla-b2g/gaia/pull/10491/files#r4845430

Once this is fixed, r=me

Comment 14

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

(In reply to Rick Waldron from comment #13)
> https://github.com/mozilla-b2g/gaia/pull/10491/files#0
> 
> Once this is fixed, r=me

Hi Rick,

thanks for taking another look, but your last comment is not clear to me, it seems that the link is not pointing me to what you want me to fix.

Are you talking about 'unfocusing' the keyboard, you want the focus back?

Thanks,
F.

Comment 15

[Borja Salguero [:borjasalguero]]

Comment on attachment 764847 [details]
Pointer to PR 10491

Evan, this is the new generic dialog, could you take a look? Thanks!

Comment 16

[Evan Tseng [:evanxd]]

Hi Borja,

Cool.
Hope we could land the patch today. :)

Thanks.

Comment 17

[Borja Salguero [:borjasalguero]]

Comment on attachment 764847 [details]
Pointer to PR 10491

Thanks for the patch Francisco!

Comment 18

[Borja Salguero [:borjasalguero]]

https://github.com/mozilla-b2g/gaia/commit/72565090cb39dc65d136027769018a76cd61041b
https://github.com/arcturus/gaia/commit/8b41ee9a80c1d6b3613339e7388035d3fbcc575d

R+. Landed!

Comment 19

[Gareth Aye [:gaye] (back from PTO)]

I was not able to uplift this bug to v1-train.  If this bug has dependencies which are not marked in this bug, please comment on this bug.  If this bug depends on patches that aren't approved for v1-train, we need to re-evaluate the approval.  Otherwise, if this is just a merge conflict, you might be able to resolve it with:

  git checkout v1-train
  git cherry-pick -x -m1 72565090cb39dc65d136027769018a76cd61041b
  <RESOLVE MERGE CONFLICTS>
  git commit

Comment 20

[Rick Waldron [:rwaldron]]

Borja, 

There are a lot of issues with this patch:

1. It duplicates too much functionality 
  - OptionMenu and Dialog can definitely be merged into a single entity
  - Utils.getContactDisplayInfo should use Utils.getCarrierTag

2. There are several changes that have nothing to do with the bug

3. Utils.getContactDisplayInfo reintroduces a nasty bug that was fixed elsewhere. The code that reintroduces this issue is actually unnecessary since it only duplicates the functionality of Utils.getCarrierTag

Comment 21

[bov]

This issue tested on Leo 1.1 (Test Run Firefox OS 1.1.0 Full – Leo Run 4)
Gecko a378807ff04076c20f08b0102286b9eb2d08d60a
Gaia 1436e2778b90bd74635b0b94d1cf8ccb0d71b60c
Platform Version: 18.1

STEPS:
1.Start creating a SMS or MMS.
2.Tap on ‘+’ to add one recipient from contacts
3.Once the contact is into ‘To’ field, tap on it
4.Check the dialog shown

EXPECTED
The dialog shown should be the specified on page 9 of wireframe pack HTML5_SMS-MMSUserStorySpecifications_20130503_V8.0. : 
Contact info:
- first name
- last name
- type
- phone number
Options:
- Remove
- Cancel

ACTUAL
Current dialog message does not show correct options and labels as defined in the corresponding specs. Ask the user about removing the contact. And the two only options are "Cancel" and "OK"

Link to failed test case:
https://moztrap.mozilla.org/manage/case/7918/
https://moztrap.mozilla.org/manage/case/7916/
https://moztrap.mozilla.org/manage/case/7917/
https://moztrap.mozilla.org/manage/case/7969/

Comment 22

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

(In reply to bov from comment #21)
> This issue tested on Leo 1.1 (Test Run Firefox OS 1.1.0 Full – Leo Run 4)
> Gecko a378807ff04076c20f08b0102286b9eb2d08d60a
> Gaia 1436e2778b90bd74635b0b94d1cf8ccb0d71b60c
> Platform Version: 18.1
> 
> STEPS:
> 1.Start creating a SMS or MMS.
> 2.Tap on ‘+’ to add one recipient from contacts
> 3.Once the contact is into ‘To’ field, tap on it
> 4.Check the dialog shown
> 
> EXPECTED
> The dialog shown should be the specified on page 9 of wireframe pack
> HTML5_SMS-MMSUserStorySpecifications_20130503_V8.0. : 
> Contact info:
> - first name
> - last name
> - type
> - phone number
> Options:
> - Remove
> - Cancel
> 
> ACTUAL
> Current dialog message does not show correct options and labels as defined
> in the corresponding specs. Ask the user about removing the contact. And the
> two only options are "Cancel" and "OK"
> 
> Link to failed test case:
> https://moztrap.mozilla.org/manage/case/7918/
> https://moztrap.mozilla.org/manage/case/7916/
> https://moztrap.mozilla.org/manage/case/7917/
> https://moztrap.mozilla.org/manage/case/7969/

Sorry, this is still not uplift to v1-train yet

Comment 23

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

Hi Rick,

this landed and some bugs were dependant of this. Some of the functions that you comment are not working in the way the should (like having a single way displaying all the info, standarised).

Again, totally agree with you, and taking into account that is already landend in master and has already a hard dependency for bug 880257. Let's not reopen the bug and fill a new one to address all the issues you want to fix (will be happy to fix them following your criteria)

Thanks!!
F.

Comment 24

[Borja Salguero [:borjasalguero]]

I agree that we should land this, and if you consider that it's something pending here please file a bug. Thanks!

Comment 25

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

@gaye already uplift to v1-train.

Cannot find the flag to say it's fixed in v1-train, do we need to add anything else to move this to v1.1-hd, or it's ok now?

Comment 26

[bov]

Tested on Unagi device.
Build: user.manifest.V1-train.Rel0.4.Sprint12.B-290.Gecko-e78450a.Gaia-7c40bda
Commercial RIL: 152

 STEPS:
1.Start creating a SMS or MMS.
2.Tap on ‘+’ to add one recipient from contacts
3.Once the contact is into ‘To’ field, tap on it
4.Check the dialog shown
 
EXPECTED
The dialog shown should be the specified on page 9 of wireframe pack
HTML5_SMS-MMSUserStorySpecifications_20130503_V8.0. : 
Contact info:
- first name
- last name
- type
- phone number
 Options:
- Remove
- Cancel

ACTUAL
It works as expected

Comment 27

[Rick Waldron [:rwaldron]]

Further issues created by this patch: https://github.com/mozilla-b2g/gaia/commit/8b41ee9a80c1d6b3613339e7388035d3fbcc575d#L2R227 created the issue described here: https://github.com/mozilla-b2g/gaia/pull/10771#issuecomment-20734398

Comment 28

[Alex Keybl [:akeybl]]

(In reply to Rick Waldron from comment #27)
> Further issues created by this patch:
> https://github.com/mozilla-b2g/gaia/commit/
> 8b41ee9a80c1d6b3613339e7388035d3fbcc575d#L2R227 created the issue described
> here: https://github.com/mozilla-b2g/gaia/pull/10771#issuecomment-20734398

Rick, please file a new bug and nominate it if you believe it is critical.

Comment 29

[Wayne Chang [:wchang]]

Hi RyanVM, gaye, can you help and address the comment 25?

(In reply to Francisco Jordano [:arcturus] from comment #25)
> @gaye already uplift to v1-train.
> 
> Cannot find the flag to say it's fixed in v1-train, do we need to add
> anything else to move this to v1.1-hd, or it's ok now?

Rick, 
can you file the found bugs and describe if they are critical? Is it worth Leo taking the patch here without the new problems being fixed? (or should we back this one out if the new bugs cannot be fixed in time?)

Comment 30

[Wayne Chang [:wchang]]

Sorry, answer myself here. b2g-18 denotes v1-train.

Leaving ni?rick for comment 28 and comment 29

Comment 31

[Borja Salguero [:borjasalguero]]

Hi all,
Rick's comment is about a scenario when adding a contact which has been identified and fixed in bug https://bugzilla.mozilla.org/show_bug.cgi?id=888150. As this bug is leo+ as well, the whole functionality will be ready soon. No need to file a new bug due to the reasons explained here. Thanks!

Comment 32

[Borja Salguero [:borjasalguero]]

Actually bug https://bugzilla.mozilla.org/show_bug.cgi?id=888150 has been landed, so the whole functionality is working as expected.

Comment 33

[John Ford [:jhford] CET/CEST Berlin Time]

v1.1.0hd: 7c40bdaeaffae708342fc773926dcfac5389348e