884890 - [MMS][Multirecipient] Use CSS and touch events for handling the recipients container pull&push effects

Status: RESOLVED FIXED

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

Description

[Julien Wajsberg [:julienw]]

Attachment: before change screenshot

see screenshot

I think the top space is too big when we scroll the recipients down. Moreover if it was a little higher, I think the 3rd line would be entirely displayed.

I understand the margin is there so that when the recipients editor is "closed down", we still can see the bottom of the recipients that are higher. However, we may remove that margin (or rather padding) when the recipients editor is opened, so that we could see 3 full lines, and remove that disgraceful space.

I've found no visual design about this specific issue.

I also think this margin could be removed when there is only one line of recipients, but this is less disgraceful when the editor is smaller.

needinfo on Victoria about this.

Comment 1

[Rick Waldron [:rwaldron]]

Attachment: Github Pull Request https://github.com/mozilla-b2g/gaia/pull/10493

Comment 2

[Rick Waldron [:rwaldron]]

Visual output https://www.dropbox.com/sh/luf7o1nyy4ozmre/bRn9bMEUIR#/

Comment 3

[Julien Wajsberg [:julienw]]

waiting for Victoria's input before reviewing.

Comment 4

[Rick Waldron [:rwaldron]]

The change is one line and I've included screen shots of the resulting UI.

Comment 5

[Victoria Gerchinhoren [:vicky]]

Hi Julien, Rick,

The solution for the particular issue of starting the first recipient higher is good, I think looks much better. 

But I see a bigger bug here that it's not solved, and this is that the recipients field should take the maximum area available to show recipients before scrolling. In the screenshots I see a gap between the recipients field and the input text area despite there are more hidden recipients. The bug is: https://bugzilla.mozilla.org/show_bug.cgi?id=870416

Thanks!

Comment 6

[Julien Wajsberg [:julienw]]

Attachment: after change one-line no recipient block screenshot

Comment 7

[Julien Wajsberg [:julienw]]

Attachment: after change one-line with recipient block screenshot

Comment 8

[Julien Wajsberg [:julienw]]

see the 2 new screenshots, I think it's not good enough, maybe the "to:" label should be higher too. We see this on your 3-line screenshot too.

Victoria ?

Comment 9

[Victoria Gerchinhoren [:vicky]]

Hi Julien, 
Please follow the original specs. 
The position of the "To" word is though to fit well in the case when the to field is collapsed and the recipients occupy more than one line, suggesting this can be pulled off and reveal the rest of recipients.

So this is correct: https://bug884890.bugzilla.mozilla.org/attachment.cgi?id=765877
And this : https://bug884890.bugzilla.mozilla.org/attachment.cgi?id=765875 the input text's baseline there should line up with the baseline of the "TO" word.

Comment 10

[Julien Wajsberg [:julienw]]

Thanks a lot Victoria ! I haven't seen your previous comment ;)

Comment 11

[Rick Waldron [:rwaldron]]

(In reply to Victoria Gerchinhoren from comment #5)
> Hi Julien, Rick,
> 
> The solution for the particular issue of starting the first recipient higher
> is good, I think looks much better. 
> 
> But I see a bigger bug here that it's not solved, and this is that the
> recipients field should take the maximum area available to show recipients
> before scrolling. In the screenshots I see a gap between the recipients
> field and the input text area despite there are more hidden recipients. The
> bug is: https://bugzilla.mozilla.org/show_bug.cgi?id=870416

Bug 870416 has a patch pending review.

Comment 12

[Rick Waldron [:rwaldron]]

From what I can tell, the screenshots of the changes are correct? 

FWIW, Neither of them illustrates the actual fix this bug addresses.

Comment 13

[Julien Wajsberg [:julienw]]

Rick, the screenshots clearly illustrate changes that the fix makes ;) You can see easily with the "j" character that without your patch, it's properly aligned, whereas with your patch, it's slightly up. That's what Victoria is confirming in comment 9.

I don't know if that's fixable using only CSS, but maybe you can ask Mike for some help if necessary.

Comment 14

[Rick Waldron [:rwaldron]]

Glad someone can tell, I sure can't. Either way, it's a shame about that j, someone else should take it from here.

Comment 15

[Julien Wajsberg [:julienw]]

Mike, do you feel like tackling this ?

Comment 16

[Mike Pennisi [:jugglinmike]]

Based on Comment 11 (and after confirming with Julien on IRC), I'm marking this bug as dependent on Bug 870416. Once that lands, I'll get to work on a new patch.

Comment 17

[Julien Wajsberg [:julienw]]

Comment on attachment 764911 [details] [review]
Github Pull Request https://github.com/mozilla-b2g/gaia/pull/10493

removing the r request in the mean time

Comment 18

[Mike Pennisi [:jugglinmike]]

Attachment: Correct participants offset - wip

This patch is not ready for review (see the bug described in the commit message). I'm uploading it to track my progress

Comment 19

[Borja Salguero [:borjasalguero]]

As we know this should not block leo, but JS logic for the recipients-panel is dangerous (we are adding a lot of JS, and should be CSS instead with only a little bit of JS tracking the touch events), so I would like to see this fixed and working as it's defined in the wireframes & visual. Nominating to koi?

Comment 20

[Julien Wajsberg [:julienw]]

The bug title is probably wrong now that we're actually opening the recipients panel in the available space.

Comment 21

[Borja Salguero [:borjasalguero]]

Julien, in master this is not working as expected (we need only to take the space needed by the recipients, and the max-height is the max available in the screen without the header). Furthermore the touch events are not working as expected (you should be able to pull/push without pulling the entire panel down) etc.... a lot of things to fix, but we could wait till KOI

Comment 22

[Julien Wajsberg [:julienw]]

(In reply to Borja Salguero [:borjasalguero] from comment #21)
> Julien, in master this is not working as expected (we need only to take the
> space needed by the recipients,

Mmm I checked the spec lately (because it looked wrong to me as well) and it seemed to be what's implemented right now, but I may be wrong.

Just checked it again and it's not clear enough: it's not explained what should be done if the recipients list does not fill all the available space (slide 19, 6th panel).


Anyway, then this bug should be retitled and probably Mike's patch should be obsolete, right ?

Comment 23

[Borja Salguero [:borjasalguero]]

I think so. However this is one of the things that we could discuss with Ayman in Paris during the WW, and have a clear idea about the things to be implemented.

Comment 24

[ayman maat :maat]

ok lets discuses during WW Paris. in the mean time i wont do anything with this.
Julien, Borja - let me know if that is not ok for you.

Comment 25

[Rick Waldron [:rwaldron]]

(In reply to Borja Salguero [:borjasalguero] from comment #19)
> As we know this should not block leo, but JS logic for the recipients-panel
> is dangerous (we are adding a lot of JS, and should be CSS instead with only
> a little bit of JS tracking the touch events), so I would like to see this
> fixed and working as it's defined in the wireframes & visual. Nominating to
> koi?

Originally there was very little JS logic at all—only what was needed for handling the "pull" gesture. This is still the case, however now there is a significant amount of JS that must calculate and recalculate the available space. The operation that does the (re)calculation is _completely_ decoupled from the actual "pull down" mechanism (which means this bug's new title is incorrect).

Comment 26

[Mike Pennisi [:jugglinmike]]

Stepping off of this bug to focus on higher-priority Messaging work

Comment 27

[Julien Wajsberg [:julienw]]

> Just checked it again and it's not clear enough: it's not explained what should be done if the recipients list does not fill all the available space (slide 19, 6th panel).

So I discussed with Ayman and here is how it should behave:

* the bottom border of the panel should be adjacent to the top border of the one-line message input. However, if the message input has several lines, then the bottom border of the panel should still stop at the (now virtual) top border of a one-line message input. That means it would effectively hide the top part of the several-line input. And that the max-height of the recipients panel is fixed for a screen height (of course it will change whether the keyboard is displayed or hidden).

* ideally the recipients panel should open to show the recipients and only the recipients. That means there should not be a blank space.

Is it clear for you Rick ?

Comment 28

[Rick Waldron [:rwaldron]]

Julian, 

It looks like the pull down is broken _again_. This is the second time it's been negligently broken since the code for it landed. I will track down the issue and report back. Once I know more about that issue, I will respond to your notes

Comment 29

[Rick Waldron [:rwaldron]]

Checking in... I'm bisecting back from my commit that fixed this the first time it broke.

Comment 30

[Mike Pennisi [:jugglinmike]]

Attachment: Bug-884890-partial.patch

In comment 28, Rick identified a regression relating to the behavior of the Recipients list pull down. He correctly attributed that regression to a change that I made in resolving Bug 875362. This patch is my attempt to correct this regression.

I've also created a branch on GitHub in order to facilitate discussion on my approach:

https://github.com/jugglinmike/gaia/compare/mozilla-b2g:c5a589e...jugglinmike:884890-recipients-list

Comment 31

[Mike Pennisi [:jugglinmike]]

Attachment: Bug-884890.patch

Thanks to Rick's feedback (on IRC), we were able to promote the previous regression patch into a complete bug fix. (I've updated the GitHub pull request to match)

Comment 32

[Rick Waldron [:rwaldron]]

I'm really pleased with the solution that Mike and I were able to produce and I want to thank Mike for taking the whole day to work through this issue. 

I will address Julien's notes below, but first I made an assumption based on the second starred item: and that is that the pulldown should only open _fully_ if there so many recipients that the list will scroll.

(In reply to Julien Wajsberg [:julienw] from comment #27)
> > Just checked it again and it's not clear enough: it's not explained what should be done if the recipients list does not fill all the available space (slide 19, 6th panel).
> 
> So I discussed with Ayman and here is how it should behave:
> 
> * the bottom border of the panel should be adjacent to the top border of the
> one-line message input. 

Confirmed.

> However, if the message input has several lines,
> then the bottom border of the panel should still stop at the (now virtual)
> top border of a one-line message input. 

Confirmed.

> That means it would effectively hide
> the top part of the several-line input. 

Confirmed.

> And that the max-height of the
> recipients panel is fixed for a screen height (of course it will change
> whether the keyboard is displayed or hidden).

Confirmed.

> 
> * ideally the recipients panel should open to show the recipients and only
> the recipients. That means there should not be a blank space.

Confirmed.


All of these points are addressed in the patch that Mike attached.

Comment 33

[Julien Wajsberg [:julienw]]

Comment on attachment 788377 [details] [diff] [review]
Bug-884890.patch

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

I haven't tried it on the device (corey: please do ;) ) but this looks good to me. I especially love the simplification of this.

::: apps/sms/js/thread_ui.js
@@ +2130,5 @@
>    index = generateHeightRule.index || sheet.cssRules.length;
>    tmpl = generateHeightRule.tmpl || Utils.Template('height-rule-tmpl');
>  
>    css = tmpl.interpolate({
> +    height: String(height)

is the "String" explicit coercion really necessary here ?

Comment 34

[Rick Waldron [:rwaldron]]

(In reply to Julien Wajsberg [:julienw] from comment #33)
> Comment on attachment 788377 [details] [diff] [review]
> Bug-884890.patch
> 
> Review of attachment 788377 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I haven't tried it on the device (corey: please do ;) ) but this looks good
> to me. I especially love the simplification of this.
> 
> ::: apps/sms/js/thread_ui.js
> @@ +2130,5 @@
> >    index = generateHeightRule.index || sheet.cssRules.length;
> >    tmpl = generateHeightRule.tmpl || Utils.Template('height-rule-tmpl');
> >  
> >    css = tmpl.interpolate({
> > +    height: String(height)
> 
> is the "String" explicit coercion really necessary here ?

Yes, Utils.Template.prototype.interpolate only allows string values (this has always been the case)

Comment 35

[Mike Pennisi [:jugglinmike]]

Attachment: Bug-884890-v2.patch

I just realized that Julien is a more appropriate reviewer for this patch since he's been working with Rick on this bug and has familiarity with it. This new patch includes his handle in the commit message (and I've updated the pull request on GitHub to match).

Sorry for the noise, Corey!

Comment 36

[Julien Wajsberg [:julienw]]

Comment on attachment 788971 [details] [diff] [review]
Bug-884890-v2.patch

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

bug 1:
* have a multiline recipient
* tap on the recipient panel as to add a new recipient => it should be slid up
* slide it down
* press backspace
=> it's slid up again, but you can't slide it down unless you first focus the composer input

Bug 2:
* have a 2-line recipients panel
* slide it down => you have the keyboard
* tap the grey space in the middle => the keyboard hides
=> the bottom "padding" grows, which is a bug imho
=> maybe we should slide up the panel in that case ?

Bug 3:
* slide down the panel
* tap inside it to add a new recipient
=> it's slid up, whereas I think it should stay slid down in that case.

Bug 4:
* tap inside the panel
* slide down the panel
* type something
=> it's slid up, whereas I think it should stay slid down in that case too.

Comments:
* I'd like to somehow align the left border of the recipient boxes with the cursor (when it's at the start of a new line)
* I would decrease a little (2 px ?) the top and bottom "padding" in multiline mode

::: apps/sms/js/thread_ui.js
@@ +2127,5 @@
>    }
>  
>    sheet = generateHeightRule.sheet || sheets[sheets.length - 1];
>    index = generateHeightRule.index || sheet.cssRules.length;
>    tmpl = generateHeightRule.tmpl || Utils.Template('height-rule-tmpl');

could this template use max-height instead of height ?

and then you could just set the height to "natural". Or maybe not fix the height at all, and let CSS do its magic ?

Also, just to be sure: are we injecting CSS so that we don't have a synchronous reflow but a interruptible reflow instead ? Therefore would it make sense to put the composer input max height in that template as well ?

Comment 37

[Julien Wajsberg [:julienw]]

Ayman, could you please give your advice about the Bug 3 and Bug 4 in my comment 36 ? I mean, are there expected behaviour, or are there bugs ?

Comment 38

[Mike Pennisi [:jugglinmike]]

Hey Julien,

Thanks as always for the thorough review. I'm going to have to take some time with each of the bugs you mentioned but regarding the `height` suggestions:

(In reply to Julien Wajsberg [:julienw] from comment #36)
> ::: apps/sms/js/thread_ui.js
> @@ +2127,5 @@
> >    }
> >  
> >    sheet = generateHeightRule.sheet || sheets[sheets.length - 1];
> >    index = generateHeightRule.index || sheet.cssRules.length;
> >    tmpl = generateHeightRule.tmpl || Utils.Template('height-rule-tmpl');
> 
> could this template use max-height instead of height ?
> 
> and then you could just set the height to "natural". Or maybe not fix the
> height at all, and let CSS do its magic ?
> 
> Also, just to be sure: are we injecting CSS so that we don't have a
> synchronous reflow but a interruptible reflow instead ? Therefore would it
> make sense to put the composer input max height in that template as well ?

We need to explicitly set `height` (i.e. not rely on `height: auto;` and/or set `max-height`) so that the Recipients element animates as it transitions between `singleline` and `multiline` mode.

Comment 39

[Julien Wajsberg [:julienw]]

(In reply to Mike Pennisi [:jugglinmike] from comment #38)

> We need to explicitly set `height` (i.e. not rely on `height: auto;` and/or
> set `max-height`) so that the Recipients element animates as it transitions
> between `singleline` and `multiline` mode.

Tell me if I'm wrong here, but since we only open the panel to contain the recipients now, couldn't a transform: translate work fine here (if not better, would probably be smoother), using translate(-100%) for example ?

Comment 40

[Mike Pennisi [:jugglinmike]]

That's an interesting idea, Julien! It will definitely perform better, but it may take a little more refactoring.

The document layout will not respond to changes in an element's `transform` property, and right now, we are relying on the change in the `messages-recipients-list` height to effect the height of the containing `messages-to-field`. We may be able to get around this by instead animating the `transform` of the `messages-to-field`--we'll just need to set a fixed position for the "To:" label and the "Add contact" button.

In my experience, removing elements from the document flow leads to less maintainable code. I will investigate this approach and report back on what it could mean for maintainability.

Comment 41

[Julien Wajsberg [:julienw]]

Note that we can move forward with this bug that is quite ready and do the transform experiment in another bug. But now is a good time to experiment with this imho.

Comment 42

[ayman maat :maat]

(In reply to Julien Wajsberg [:julienw] from comment #36)
> Comment on attachment 788971 [details] [diff] [review]
> Bug-884890-v2.patch
> 
> Review of attachment 788971 [details] [diff] [review]:
> -----------------------------------------------------------------
 
> Bug 3:
> * slide down the panel
> * tap inside it to add a new recipient
> => it's slid up, whereas I think it should stay slid down in that case.

referencing 
wireframe pack: HTML5_SMS-MMSUserStorySpecifications_20130503_V8.0
page: 19
frame 7: Closing the to field

The ‘to’ field is closed be the user:
1) typing on the keyboard when the focus is still on the ‘to’ field
2) tapping on the ‘your message’ textfield and so changing focus
to the ‘your message field’

…so this is behaving as specified, but i would say that the current transition makes it feel awkward. 


> Bug 4:
> * tap inside the panel
> * slide down the panel
> * type something
> => it's slid up, whereas I think it should stay slid down in that case too.

…so this is behaving as specified.

Comment 43

[Julien Wajsberg [:julienw]]

Thanks Ayman. And just to be clear, you still do think that the specified behavior is good here ?

Comment 44

[ayman maat :maat]

(In reply to Julien Wajsberg [:julienw] from comment #43)
> Thanks Ayman. And just to be clear, you still do think that the specified
> behavior is good here ?

From a UX PoV it is not a deal breaker if the user pulls down on the 'to' field, starts to type in the 'to' field and the 'to' field stays open ...in fact seeing it in action it would probably be a better UX than is specified if it did stay open because the end user has visibility of a greater number of recipients - thus their cognitive load is lowered - and their activity focus is addition of recipients if they continue to type in the to field.  

All i can say is that what has been implemented is what i specified. So the developer has not deviated. However if we have bandwidth to alter the specified behavior and it does not bring any risk to the app i am happy to underwrite a change in the UX so that the 'to' field stays 'open' when the user types into it... let me know what you plan to do

Comment 45

[Julien Wajsberg [:julienw]]

That's 1.2 work so I'm ok with taking some time to do it better for the user :)

Comment 46

[Mike Pennisi [:jugglinmike]]

Attachment: Bug-884890-v3.patch

Hi Julien,

Yesterday, I was able to reproduce "bug 1" on `master`. Today, I can't reproduce it on `master` *or* with this patch applied. Could you verify that the bug still exists with this patch and not on the latest `master` branch?

This patch addresses "bug 2" through `box-sizing: border-box;`.

It sounds like "bug 3" and "bug 4" are both specified behavior, so I have not made any changes to address them.

Comment 47

[Mike Pennisi [:jugglinmike]]

Comment on attachment 789671 [details] [diff] [review]
Bug-884890-v3.patch

Somehow I missed comment 44 and comment 45. I will work on a new patch that implements the new pulldown functionality.

Comment 48

[ayman maat :maat]

(In reply to Julien Wajsberg [:julienw] from comment #45)
> That's 1.2 work so I'm ok with taking some time to do it better for the user
> :)

ok, i was thinking this over lastnight. We should do this as it is an improvement to the UX but one thing we need to bear in mind is that in doing it we are removing a mechanism to close the opened 'to' field. This puts more 'pressure', shall we say, on creating an affordance/easily selectable touch area to drag the 'to' field up in order to close it. I would therefore suggest that when the to field is open we leave more of a gap between the underside of the last contact module at the bottom of the list of recipients and the bottom edge of the 'to' field. This would give a greater target area and also reduce the risk of user scrolling the list of recipients instead of closing the 'to' field when it is open.

...we might therefore require also a little visual design input. What do you think Julien?

Comment 49

[Julien Wajsberg [:julienw]]

Ayman, we just agreed with Mike that this would be done in another bug, I'll file it and answer you there.

Comment 50

[Mike Pennisi [:jugglinmike]]

Comment on attachment 789671 [details] [diff] [review]
Bug-884890-v3.patch

Re-requesting review on this bug based on an IRC discussion with Julien. Implementation of the new pull-down behavior will be tracked in bug 905169

Comment 51

[ayman maat :maat]

(In reply to Julien Wajsberg [:julienw] from comment #49)
> Ayman, we just agreed with Mike that this would be done in another bug, I'll
> file it and answer you there.

good stuff.

Comment 52

[Julien Wajsberg [:julienw]]

Comment on attachment 789671 [details] [diff] [review]
Bug-884890-v3.patch

r=me

if you can do a test without changing the app code, it's good, otherwise just land it.

Thanks !

Comment 53

[Julien Wajsberg [:julienw]]

Filed bug 905169 for the specification changes, bug 905260 for some padding changes, and Mike will file a bug for using max-height and transformations.

Comment 54

[Mike Pennisi [:jugglinmike]]

Landed on `master` at commit 79e0d142dc6ea6741d37d593ebb639299f827275

Thanks for the help, Julien and Rick!

Comment 55

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

clear the koi? flag. it's already landed on master