934531 - [Messages] The Visual Design is incorrect for the contact card

Status: RESOLVED FIXED

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

Description

[Julien Wajsberg [:julienw]]

Description:
When the user taps on a header of a group sent MMS message, taps on a contact, they will notice that the context menu is incorrect.

The same context menu is displayed when the user taps on a header of a known contact's thread.

Repro Steps:
1)  Have a recent nightly
2)  Ensure that the user has a contact created on the phone that has a name and carrier type.
3)  Tap on the "Messaging" icon.
4)  Tap on the "Compose New Message" icon.
5)  Tap on the "+" icon and add the contact from step 2 to the "To:" field.
6)  Type a SMS.
7)  Press send.
8)  Tap on the header of the SMS message

See attachments.
(note: these repro steps is not a regression, however the repro steps in bug 924475 shows the same contact card and is a regression)

Comment 1

[Julien Wajsberg [:julienw]]

Attachment: latest visual spec

Comment 2

[Jason Smith [:jsmith]]

Confused. Is the keyword here right that this is a regression or not?

Comment 3

[Julien Wajsberg [:julienw]]

Attachment: current behaviour (after bug 924475 patch)

Comment 4

[Julien Wajsberg [:julienw]]

Yep Jason, this is a regression.

This is due to either a BB evolution or a change in the SMS app, but still, this is a regression.

We fixed a first issue that was occuring on 1.2 in bug 924475, but on 1.3 there is a different issue. One of the root cause is that we're outputting a markup that we should not (a <a> element), and that get styled as a <a> element.

A quick fix would be to overload that style, but I want to catch the opportunity to output a cleaner markup.

Comment 5

[Joe Cheng [:jcheng] (please needinfo)]

triage: would not block release, please land directly into master 1.3

Comment 6

[Julien Wajsberg [:julienw]]

This is a regression, we blocked bug 924475 for something similar (except in bug 924475 we also missed some information; but it was in a more obscure panel, whereas now it's in a panel displayed as soon as we tap the header of a known contact's thread).

Renominating.

Comment 7

[Julien Wajsberg [:julienw]]

Attachment: WIP patch v1

Hey Steve,

The patch is not completely finished yet, especially I'd like to add some comments to be more explicit about the properties templates can use.

However, I think the code is fairly complete, and everything seems to work on Firefox Desktop and Peak. I haven't tried on a non-HD phone yet, I'd like to see how it looks like for not long names.

But still I think you can already have a look at the code before the final patch where I'll ask you review.

Thanks!

Github PR is at https://github.com/mozilla-b2g/gaia/pull/13713

This changes how we render a contact in the Messages app.
---
 apps/sms/index.html                              |   46 ++-
 apps/sms/js/contact_renderer.js                  |  228 ++++++++++++
 apps/sms/js/desktop-only/contacts.js             |   84 +++--
 apps/sms/js/desktop-only/mobilemessage.js        |   20 +-
 apps/sms/js/desktop-only/photo-man-bowtie.jpg    |  Bin 0 -> 7046 bytes
 apps/sms/js/desktop-only/photo-man.jpg           |  Bin 0 -> 6769 bytes
 apps/sms/js/desktop-only/photo-woman.jpg         |  Bin 0 -> 7179 bytes
 apps/sms/js/startup.js                           |    1 +
 apps/sms/js/thread_ui.js                         |  243 ++-----------
 apps/sms/style/sms.css                           |   60 ++-
 apps/sms/test/unit/contact_renderer_test.js      |  394 ++++++++++++++++++++
 apps/sms/test/unit/mock_contact_renderer.js      |   14 +
 apps/sms/test/unit/thread_ui_integration_test.js |  130 +------
 apps/sms/test/unit/thread_ui_test.js             |  425 ++++------------------
 14 files changed, 893 insertions(+), 752 deletions(-)
 create mode 100644 apps/sms/js/contact_renderer.js
 create mode 100644 apps/sms/js/desktop-only/photo-man-bowtie.jpg
 create mode 100644 apps/sms/js/desktop-only/photo-man.jpg
 create mode 100644 apps/sms/js/desktop-only/photo-woman.jpg
 create mode 100644 apps/sms/test/unit/contact_renderer_test.js
 create mode 100644 apps/sms/test/unit/mock_contact_renderer.js

Comment 8

[Joe Cheng [:jcheng] (please needinfo)]

1.3+ as it blocks committed user story

Comment 9

[Steve Chung [:steveck]]

Comment on attachment 8333953 [details] [diff] [review]
WIP patch v1

Hi Julien, I've left some comments in the patch, might need another deep look when finished but I haven't discover seriours drawback yet. Thanks for the refactoring!

Since the contact rendering is related to bug 933131, which is 1.3 blocking issue now. I've suggested Joe to set this issue as 1.3 blocker.

One question is about the layout in option menu. You put the fancy header in the contact option menu. Did Ayman confrim this changes? It seems not a proper place to put contact in section(it should be options instead), but I'm not sure if we should put complete contact section in the header(and we didn't display the portrait in contact removal confirm prompt). I'll attach the image to UX for mare information later.

Comment 10

[Steve Chung [:steveck]]

Attachment: Screen Shot 2013-11-03 at 17.02.12.png

Hi Ayman, this is the screenshot from Julien's patch. Do you think it looks great if we add the contact portrait in the header, or we should just put text in the header?

Comment 11

[Julien Wajsberg [:julienw]]

Steve, please see https://bugzilla.mozilla.org/attachment.cgi?id=826849, this is the latest Visual Spec I got.

Maybe José or Victoria could confirm this.

Comment 12

[Steve Chung [:steveck]]

(In reply to Julien Wajsberg [:julienw] from comment #11)
> Steve, please see https://bugzilla.mozilla.org/attachment.cgi?id=826849,
> this is the latest Visual Spec I got.
> 
> Maybe José or Victoria could confirm this.

Oops, sorry I missed the visual spec in the front... I think the information is clear since vsual spec is right here.

Comment 13

[Julien Wajsberg [:julienw]]

To be fair, I got it from a quite old bug (2 months ago), it can make sense to still ask Visual Team if it's still current so that we don't have bugs filed later :)

Hey Victoria, José, could you please tell us if https://bugzilla.mozilla.org/attachment.cgi?id=826849 is still current for showing a prompt about a contact?

Thanks!

Comment 14

[José Vittone]

Attachment: Overlay with Photo - SPEC

Hi Julien, 

Thanks for asking. I checked it with Vicky, made a little adjustment and walla! Here is the last spec.

Comment 15

[Julien Wajsberg [:julienw]]

ok, so the right padding then :)
Will adjust tomorrow.

Comment 16

[José Vittone]

Julien, 

I misunderstand you, sorry about that. I thought that you were asking about the spec itself, not the implementation of it.

This is the actual implementation? https://bug934531.bugzilla.mozilla.org/attachment.cgi?id=8334407 
Can I see a screenshot from the phone of these? Or can I see it on my phone? It's kind of difficult to check the implemetation without the correct fonts.

Comment 17

[Julien Wajsberg [:julienw]]

Attachment: patch v2

Hey Steve,

here is the updated patch.

The pull request is updated too: https://github.com/mozilla-b2g/gaia/pull/13713
Note that I added the changes to the previous patch as an additional commit on the pull request.

I'm still pending Arnau's feedback on the building blocks issue, but we can probably start reviewing. I won't merge without his feedback here anyway.

Thanks!

Comment 18

[Julien Wajsberg [:julienw]]

(In reply to José Vittone from comment #16)
> Julien, 
> 
> I misunderstand you, sorry about that. I thought that you were asking about
> the spec itself, not the implementation of it.

That's exactly what I was asking :)

> 
> This is the actual implementation?
> https://bug934531.bugzilla.mozilla.org/attachment.cgi?id=8334407 

This is the implementation of the first patch, but not on a device.

> Can I see a screenshot from the phone of these? Or can I see it on my phone?
> It's kind of difficult to check the implemetation without the correct fonts.

I'll do a screenshot on the device right now and ask you feedback on it.

Comment 19

[Julien Wajsberg [:julienw]]

Attachment: contact with photo and no phone number type

Here is the contact card for a contact with a photo, a phone number without the carrier information, but with a type information.

Comment 20

[Julien Wajsberg [:julienw]]

Comment on attachment 8335199 [details]
contact with photo and no phone number type

Sorry, this one has _no_ phone number type (ie: a custom empty one). This does not happen a lot on our real phones when using the Contacts app, but I have this contact on my reference workload.

Comment 21

[Julien Wajsberg [:julienw]]

Attachment: Contact with photo, a phone number type, and a long name

Comment 22

[Julien Wajsberg [:julienw]]

Attachment: No contact, only a plain number

Comment 23

[Julien Wajsberg [:julienw]]

Attachment: Contact without a photo, with carrier and type

Comment 24

[Julien Wajsberg [:julienw]]

Attachment: contact with a long name without a photo

Comment 25

[Julien Wajsberg [:julienw]]

Hey José,

I uploaded all screenshots taken on a Peak. I can do some from a non-HD device too, but only later today as I'm not in the office right now.

Comment 26

[José Vittone]

Hey Julien, 

Thanks a lot! I'll check them out.

Comment 27

[Julien Wajsberg [:julienw]]

bug 925929 just landed, I'll check today how this affects my work here.

Comment 28

[José Vittone]

Hey Julien,

General comments on this:

- The image should be 6x6rem (sorry, it is not in the spec)
- Fonts on header (.name & .details) must be light
- Spacing tweaks (shown in comparison)
- When there is no contact (only a plain number) should be styled as .name (font-size 2.3rem and light)

Arnau (who is next to me) told me "Joan will create a patch for your pull request with these changes". Is that OK?

Comment 29

[José Vittone]

Attachment: Visual review - Card with Photo

Comment 30

[José Vittone]

Attachment: Visual review - Card without Photo

Comment 31

[José Vittone]

Attachment: Visual review - Card not contact

Comment 32

[Julien Wajsberg [:julienw]]

follow-up to comment 27: the landing of bug 925929 doesn't affect us here.

José, about the image size, I thought like you that it should be 6x6rem, the problem is that when I add everything, it didn't match correctly. Maybe it will be ok once the spacing tweaks are done, but then maybe the spec is not good?

You can tell Arnau there is a question for him on the pull request too ;)

Comment 33

[Arnau March [:arnau] ( not working in Firefox OS anymore :( )]

Julien, Joan who works with me, could create a patch to your PR to fine-tune css values to fit latest visuals.

Comment 34

[José Vittone]

Hey Julien, 

The numbers in the spec are not that precise, sorry about that. I'm not going to blame anybody more than me, next time I'll double check them. 

I can update the Spec if you consider necessary for other reasons (like QA), just tell me.

Comment 35

[Julien Wajsberg [:julienw]]

Hey José,

Yes please, an updated spec would be definitely desirable, for future reference :)

Arnau, if I have an updated spec, I can fine-tune myself, but I would also appreciate a PR if she has the time :)

Arnau (again), I had a question for you on the PR, Basically, it's about the code at [1], I think it could be done in the BB directly. If that sounds good to you, I can change the BB in my pull request and ask a review from you.

[1] https://github.com/mozilla-b2g/gaia/pull/13713/files#diff-40ff32ff8ddce22c88b864f33e07f312R873

(Flagging Arnau and José)

Comment 36

[Julien Wajsberg [:julienw]]

Comment on attachment 8335197 [details] [diff] [review]
patch v2

Removing review flag until I have an updated spec.

Steve, if it's still not ok on Monday, I may split this bug in half: the refactoring in another bug that we'll be able to land, and the fine visual changes here, so that you're not blocked.

Comment 37

[Steve Chung [:steveck]]

(In reply to Julien Wajsberg [:julienw] from comment #36)
> Comment on attachment 8335197 [details] [diff] [review]
> patch v2
> 
> Removing review flag until I have an updated spec.
> 
> Steve, if it's still not ok on Monday, I may split this bug in half: the
> refactoring in another bug that we'll be able to land, and the fine visual
> changes here, so that you're not blocked. 

I can still take a review first and land it if possible. We can create a follow-up bug for any visual changes in the future.

Comment 38

[Arnau March [:arnau] ( not working in Firefox OS anymore :( )]

If you want to split the patch, Joan can update the visuals later.
So we are not delaying you before closing 1.3

Comment 39

[José Vittone]

Attachment: SPEC

The measurement are now correct. Also added more info about the font styles.

The way you implement them can vary as you want, for example use more padding rather than modifying the line-height. In this particular case,line-height value is not a must have, since there are only single lines and not paragraphs.

Sorry for the inconvenience again.

Comment 40

[Julien Wajsberg [:julienw]]

Attachment: patch v3

Hey Steve,

I want to move forward here so I filed bug 943165 for making the "number/not a contact" case right. The "contact" case should be fine with this patch.

I think I fixed all your comments :)

I added a follow-up commit to the PR at https://github.com/mozilla-b2g/gaia/pull/13713 for an easy review and the patch here contains all the changes.

Comment 41

[Steve Chung [:steveck]]

Comment on attachment 8338173 [details] [diff] [review]
patch v3

Review of attachment 8338173 [details] [diff] [review]:
-----------------------------------------------------------------

It's looks great, thanks for the refactoring!

Comment 42

[Joan Leon [:joan]]

Attachment: sms-fix-headers.patch

Patch to update visual styles in action menu headers

Comment 43

[Joan Leon [:joan]]

Julien, have a look to my patch. You could apply it to your branch 934531-fix-contact-card to fix the headers.

Comment 44

[Julien Wajsberg [:julienw]]

Thanks Joan, I'll definitely use it in bug 943165!
I'd like to see if I could not inherit a little more from the BB because redefining everything in sms.css seems counter-productive.

Comment 45

[Julien Wajsberg [:julienw]]

master: 62009adb592cfb3bd68a06318c5d01552ede79e7