In this bug, we want to do short-term (ie: 1.2) fixes for the recipient panel. We had lots of bugs reported by partners or QA about this, and Ayman and I think we must fix the biggest problem for 1.2. The most broken behaviour is that if the user types something and doesn't tap a contact in the suggestion list, the typed text will be handled like it is a number, even if it's only characters. Here are the proposed changes for 1.2. We don't want new strings so this proposal is new-string-free. * if the trimmed typed text start with a digit -> no change to the behavior, it will be handled like a number * if the trimmed typed text start with something else, the user probably tried to type a contact. + if the user terminates this recipient pressing ';' or 'enter' or tapping elsewhere (so, not tapping something in the suggestions list), we should try to match it as a contact. Questions for Ayman: o Should we do this for 1.2, or are the following behaviours enough? o If we do it, should we take only an exact match, or should we try to replace with the first result?) + if there is no match, we should indicate this recipient is incorrect (eg red border or red shadow?) and ignore it when the user sends the message. This means that if this is the only recipient, the "send" button should stay disabled. + when the recipient is incorrect, tapping it will trigger the "remove this recipient ?" dialog we already have for a normal contact. (this will be changed in bug 914103). I discussed about this orally with Ayman but I'm flagging him to approve or change some things if necessary, and answer the remaining questions. A more complete and consistent fix will be done for 1.3 in bug 914103. Nominating koi? and I'll dupe all existing bugs to this one.

Ayman already gave some wireframes in bug 907088. (https://bug907088.bugzilla.mozilla.org/attachment.cgi?id=792689) These wireframes add new strings (which we want to avoid here) but can help for guidance.

Another different wireframe is in bug 823392 (https://bug823392.bugzilla.mozilla.org/attachment.cgi?id=762629). Ayman worked a lot ;)

LGTM. Were you planning to implement this? If not, I can take it

please take :) but take care Ayman might do small adjustments and we'll also need some visual from Vicky.

(In reply to Julien Wajsberg [:julienw] from comment #7) > please take :) > > but take care Ayman might do small adjustments and we'll also need some > visual from Vicky. Ok, I'll hold off until I get green lights here.

Ayman, This is moving along smoothly now, but there are some remaining questions that I need to follow up on (originally asked by julienw above): > * if the trimmed typed text start with something else, the user probably tried to type a contact. > + if the user terminates this recipient pressing ';' or 'enter' or tapping elsewhere[...], we should try to match it as a contact. > o If we do it, should we take only an exact match, or should we try to replace with the first result?) A single match results in replacing the "manually" entered recipient with the correct contact. What should the app do if there are >1 matches?

- Decouple searchContact from list display - Create ThreadUI.listContact(...) - Create ThreadUI.validateContact(...) - Adds two level validation: - Is the entry questionable? - This kicks off a silent search for matching contacts - Is the entry invalid? - This state is reached if no contacts could be found for the typed value. - Prevents enableSend - Prevents "export" of recipient entry at Send (when mixed with multiple valid recipients) - Refactor Utils.getContactDisplayInfo logic - Adds tests for enableSend NOTE: This is still pending the ni? response from :maat

is this ok to target this bug for v1.2 c3 by Oct 25?

(In reply to Joe Cheng [:jcheng] from comment #11) > is this ok to target this bug for v1.2 c3 by Oct 25? Yes, we're just waiting on some desktop testing related questions to be resolved. Thanks!

- Decouple searchContact from list display - Create ThreadUI.listContact(...) - Create ThreadUI.validateContact(...) - Adds two level validation: - Is the entry questionable? - This kicks off a silent search for matching contacts - Is the entry invalid? - This state is reached if no contacts could be found for the typed value. - Prevents enableSend - Prevents "export" of recipient entry at Send (when mixed with multiple valid recipients) - Refactor Utils.getContactDisplayInfo logic - Adds tests for enableSend

Comment on attachment 813553 [details] https://github.com/mozilla-b2g/gaia/pull/12629 LGTM - Few comments on the PR but nothing standing out

(In reply to Rick Waldron from comment #9) > Ayman, > > This is moving along smoothly now, but there are some remaining questions > that I need to follow up on (originally asked by julienw above): > > > > * if the trimmed typed text start with something else, the user probably tried to type a contact. > > + if the user terminates this recipient pressing ';' or 'enter' or tapping elsewhere[...], we should try to match it as a contact. > > o If we do it, should we take only an exact match, or should we try to replace with the first result?) > > A single match results in replacing the "manually" entered recipient with > the correct contact. What should the app do if there are >1 matches? Take the first result in the list of contacts returned, ni? me if in your opinion this behaviour produces undesirable results or you see issues.

(In reply to Julien Wajsberg [:julienw] from comment #0) > + if there is no match, we should indicate this recipient is incorrect (eg > red border or red shadow?) and ignore it when the user sends the message. > This means that if this is the only recipient, the "send" button should stay > disabled. Yes we should certainly do this, and we will need some messaging to reinforce the indication. In terms of treatment of indication that there is no match, this is more a visual design question. however I am attaching some specs here (which *i know* have attached this sketch on other bugs when we have discussed invalid recipients - its doing something of a tour ;-) ) which give an outline of what/how i feel we need to communicate this to the end user. ni? to Victoria and Jose to give Visual input on this > + when the recipient is incorrect, tapping it will trigger the "remove > this recipient ?" dialog we already have for a normal contact. (this will be > changed in bug 914103). no, we should take the recipient back into 'edit mode' like we do for phone numbers. We cannot guarantee that when the user 'closes' a recipient they meant to close it.. they might have received an incoming call or tapped outside of the 'to field' / keyboard by mistake. We should give them the chance to continue editing the recipient not force them to start from the beginning.

Unflagging myself as I already talked with Jose offline, he'll provide a visual solution for this. Thanks!

(In reply to ayman maat :maat from comment #15) > (In reply to Rick Waldron from comment #9) > > Ayman, > > > > This is moving along smoothly now, but there are some remaining questions > > that I need to follow up on (originally asked by julienw above): > > > > > > > * if the trimmed typed text start with something else, the user probably tried to type a contact. > > > + if the user terminates this recipient pressing ';' or 'enter' or tapping elsewhere[...], we should try to match it as a contact. > > > o If we do it, should we take only an exact match, or should we try to replace with the first result?) > > > > A single match results in replacing the "manually" entered recipient with > > the correct contact. What should the app do if there are >1 matches? > > Take the first result in the list of contacts returned, ni? me if in your > opinion this behaviour produces undesirable results or you see issues. This matches what I assumed and have already implemented :)

Ayman, is it ok to do the various notices in bug 914103 targeting 1.3 ? I'd like to avoid introducing new strings in 1.2.

Ayman, the "no entry" sign is not known in America, maybe we should have another sign. Well, this is Visual Design's problem I guess :)

(In reply to Julien Wajsberg [:julienw] from comment #19) > Ayman, is it ok to do the various notices in bug 914103 targeting 1.3 ? I'd > like to avoid introducing new strings in 1.2. Sure, but please ensure they are landed in 1.3. The messaging is important to orientate the user. (In reply to Julien Wajsberg [:julienw] from comment #20) > Ayman, the "no entry" sign is not known in America, maybe we should have > another sign. Well, this is Visual Design's problem I guess :) No no, the icon in the wireframes is just indicative :) ...The icon's purpose is to reinforce which modules 'wont go though' and also to make a connection between the 'erroneous' contact module and warning message. The Visual Designer who is driving iconography is free to identify/propose the appropriate one.

(In reply to Rick Waldron [:rwaldron][:rick][:rick waldron] from comment #18) > (In reply to ayman maat :maat from comment #15) > > (In reply to Rick Waldron from comment #9) > > > Ayman, > > > > > > This is moving along smoothly now, but there are some remaining questions > > > that I need to follow up on (originally asked by julienw above): > > > > > > > > > > * if the trimmed typed text start with something else, the user probably tried to type a contact. > > > > + if the user terminates this recipient pressing ';' or 'enter' or tapping elsewhere[...], we should try to match it as a contact. > > > > o If we do it, should we take only an exact match, or should we try to replace with the first result?) > > > > > > A single match results in replacing the "manually" entered recipient with > > > the correct contact. What should the app do if there are >1 matches? > > > > Take the first result in the list of contacts returned, ni? me if in your > > opinion this behaviour produces undesirable results or you see issues. > > This matches what I assumed and have already implemented :) Sorted! nice one Rick :)

To give an idea of where we currently are, this is what an "invalid" entry looks like with a valid entry

Here it is the visual spec of how to show an error for an invalid contact. Mainly color changes.

Hi guys, based on Ayman's user story (https://bug920546.bugzilla.mozilla.org/attachment.cgi?id=817769) this the visual solution that applies to v1.2. We should open another bug for the notification messages not included in this version. And... sorry for the delay!

A fast approach is to reuse the in-app messages to notify invalid contacts, but needs iteration.

(In reply to José Vittone from comment #26) > Created attachment 819764 [details] > SMS_MMS__0001_Recipient-error---alert.png > > A fast approach is to reuse the in-app messages to notify invalid contacts, > but needs iteration. This says "Invalid contact..." but the entry may not be a "contact" at all. The entry is always a "recipient", so perhaps "Invalid recipient added"?

José, thanks a lot for the visual design ! The colors are actually subtler than what Rick did ;) We already have another bug for 1.3 ;) that's bug 914103. I'm a little concerned that we don't have an icon to indicate a recipient is invalid. As you probably know, about 10% of men and 1% of women have some sort of color blindness, and therefore for accessibility reason an information can't be conveyed by color only. Moreover, Ayman specifically added an icon, so I think that it's really part of his UX Design. What do you think José ?

Rich, yes, that can be "recipient". I don't know what fits best in this case, maybe Ayman can give us a word on this.

Hi Julien, I agree and we need to iterate and evolve the design. As Ayman commented, the idea is to make some kind of connection between the erroneus recipient and the proper notification. At this moment, the notification is not completely defined so this connection is not going to be achieved. That's why I think this subtle solution is better now. BTW, I thought that colour blindness was between colors rather than color with grays.

José, sounds good. That message doesn't block this ticket, so I will ask from the other ticket. Thanks again!

(In reply to José Vittone from comment #30) > Hi Julien, I agree and we need to iterate and evolve the design. > > As Ayman commented, the idea is to make some kind of connection between the > erroneus recipient and the proper notification. At this moment, the > notification is not completely defined so this connection is not going to be > achieved. I don't exactly see why this prevents us from adding an "invalid" icon in the invalid recipient "box" now. I'd really like to have something else than color to convey this information, whether it's a change in brightness/contrast or an icon. > > That's why I think this subtle solution is better now. BTW, I thought that > colour blindness was between colors rather than color with grays. Yeah, it's much more complicated than this ;) see http://colorvisiontesting.com/color2.htm for a very good explanation. But in short, there is very often a lost of brightness, especially in red-ish colors. (note: thank you for making me read this again ;) )

Julien, I have an iteration that it covers the color blindness. And thank you for your feedback guys!

Hey José, works for me :) Rick, do you think you can use a `:before` css rule to do this ? Or do we need an asset ?

(In reply to Julien Wajsberg [:julienw] from comment #34) > Hey José, > > works for me :) > > Rick, do you think you can use a `:before` css rule to do this ? Or do we > need an asset ? We're good to go! Patch updated using :before + content: "!"

(In reply to Rick Waldron [:rwaldron] from comment #31) > José, sounds good. That message doesn't block this ticket, so I will ask > from the other ticket. Thanks again! hey Rick. you ni?ed me but it is not clear what you want, could you clarify please.

(In reply to José Vittone from comment #33) > Created attachment 820194 [details] > SMS_MMS__0005_Recipient-error-new.png > > Julien, I have an iteration that it covers the color blindness. And thank > you for your feedback guys! Good work Jose :)

(In reply to ayman maat :maat from comment #36) > (In reply to Rick Waldron [:rwaldron] from comment #31) > > José, sounds good. That message doesn't block this ticket, so I will ask > > from the other ticket. Thanks again! > > hey Rick. you ni?ed me but it is not clear what you want, could you clarify > please. Oh, sorry about that. I had entered the "ni?" and then realized I should do this from the other ticket and forgot to remove the ni?

Comment on attachment 813553 [details] https://github.com/mozilla-b2g/gaia/pull/12629 r=me I am so happy with this patch, probably one of the most important patch (from the user perspetive) we did for 1.2 in the SMS app :) The only glitch is that when there is a field with several words (like "Adam L."), then we will never be able to exact match it. I don't care about this for this bug, as very few users will write a whole contact name, especially if it's long, so let's fix this (if possible) in bug 914103. As travis is currently green, please rebase and wait for a green travis before merging !

gaia @ master 7c5b6e746b800b3747c3be6eae67ad3948891497

gaia @ master 7c5b6e746b800b3747c3be6eae67ad3948891497

When trying to edit a recipient after editing other, the content disappear (check video http://youtu.be/geIwrqhj4J8). This is really important because this was marked as koi+, so we need to ensure that it's working as expected. Reopening and asking Julien for the way to fix this (revert? followup?). Thanks!

You're right, let's revert this since it's koi+. I'm doing the revert.

reverting on master: 8cb2e45e3bdd291fad1f342bf10454b1635ff8b9

Also Rick, when you'll reland next time, please edit your commit log ;)

(In reply to Julien Wajsberg [:julienw] from comment #45) > Also Rick, when you'll reland next time, please edit your commit log ;) Just a check in, this is going to require more time tomorrow.

- Do contact search for any accepted recipient that has either - isLookupable = true - isQuestionable = true - Validation Rules: - If there are 1 or more contact record matches - Received an exact match with a single tel record - This is a valid contact - Received an exact match with multiple tel records - This may be a valid contact, extract other number - If no number could be extracted, mark invalid - If input value matches a valid contact - Remove the manually entered recipient - Generate a basic contact data object, source = 'contacts' - Add the newly accepted, valid contact record as a recipient - Return - If input value does not match the first contact record - Try again with the next contact record - Return - Else - Mark the manually entered and accepted recipient as invalid - If there is an active placeholder with typed contents (this happens when editting a manually entered recipient) - Preserve all accepted recipients - Else - Update the manually entered and accepted recipient as "invalid"

All operations are case insensitive. 1. The user... 1. Types the start of a known contact's name (either given name or last name). 2. Accepts a contact as a recipient by tapping an item in the results list. 3. No lookup, selected contact is accepted as a valid recipient. 4. Tapping the contact recipient should display the "remove" dialog 2. The user... 1. Types the full name of a known contact. 2. Taps either <enter>, <;>, the attachment button, or the composer's input box. 3. The known contact is accepted as a valid recipient. 4. Tapping the contact recipient should display the "remove" dialog. (known caveat: contacts that have several words as either given name or last name won't work properly when terminating with <;> or tapping elsewhere) 3. The user... 1. Types the start of a contact's name (either given name or last name). 2. Accepts the contact as a recipient by tapping <enter>. 3. Lookup to get first match. 4. The full contact's name should be shown. 5. Tapping the contact recipient should display the "remove" dialog. 4. The user... 1. Types the name of a unknown contact that won't match a known contact. 2. Accepts the contact as a recipient by tapping <enter>. 3. Lookup to get first match. => Mark the typed text as invalid. 4. Tapping the invalid recipient should allow to edit it. 5. Add some text to the composer input 6. Try to press send => the send button shoul dstay disabled, and nothing should happen 5. The user... 1. Types the name of a unknown contact or the start of an existing contact 2. Accepts the name as a recipient by typing <;>, the attachment button, or tapping the composer's input box 3. Lookup to get exact match 4. Mark the typed text as invalid 5. Tapping the invalid recipient should allow to edit it. 6. The user... A. 1. taps a text that will match at least 2 contacts, then press enter to accept it 2. taps the exact same thing, then press enter => the other contact is matched and is added B. 1. taps a text that will match one contact only, then press enter to accept it 2. taps the exact same thing, then press enter => the typed text is marked as invalid, and typing it should allow to edit it 7. The user... 1. Types a stored number 2. Accepts the number as a recipient by tapping <enter>, <;>, the attachment button, or tapping in the composer's input box 3. Do exact match lookup (via search and filter by normalized tel records) 4. The looked up contact should be shown as a recipient 5. Tapping the contact recipient should display the "remove" dialog 8. The user... A. 1. Types the start of a number stored as a contact 2. Accepts the number as a recipient by tapping <enter>, <;>, the attachment button, or tapping in the composer's input box 3. Should keep the typed number and _not_ use the stored number 4. Tapping the number recipient should allow to edit the number B. 1. Types a random number. 2. Repeat 8.A.2-8.A.4 9. The user... 1. Types the start of a number stored as a contact 2. Taps the known contact in the suggestions list 3. The contact name should be shown as a recipient 4. Tapping the contact recipient should display the "remove" dialog 10. The user... 1. Accepts a known contact as a recipient 2. Accepts an udds an unknown 11. The user... 1. Types an invalid string that does not resolve to a known contact, accepts by tapping <enter> 2. Types a number that does not resolve to a known contact, accepts by tapping <enter> 3. Taps the invalid string 4. Taps the number => We should be able to edit the number 12. The user... 1. Does scenario 8.B twice 2. Tap number 1 (becomes editable) 3. Tap number 2 (becomes editable) 4. Tap number 1 (becomes editable) => We should be able to edit number 1, and number 2 is shown as a normal recipient 13. The user... 1. Does scenario 3 to add a contact 2. Does scenario 8.B to add a number 3. Taps the number (becomes editable) 4. Taps the contact (prompts to remove contact recipient) 5. Tap 'Cancel' => The number and the contact should still be both there, as normal contact (eg: not invalid) 14. The user... 1. Does scenario 5 to add an invalid contact 2. Does scenario 8.B to add a number 3. Tap recipient 1 4. Tap recipient 2 5. Tap recipient 1 => We should be able to edit recipient 1, and recipient 2 is shown as a normal recipient 6. Tap recipient 2 => We should be able to edit recipient 2, and recipient 1 is still shown as invalid

15. The user... 1. Types an invalid string that does not resolve to a known contact, accepts by tapping <enter> 2. Types a number and accepts the contact tapping <enter> 3. Header should say "1 Recipient"

Comment on attachment 824112 [details] https://github.com/mozilla-b2g/gaia/pull/12629 r=me let's try to land again, until Borja finds a bug ;) Let's wait for a green travis! Thanks a lot, I know how this took time.

master: e48489e4dda6efaf4e5f06564b851c64d1648478

Paul, can you somehow integrate comment 48 and comment 49 into moztrap ? Zac, do you think some of them could be made as UI Tests ?

I'll check for this, leave ni? till test case done

Julien we can write them but all of them will be a bit much. Can you cherry-pick 2 or 3 of the highest priority ones for me? Then I'll write them up into tasks in the UI tests component.

I"d say: * 4 * 6A with the additional steps: + at step 2 the suggestions list should not show the previously added contact + step 3, tapping any of the added contact should trigger the "remove contact" dialog * 6B with the additional step: + at step 2, the suggestions list should show nothing Thanks a lot Zac!

I was not able to uplift this bug to v1.2. 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.2, 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.2 git cherry-pick -x -m1 e48489e4dda6efaf4e5f06564b851c64d1648478 <RESOLVE MERGE CONFLICTS> git commit

(In reply to John Ford [:jhford] -- please use 'needinfo?' instead of a CC from comment #56) > I was not able to uplift this bug to v1.2. 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.2, 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.2 > git cherry-pick -x -m1 e48489e4dda6efaf4e5f06564b851c64d1648478 > <RESOLVE MERGE CONFLICTS> > git commit Landed https://github.com/mozilla-b2g/gaia/commit/7bb90d2e127792110029bdfb38f7750e62bf8bbe

Zac, see comment 55 thanks!

Rick, I think that you made the v1.2 tests fail when you uplifted, see https://s3.amazonaws.com/archive.travis-ci.org/jobs/13591308/log.txt Could you please fix them in a adhoc pull request, or I'll have to backout the v1.2 merge...

(In reply to Julien Wajsberg [:julienw] from comment #59) > Rick, I think that you made the v1.2 tests fail when you uplifted, see > https://s3.amazonaws.com/archive.travis-ci.org/jobs/13591308/log.txt > > Could you please fix them in a adhoc pull request, or I'll have to backout > the v1.2 merge... We need to back it out because it depends on another patch any way

Can I let you handle all this ?

Instead of going through the pains of uplifting a patch that may not have any benefit on 1.2, this will cover the renaming of compareDialables to probablyMatches

Uplift supplement landed in v1.2: 505db353e7fe24a1251de2aeb2e6c85c239243f6

QA, could you please test thoroughly the recipient panel behaviour in the "new message" view in v1.2, and report any bug you find? Thanks!

(In reply to Julien Wajsberg [:julienw] from comment #58) > Zac, see comment 55 > > thanks! Julien yep I got this. We're already submitted one test and have a couple more in the pipeline. Thanks!

I went through the cases in comment 48 and comment 49 - Verified fixed on 1.2. I noticed an issue with entering full name of contact and pressing <enter>, <;>, tap attachment icon, or tap on the composer's input box. It will result in having the contact name having the red outline and exclamation briefly as an invalid contact would. This issue is more related to bug 914103 instead of this bug. Device: Buri v1.2 comRIL BuildID: 20131111004004 Gaia: 670b2c8329bca6f142939185be71274166d82bb8 Gecko: 1ac147e4e2f0 Version: 26.0 Firmware Version: US_20131104 RIL information: 01.02.00.019.102

Test cases done, please check https://moztrap.mozilla.org/manage/cases/?pagenumber=1&pagesize=20&sortfield=created_on&sortdirection=desc&filter-tag=2311

(In reply to Paul Yang [: pyang] from comment #67) > Test cases done, please check > https://moztrap.mozilla.org/manage/cases/ > ?pagenumber=1&pagesize=20&sortfield=created_on&sortdirection=desc&filter- > tag=2311 Note - in case where you are tracking getting moztrap test cases, use the in-moztrap flag.