840055 - [MMS][User Story] Message thread view

Status: RESOLVED FIXED

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

Description

[Peter Dolanjski [:pdol]]

UCID: Messages-052

User Story:
As a user, I want to view messages to/from the same recipient(s) in a thread to avoid having to open multiple messages individually.

Comment 1

[Brad Lassey [:blassey] (use needinfo?)]

The assumption is that this already works with the implementation of SMS, assigning to Bocoup to confirm.

Comment 2

[Casey Yee]

Flagging Peter for NeedsInfo to confirm that the current implementation fulfills this requirement.

Comment 3

[Peter Dolanjski [:pdol]]

(In reply to Casey Yee [:cyee] from comment #2)
> Flagging Peter for NeedsInfo to confirm that the current implementation
> fulfills this requirement.

It does.  This requirement is basically in place to ensure that this continues to be the case with MMS.

Comment 4

[Borja Salguero [:borjasalguero]]

Same as in other bugs. This it's working and should be working with MMS integration. No 'real bug', no 'new feature' here. We should close all these bugs in order to know which is the real work to be done in the following weeks.

Comment 5

[Peter Dolanjski [:pdol]]

(In reply to Borja Salguero [:borjasalguero] from comment #4)
> Same as in other bugs. This it's working and should be working with MMS
> integration. No 'real bug', no 'new feature' here. We should close all these
> bugs in order to know which is the real work to be done in the following
> weeks.

I'd prefer to verify that this works with MMS first (although it is the same app, this should be a quick verification once MMS is working).

Comment 6

[Dietrich Ayala (:dietrich)]

Per partner and release-driver discussions, marking blocking- until all MMS functionality in bug 849867 is complete, allowing it all to be uplifted at once to avoid SMS bustage.

Comment 7

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

leo+ as this is a part of MMS and part of v1.1 to be included in leo+ queries. No_UPLIFT for now before the whole MMS is completed

Comment 8

[Paul Yang [:pyang] (away)]

moztrap #6542

Comment 9

[Steve Chung [:steveck]]

Hi Cassie,
Do you have any progress on this issue? The wireframe have some layout changes recently, so we might reassign to other people in the work week. If you already some progress in this feature, please let me know, thanks.

Comment 10

[cassie]

This is one we categorized as to be checked when MMS is complete, so it has not been worked on and can be reassigned.

Comment 11

[Steve Chung [:steveck]]

Attachment: New layout for message thread view with mms message

This visual might have some problem in mms message layout, but the new style for message bubble is confirmed.

Comment 12

[Victoria Gerchinhoren [:vicky]]

Hi all,

An interesting portion of the new visual work for the MMS/SS is done and you can find it in the following link below. Note that you can also find the specs for the mark up inside, with font sizes and colors, spacing, etc. This last can be found inside the folder "_Specs".

https://www.dropbox.com/sh/7ky6j0wn81l7cl5/q8yfFz3BEY

If you have any question, please do not hesitate.

Victoria

Comment 13

[Steve Chung [:steveck]]

Hi Ayman/ Victoria,
I have a concern in thread view's edit mode. How will we handle the checkbox in this view : https://www.dropbox.com/sh/8jkb0osnvfda96e/TkcQuTbhHj#f:02.%20SMS.MMS_0001_Thread1.png ?
It seems we don't have the sufficient space for checkbox in the new layout. Even we set limitation for the bubble width, it's still weird that we only set the checkbox inside the receiver bubble but outside the sender bubble. Could you please confirm the layout? Thanks.

Comment 14

[Victoria Gerchinhoren [:vicky]]

(In reply to Steve Chung from comment #11)
> Created attachment 738029 [details]
> New  layout for message thread view with mms message
> 
> This visual might have some problem in mms message layout, but the new style
> for message bubble is confirmed.

HI Steve, 
The attachment you're creating is obsolete, please grab the designs from the linke I provided above.

Comment 15

[Victoria Gerchinhoren [:vicky]]

(In reply to Steve Chung from comment #13)
> Hi Ayman/ Victoria,
> I have a concern in thread view's edit mode. How will we handle the checkbox
> in this view :
> https://www.dropbox.com/sh/8jkb0osnvfda96e/TkcQuTbhHj#f:02.%20SMS.
> MMS_0001_Thread1.png ?
> It seems we don't have the sufficient space for checkbox in the new layout.
> Even we set limitation for the bubble width, it's still weird that we only
> set the checkbox inside the receiver bubble but outside the sender bubble.
> Could you please confirm the layout? Thanks.

Hi Steve,

I am working on the visuals for the edit mode. They're not ready yet, but it will be solved. As it happens all though the system it may mean that the text boxes will be moved a bit. I am working on the solution, but, are you stucked with this right now?

Comment 16

[Steve Chung [:steveck]]

Hi Victoric,
The structure of the thread view could be way different depend on the different edit mode layout. Do you have a image to illustrate the brief idea about the new edit mode? That would be great help because we want to land the new thread view layout this week.

Comment 17

[Victoria Gerchinhoren [:vicky]]

Hi Steve,
Attaching the edit mode layout. We can discuss it if ypu want over skype: @vixvixy

Thanks!

Comment 18

[Victoria Gerchinhoren [:vicky]]

Attachment: Shows the visual solution for the edit mode

Comment 19

[Mike Pennisi [:jugglinmike]]

Attachment: Update Thread List layout

Comment 21

[Julien Wajsberg [:julienw]]

Comment on attachment 744427 [details] [diff] [review]
Update Thread List layout

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

generic comment: could you try to order your css properties according to https://github.com/necolas/idiomatic-css ? with empty lines between the various groups ?

the switch to "edit" mode is not very nice, I don't see the transition happening at all, so something must be wrong around this.

::: apps/sms/style/sms.css
@@ +80,5 @@
>    opacity: 0;
>    display: block;
>    overflow: hidden;
>    right: 5rem;
> +  transform: translateX(0);

nit: do we need that transform ? Isn't that the default ?

Comment 22

[Julien Wajsberg [:julienw]]

the error handling seems to be broken too, I think you need to change which classes you add to which elements.

How to reproduce :
* enable the sim pin security 
* enable flight mode
* try to send a sms
=> we sohuld have the error flag + the alert box saying that we are in flight mode

Another STR :
* enable the sim pin security 
* enable flight mode
* disable flight mode
* _not_ enter the pin code
* try to send a sms
=> we should have the error flag

none of these str work with your patch.

Comment 23

[Mike Pennisi [:jugglinmike]]

Attachment: WIP: Incorporate Julien's feedback

In fixing the bug identified by Julien, I felt I should add some unit tests to prevent regressions. These tests are tightly-coupled to the DOM and tend to decrease the maintainability of the code base.

This reflects a larger problem with the structure of the application logic, so I intend to re-factor that code. This patch should not be considered for merge; I am only uploading it to track my progress. Expect a fully-squashed review-ready patch later today.

Comment 24

[Mike Pennisi [:jugglinmike]]

Attachment: New layout for message thread view with mms message v3

Comment 25

[Mike Pennisi [:jugglinmike]]

Hi Julien,

Thanks for the review! Improving this patch lead lead to a "cascade" of
changes, so I wanted to describe everything in detail.

## Templating

I've refactored the message markup generation to use an HTML template. My
reasoning is pretty standard:

- It's more performant (just setting `innerHTML` versus manually attaching DOM
  nodes
- It's more maintainable (changing the markup does not require writing
  JavaScript)

## CSS and Structure Modifications

I've also re-factored the structure and CSS. Previously, the message container
was labeled with the `bubble` CSS class, despite the fact that its child was
the bubble.

I've also changed the "bubble" tag to be a "section" tag, because the parsing
logic behind `innerHTML` will fail when attempting to generate invalid markup.

Instead of programatically building the markup based on the message, the markup
remains (relatively) constant for every message, and CSS classes control how
the message is rendered.

In addition to being more maintainable, it limits the scope of bugs: note how
`onMessageSent` and `onMessageFailed` no longer depend on DOM structure created
elsewhere.

> the error handling seems to be broken too, I think you need to change which
> classes you add to which elements.

This prevents regressions along these lines. All the same, I added a few simple
tests to ensure that the correct classes are added at the correct time.

> generic comment: could you try to order your css properties according to
> https://github.com/necolas/idiomatic-css ? with empty lines between the
> various groups ?

Done! I've tried different CSS style guides in the past, but I never found an
approach that I like. I have to say, I like this organization for CSS
attributes. Simple enough to remember, but still adds value. Thanks for the
recommendation!

> >    opacity: 0;
> >    display: block;
> >    overflow: hidden;
> >    right: 5rem;
> > +  transform: translateX(0);
> 
> nit: do we need that transform ? Isn't that the default ?

I prefer explicit over implicit for things like animations, but you're right:
we don't need this. I've removed it.

## Animations

> the switch to "edit" mode is not very nice, I don't see the transition
> happening at all, so something must be wrong around this.

I worked hard with my initial patch to ensure that animations were functioning
correctly (indeed, I fixed an unreported bug: messages did not properly animate
when leaving "edit" mode). I believe that you were not seeing animations
because of hardware limitations on the Unagi--there is a limit to how many
CSS animations can be correctly displayed at once. (This limitation was no
doubt forced by the previous bug, where all messages were rendered in the
"Sending" state, meaning there were a likely a large number of "spinner"
animations active during your review.)

Comment 26

[Julien Wajsberg [:julienw]]

Comment on attachment 745436 [details] [diff] [review]
New layout for message thread view with mms message v3

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

overall these are very good changes.

Still some work to do though :)

::: apps/sms/index.html
@@ +183,5 @@
> +      <section class="bubble">
> +        <aside class="pack-end">
> +          <progress></progress>
> +        </aside>
> +        <p>${body}</p>

I'd prefer to use `bodyHTML` as we did in other templates, to make it clear we'll have markup here

::: apps/sms/js/thread_ui.js
@@ +790,5 @@
> +    var classNames = ['message', message.type, delivery];
> +    if (delivery === 'received') {
> +      classNames.push('incoming');
> +    } else {
> +      classNames.push('outgoing');

nit: looks like a good location to use the ternary operator ;)

@@ +811,5 @@
>        ThreadUI.addResendHandler(message, messageDOM);
>      }
>  
>      var pElement = messageDOM.querySelector('p');
> +    if (message.type === 'mms') { // MMS

I think we checked for "message.type" here because that avoids some warning in Firefox' console

@@ +1106,2 @@
>      // Check if it was painted as 'error' before
> +    if (!messageDOM.classList.contains('sending')) {

If I follow that comment, probably we want to check if it contains 'error' here. Previously we didn't use any "error" class so that's why...

@@ +1110,3 @@
>  
> +    // Update class names to reflect message state
> +    messageDOM.classList.remove('sending');

we probably want to remove 'sending' anyway, before the "if", if you change that if condition.

::: apps/sms/style/sms.css
@@ +328,4 @@
>  }
>  
> +#messages-container[data-type="list"] .message.outgoing {
> +  transition: padding-left 0.3s ease 0s;

I _think_ we never change padding-left on the outgoing messages, so this transition should be removed anyway.

@@ +335,5 @@
> +  float: right;
> +
> +  /* Prevent text content from rendering underneath the absolutely-positioned
> +   * aside.
> +   * TODO: Render the aside within the layout using flexbox. */

I doubt we'll ever use flexbox here, or it's in a long time. So I think this TODO comment is useless.

@@ +348,5 @@
> +  margin-right: 2rem;
> +  padding-left: 2rem;
> +
> +  background: #FCC987;
> +  transition: padding-left 0.3s ease 0s;

don't transition on padding, only transition on transformations, so that they're run on the GPU. as a matter of fact, we don't see that animation at all on the device.

I think the correct way to implement this, then, would be that the incoming messages' normal state is to be translated to the left, and their "edit mode" state is to be translated back to 0.

If I'm not wrong, though, the outgoing padding-left is never changed, so this transition here should be removed anyway. (but not the one on incoming obviously)

::: apps/sms/test/unit/mock_utils.js
@@ +13,5 @@
>    getDayDate: Utils.getDayDate,
>    getFormattedHour: Utils.getFormattedHour,
> +  getHeaderDate: Utils.getHeaderDate,
> +  getContactDetails: Utils.getContactDetails,
> +  getResizedImgBlob: Utils.getResizedImgBlob,

we don't want to take the real functions for these ones, just make them return something sensible, or something that you can set from the test, and if you want to test that they were called, set a local boolean.

see for example [1] for an example of a simple working mock.

[1] https://github.com/mozilla-b2g/gaia/blob/master/apps/system/test/unit/mock_system_banner.js

::: apps/sms/test/unit/thread_ui_test.js
@@ +3,5 @@
>  // remove this when https://github.com/visionmedia/mocha/issues/819 is merged in
>  // mocha and when we have that new mocha in test agent
>  mocha.setup({ globals: ['alert'] });
>  
> +require('/shared/test/unit/load_body_html_helper.js');

note that some other current PR will do this too, and put it in setup.js instead so that it's always loaded in all test files.

They also replace the DOM setup done in `setup` by a loadBodyHTML so it's probably sensible to do the same so that it will be easy to rebase regardless which patch will land first.

see eg https://github.com/mozilla-b2g/gaia/pull/9515/files#diff-9

@@ +325,5 @@
>        });
>      });
>    });
> +
> +  suite('message status update handlers', function() {

use angle brackets at the end of the suite names, we try to be consistent inside a test file.

For me that makes it easier to find a failing test.

@@ +341,5 @@
> +      document.body.innerHTML = this.prevBody;
> +    });
> +    setup(function() {
> +      this.container.className = '';
> +      this.container.innerHTML = ThreadUI.tmpl.message.interpolate({});

my first reaction was : just put something sensible here instead of using Template.

But as I understand, if we want to change the markup later on, you want that we change it only at one place. So ok.

@@ +373,5 @@
> +        });
> +      });
> +      suite('messages that were *not* previously in the "sending" state',
> +        function() {
> +        test('does not add the "error" class to the message element',

actually, what we want to test here, is that the resendHandler is not set twice.

note that this resendHandler could be rewritten using event delegation, now that we have the "error" class. This would let you remove this test (probably add more), and the resendHandler would be set only once.

Comment 27

[Victoria Gerchinhoren [:vicky]]

Hey Mike,
Can you place a screenshot of the Thread view mark up status? So I can review it and comment if anything needs polish before it lands.

Thanks!

Comment 28

[Mike Pennisi [:jugglinmike]]

> @@ +811,5 @@
> >        ThreadUI.addResendHandler(message, messageDOM);
> >      }
> >  
> >      var pElement = messageDOM.querySelector('p');
> > +    if (message.type === 'mms') { // MMS
> 
> I think we checked for "message.type" here because that avoids some warning in Firefox' console

As it is written, this is technically correct JavaScript, and conforming to Firefox's suggestion offers little benefit. If we're just trying to avoid console spam, I think the appropriate solution is to disable these warnings in `about:config` (search for "strict"). I consider workarounds to suppress warnings in Firefox to be implementation details that do not belong in the source code.

> @@ +373,5 @@
> > +        });
> > +      });
> > +      suite('messages that were *not* previously in the "sending" state',
> > +        function() {
> > +        test('does not add the "error" class to the message element',
> 
> actually, what we want to test here, is that the resendHandler is not set twice.
> 
> note that this resendHandler could be rewritten using event delegation, now
> that we have the "error" class. This would let you remove this test (probably
> add more), and the resendHandler would be set only once.

This sounds good to me, but implementing this will be non-trivial (likely utilizing a WeakMap in order to support retrieving messages by ID). Since others are waiting for this patch to land, I would like to open a separate bug for this re-factoring. Does this sound okay to you?

Comment 29

[Mike Pennisi [:jugglinmike]]

Hi Victoria,

Some of Julien's feedback includes UI tweaks, so I will attach a screenshot when I submit the latest version of this patch.

Comment 30

[Julien Wajsberg [:julienw]]

(In reply to Mike Pennisi [:jugglinmike] from comment #28)
> > @@ +811,5 @@
> > >        ThreadUI.addResendHandler(message, messageDOM);
> > >      }
> > >  
> > >      var pElement = messageDOM.querySelector('p');
> > > +    if (message.type === 'mms') { // MMS
> > 
> > I think we checked for "message.type" here because that avoids some warning in Firefox' console
> 
> As it is written, this is technically correct JavaScript, and conforming to
> Firefox's suggestion offers little benefit. If we're just trying to avoid
> console spam, I think the appropriate solution is to disable these warnings
> in `about:config` (search for "strict"). I consider workarounds to suppress
> warnings in Firefox to be implementation details that do not belong in the
> source code.

I think that was Rick's idea at the time, I personnaly don't really care. I was just explaining why this code was there.

> 
> > @@ +373,5 @@
> > > +        });
> > > +      });
> > > +      suite('messages that were *not* previously in the "sending" state',
> > > +        function() {
> > > +        test('does not add the "error" class to the message element',
> > 
> > actually, what we want to test here, is that the resendHandler is not set twice.
> > 
> > note that this resendHandler could be rewritten using event delegation, now
> > that we have the "error" class. This would let you remove this test (probably
> > add more), and the resendHandler would be set only once.
> 
> This sounds good to me, but implementing this will be non-trivial (likely
> utilizing a WeakMap in order to support retrieving messages by ID). Since
> others are waiting for this patch to land, I would like to open a separate
> bug for this re-factoring. Does this sound okay to you?

I'm quite sure you can do that without Weakmaps ;)

* you can get the message id from the dom id, or from a dataset
* you can get the message from the id with the mozSms API

=> win, no big refactoring., means you can do that easily now.
If you think it's too much work though, please file a bug

Comment 31

[Borja Salguero [:borjasalguero]]

Mike, your patch looks great! There are only last tweaks from UX pending, but Im pretty sure that we are going to have this landed soon ;)

Comment 32

[Mike Pennisi [:jugglinmike]]

Attachment: New layout for message thread view with mms message v4

As we discussed on Skype, I've disabled transitions for edit mode altogether (in order to avoid the performance cost of animating reflows). There is a larger problem with allowing individual messages to expand and contract vertically, but this will be tracked in a forthcoming bug.

After talking with Ayman, we've identified two additional UI bugs in the previous patch (relating to the width and padding of message containers). I've included fixes for those here, as well.

> ::: apps/sms/test/unit/mock_utils.js
> @@ +13,5 @@
> >    getDayDate: Utils.getDayDate,
> >    getFormattedHour: Utils.getFormattedHour,
> > +  getHeaderDate: Utils.getHeaderDate,
> > +  getContactDetails: Utils.getContactDetails,
> > +  getResizedImgBlob: Utils.getResizedImgBlob,
> 
> we don't want to take the real functions for these
> ones, just make them return something sensible, or
> something that you can set from the test, and if you
> want to test that they were called, set a local
> boolean.

It turns out these weren't strictly necessary--I had originally included them to solve symptoms of a different bug.

Comment 33

[Mike Pennisi [:jugglinmike]]

Attachment: New layout for message thread view with mms message v4

This version addresses a linting error in the previous patch's desktop mock file.

Comment 34

[Victoria Gerchinhoren [:vicky]]

We have discussed with Mike the problem when switching to edit mode. So I have made some adjustments to the thread view layout in order to give room to the check boxes in the edit mode and avoid this complications. This changes should be incorporated before landing. Thanks!

Comment 35

[Mike Pennisi [:jugglinmike]]

(In reply to Victoria Gerchinhoren from comment #27)
> Hey Mike,
> Can you place a screenshot of the Thread view mark up status? So I can
> review it and comment if anything needs polish before it lands.
> 
> Thanks!

Since we're updating the design, Victoria and I have agreed the work-in-progress screenshot is no longer necessary.

Comment 36

[Julien Wajsberg [:julienw]]

Comment on attachment 746119 [details] [diff] [review]
New layout for message thread view with mms message v4

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

r=me with the following comments :

* small changes to be done in the tests (see below)
* there is a small alignment problem in edit mode for one-line messages: the checkbox should be higher by a few pixels

(this means that when you fix this (and only this) you can land on bocoup's branch without another review. of course, be extra careful to the linter, that the tests passes, and that it works on the device, before landing)

::: apps/sms/test/unit/thread_ui_test.js
@@ +326,5 @@
> +
> +  suite('message status update handlers >', function() {
> +    suiteSetup(function() {
> +      this.prevBody = document.body.innerHTML;
> +      loadBodyHTML('/index.html');

could you please put this in the outermost `setup` and replace the current DOM setup ?

you can completely steal this part from my PR in https://github.com/mozilla-b2g/gaia/pull/9515

@@ +329,5 @@
> +      this.prevBody = document.body.innerHTML;
> +      loadBodyHTML('/index.html');
> +      this.fakeMessage = {
> +        id: 24601
> +      };

this can probably stay in suiteSetup

@@ +332,5 @@
> +        id: 24601
> +      };
> +      this.container = document.createElement('div');
> +      this.container.id = 'message-' + this.fakeMessage.id;
> +      document.body.appendChild(this.container);

this must be in setup

@@ +335,5 @@
> +      this.container.id = 'message-' + this.fakeMessage.id;
> +      document.body.appendChild(this.container);
> +    });
> +    suiteTeardown(function() {
> +      document.body.innerHTML = this.prevBody;

must be in teardown, and just remove the container instead of putting back the old body.

Comment 37

[Mike Pennisi [:jugglinmike]]

Attachment: New layout for message thread view with mms message v5

This version addresses Julien's suggestions in comment 36. Carrying forward "r+"

I've also submitting this via GitHub: https://github.com/mozilla-b2g/gaia/pull/9598

Comment 38

[Mike Pennisi [:jugglinmike]]

Attachment: New layout for message thread view with mms message v6

This version addresses the UI deficiency we identified yesterday. Although Julien has already approved an earlier version of this patch, this version includes non-trivial layout changes. Because of this, I am requesting an additional review from Borja.

Comment 39

[Mike Pennisi [:jugglinmike]]

Comment on attachment 746615 [details] [diff] [review]
New layout for message thread view with mms message v6

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

Bug 840057 is about to land in master, and it will conflict with the changes here. I will rebase and resolve the conflicts once that patch has been landed.

Comment 40

[Mike Pennisi [:jugglinmike]]

Attachment: New layout for message thread view with mms message v7

This version of the patch resolves the conflicts from recent changes to master. Additionally, it incorporates feedback from Victoria regarding minor UI tweaks.

Comment 41

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

v1-train: 9d452e6

Comment 42

[mdaniloff]

Verified fixed on Leo

Environmental Variables
Build ID: 20130806071254
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18/rev/a2a9b89ef5ee
Gaia: 4c1a20570e20f64782ba170c14604395c48f7381
Platform Version: 18.1

Message threads appear and function as intended