798292 - Recipient names should not show email address

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::E-Mail, defect, P3)

Description

[Victoria Gerchinhoren [:vicky]]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0) Gecko/20100101 Firefox/4.0
Build ID: 20110318052756



Actual results:

Inside the message, when reading, the sender's name in the bubble has the contact name and also the email specified. 


Expected results:

Should have only the contact's name or the email if the contact is not in the Address book.
See: https://www.dropbox.com/sh/gn7qomol29ezppj/h3oHj8AjhG#/

Comment 1

[Steve Chung [:steveck]]

Attachment: https://github.com/mozilla-b2g/gaia/pull/5761

Comment 2

[Steve Chung [:steveck]]

Hi Casey,
Since we still need to display the actual address in certain scenario, could you please define the behavior to display the hided address in the wireframe?
Thanks.

Comment 3

[Andrew Sutherland [:asuth] (he/him)]

(In reply to Steve Chung from comment #2)
> Hi Casey,
> Since we still need to display the actual address in certain scenario, could
> you please define the behavior to display the hided address in the wireframe?

I'm marking needsinfo from cyee, but as I said on the pull request, my recollection from the discussion was that we should display the e-mail address at the top of the menu thing that pops up.  I would start with that and then Casey and/or Victoria can refine it.  I do agree that it would be nice if this was in the wireframes.

Comment 4

[Andrew Sutherland [:asuth] (he/him)]

Attachment: picture of patch in action

Steve, thanks for the ping on the github pull request.  I don't receive e-mails when new commits are pushed to github, so I missed your earlier push.  (If you know of a way to get e-mails when that happens, please let me know.  I didn't see it in the settings.)

Casey, I've attached a screenshot of how this looks.  My imagined vision of what we discussed was basically the "action menu" building block with the e-mail address of the person at the top.  Is that what you were thinking?  I am quite confident this context menu stuff I had originally implemented that's on display here is not desired.

Comment 5

[Andrew Sutherland [:asuth] (he/him)]

Attachment: name displayed only; email collapsed when menu is not visible

Comment 6

[Mike Connor [:mconnor]]

not blocking per blocker retriage.

If the patch is appropriately low-risk we will consider it.

Comment 7

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 670231 [details]
https://github.com/mozilla-b2g/gaia/pull/5761

Clearing review requests while waiting for confirmation from Casey.

Oi! Casey!  Read this bug!

Comment 8

[Andrew Sutherland [:asuth] (he/him)]

Casey ping.

Comment 9

[Casey Yee]

Guys, sorry for the late response on this one.

The bubble should only show the contact name unless we don't have one in which case we should show the email address.

In the read message pane, tapping on a contact bubble will display a value selector:
http://mozilla-b2g.github.com/Gaia-UI-Building-Blocks/index.html#menus_dialogs/value_selector/

header:
[e-mail]

with options:
- Reply (takes user to new message window with To: filled in with email address)
- Add to contacts (takes user to new contact window with email field pre-populated)


If you are replying to a message tapping on a contact will display a value selector:

header: 
[e-mail]

with options:
- delete (removes contact from To/Cc/Bcc field)

Comment 10

[Casey Yee]

Wireframes updated with details:
https://www.dropbox.com/s/vaf9d6ppbw58zi1/mail-main.pdf

Comment 11

[Steve Chung [:steveck]]

Hi Casey,
Thanks for the update, I have some question about how we display the sender's email address. Is there no need to show the sender's address for avoiding phishing?

Comment 12

[Andrew Sutherland [:asuth] (he/him)]

(In reply to Steve Chung from comment #11)
> Thanks for the update, I have some question about how we display the
> sender's email address. Is there no need to show the sender's address for
> avoiding phishing?

If you look at page 21 of the "Read message" specs at https://www.dropbox.com/sh/ygwfxk6chpshxdj/O60wN6xlEH/Apps/Email/mail-main.pdf, it calls for:
- Displaying the name of the sender of the message in the header.
- Displaying the e-mail address of the sender of the message in the bubble for the 'from' field.

That change should be made at the same time as we make the bubble change.

Comment 13

[Dietrich Ayala (:dietrich)]

Driver retriage: We're not holding the whole release for this bug. Would take a patch once all P1 issues are fixed.

Comment 14

[ayman maat :maat]

(In reply to Dietrich Ayala (:dietrich) from comment #13)
> Driver retriage: We're not holding the whole release for this bug. Would
> take a patch once all P1 issues are fixed.

agreed.

Comment 15

[Casey Yee]

Spec has been updated with details on how we should be handling the Recipient name in the sent folder.

See page. 5 of spec.
https://www.dropbox.com/s/vaf9d6ppbw58zi1/mail-main.pdf

The [name] field should show the recipients name.  If the name of recipient is not available we should show the email address.

If there is more than a single recipient in the email (multiple To, Cc or Bcc recipients) we should show [x] recipients.   The user can view the full recipient list in the email by tapping on the message to open.

Comment 16

[Andrew Sutherland [:asuth] (he/him)]

(In reply to Casey Yee [:cyee] from comment #15)
> Spec has been updated with details on how we should be handling the
> Recipient name in the sent folder.

I think this comment was meant to be made on bug 806599; I've copied the comment there.

I think the wireframes were already up-to-date for this bug's purposes, although the wireframes might have been slightly more updated.  Page 22 is a duplicate of page 22 with some extra stuff; page 21 should probably be removed.  And page 23 is still the bit relevant to this bug.

Comment 17

[Steve Chung [:steveck]]

I was trying to apply the action menu to fit the wireframe p.17. but style applied in custom dialog will affect the action menu and bust the layout. Since the menu style is also a key point for this patch, I prefer to clarify the issue in building-block first.

Hi Pavel,
Can we have more specify attribute (like data-type="custom") to define the custom dialog? It only use role="dialog" for the element and it will conflict with other widget for sure(We can use not([data-type="action"]) to avoid the conflict with action menu but it would be ugly if we add another dialog widget in the app...). BTW, we have custom_dialog.js but lack of custom_dialog.css in building-block. Will we have the general custom_dialog style in building-block? Or it won't exit because it is "custom"?

Comment 18

[Pavel Ivanov [:ivanovpavel][:pivanov] UX]

I think Ismael can give you more info about [BB]custom_dialog

Comment 19

[Ismael Gonzalez [:basiclines]]

Right now email application is not linking dialog CSS to our shared Building Blocks, the only ones linked are:

  <link rel="stylesheet" type="text/css" href="shared/style/headers.css"/>
  <link rel="stylesheet" type="text/css" href="shared/style/switches.css"/>
  <link rel="stylesheet" type="text/css" href="shared/style/buttons.css"/>
  <link rel="stylesheet" type="text/css" href="shared/style/status.css"/>

So any issue with selectors in the css files is not related with our BB.

The ideal way to fix it, is replacing custom css files that emulates the originals building block, in this way we are ensuring fully isolation of the dialogs.

Comment 20

[Steve Chung [:steveck]]

Attachment: Patch for new bubble display and other layout fixing

NOTE: If blocking-basecamp+ is set, just land it for now.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: 
Testing completed: 
Risk to taking this patch (and alternatives if risky):

Comment 21

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 702406 [details]
Patch for new bubble display and other layout fixing

This works really well, thanks very much for taking on this big change.  I did notice some bugs in my testing, but they exist before this patch and are not appropriate to fix here.

Vivien, I think this would be a very good change to take since our current popup stylings are very much ugly and inconsistent with elsewhere in the app and in other apps.  There also is a very significant screen real-estate win for the message reader since we don't have to show the e-mail address.

Comment 22

[Steve Chung [:steveck]]

Hi Andrew,
I added some fixing based on your comment: https://github.com/mozilla-b2g/gaia/pull/7625#r2754857, thanks.

Comment 23

[Alex Keybl [:akeybl]]

Please land to master and provide a risk evaluation for consideration for uplift to v1-train.

Comment 24

[Andrew Sutherland [:asuth] (he/him)]

pull request merged to gaia/master:
https://github.com/mozilla-b2g/gaia/pull/7625

v1-train uplift risk analysis:
Low risk from taking it, and this is what the interaction designs called for for v1.  I'm particularly concerned that if we don't take it that we are going to end up with extreme divergence between v1-train and v2 code and anything that we do get approval to uplift is going to have to be re-done from scratch for v1.

Comment 25

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 702406 [details]
Patch for new bubble display and other layout fixing

Our impression was that this patch was not appropriate for v1.0.1 but rather for v1.1 and forward. However v1.1 has not yet branched from gaia/master and so no further landings should be required in order to ride that train.

Please renominate if you disagree, or think this is incorrect.

Comment 26

[Lukas Blakk [:lsblakk] use ?needinfo]

Tracking-b2g18=20+ so that this gets caught in the cherry picking after v1.0.1 branching.

Comment 29

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Note to QA : 
Landed on master on 1/28 will be on 1.1 without anything else needed to be done;
It is not on 1.0.1 ie v1-train nor 1.0.0.

Comment 30

[Alex Keybl [:akeybl]]

Comment on attachment 702406 [details]
Patch for new bubble display and other layout fixing

Let's leave this on the approval nom list until v1-train is v1.1.

Comment 31

[Daniel Coloma:dcoloma]

Why are we postponing this to v1.1? based on comment 24 I think there is no strong reason for not taking it

Comment 32

[Alex Keybl [:akeybl]]

(In reply to Naoki Hirata :nhirata from comment #29)
> Note to QA : 
> Landed on master on 1/28 will be on 1.1 without anything else needed to be
> done;
> It is not on 1.0.1 ie v1-train nor 1.0.0.

This is not true, master will not become v1.1. Leo features will be uplifted from master to v1-train once v1-train is v1.1.

Comment 33

[Alex Keybl [:akeybl]]

(In reply to Daniel Coloma:dcoloma from comment #31)
> Why are we postponing this to v1.1? based on comment 24 I think there is no
> strong reason for not taking it

Comment 24 is making the case to land in the v1.x timeframe, not to land on v1.0.1 specifically. The risk profile here is not low, given the amount of code change.

Comment 34

[Daniel Coloma:dcoloma]

(In reply to Alex Keybl [:akeybl] from comment #33)
> (In reply to Daniel Coloma:dcoloma from comment #31)
> > Why are we postponing this to v1.1? based on comment 24 I think there is no
> > strong reason for not taking it
> 
> Comment 24 is making the case to land in the v1.x timeframe, not to land on
> v1.0.1 specifically. The risk profile here is not low, given the amount of
> code change.

comment 24 (from the app author) says the risk is low, and he also comments that the sooner it lands the better.

Comment 35

[Alex Keybl [:akeybl]]

We're only days away from the branch point. We wanted to get more bake time with dogfooders, thus holding off approval till v1.1.

Comment 36

[Andrew Sutherland [:asuth] (he/him)]

It turns out this patch was deeply required to land bug 798258 and bug 821683/bug 820317, so I landed it on the approval authority of bug 798258.

landed on gaia/v1-train:
https://github.com/mozilla-b2g/gaia/pull/8075
https://github.com/mozilla-b2g/gaia/commit/16c454ac7a805f30050818cd48af16561e40d6f3

Comment 37

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 702406 [details]
Patch for new bubble display and other layout fixing

just keeping good records then and approving this patch :)

Comment 38

[Lukas Blakk [:lsblakk] use ?needinfo]

Batch edit: bugs fixed on b2g18 since 1/25 branch of v1.0 are fixed on v1.0.1