1022644 - [Messages] Can't open the recipient panel if there are only 2 lines of recipients

Status: VERIFIED FIXED

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

Description

[Julien Wajsberg [:julienw] (PTO -> may 13)]

STR:

1. open "New Message" window
2. enter 2 lines of recipients
3. try to slide down the panel to clearly see all recipients

=>  this does not work.

(current master)

Note that this work as soon as there are 3 lines of recipients.

This works on my peak v1.4, so it's a regression, probably from the composer refactoring in bug 963018.

Comment 1

[Maria Angeles Oteo (:oteo)]

Umm, in today's master build I am able to slide down the panel and see the two lines of recipients.

Comment 2

[Julien Wajsberg [:julienw] (PTO -> may 13)]

So maybe something else more intermittent is happening...

I think we noticed we couldn't slide down the panel if the recipients come from a draft, so maybe I'm seeing this ?

Comment 3

[Steve Chung [:steveck]]

(In reply to Julien Wajsberg [:julienw] from comment #2)
> So maybe something else more intermittent is happening...
> 
> I think we noticed we couldn't slide down the panel if the recipients come
> from a draft, so maybe I'm seeing this ?

It's not intermittent issue. I've put the STR in bug 1021513 comment 7:
1) Create a multi-recipient message and save as draft (gesture works now)
2) Close message app, and launch again(If we don't close the app, this issue could not happend :-/)
3) Open the draft again, and the gesture could not work(even create another message)

Comment 4

[Steve Chung [:steveck]]

Attachment: Bug-1022644-Messages-Can-t-open-the-recipient-panel-.patch

Hi Julien, this patch is my first proposal that:
1) Remove the view dims because I think it's redundant in current recipient structure 
2) Use minHeight instead, but maybe you have better idea about how to detect if container should be pannable or not.

Since test for this pan gesture part didn't exist before, we will need additionl test for it after we have conclusion, thanks.

Comment 5

[Julien Wajsberg [:julienw] (PTO -> may 13)]

Comment on attachment 8440622 [details] [diff] [review]
Bug-1022644-Messages-Can-t-open-the-recipient-panel-.patch

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

::: apps/sms/js/recipients.js
@@ +740,5 @@
>          //  2. The user is "pulling down" the recipient list.
>  
>          // #1
> +        if (view.state.visible === 'singleline' &&
> +            view.inner.scrollHeight > view.minHeight) {

here is another idea: since we scroll the inner container in .focus and .render (using different code by the way), maybe we only need to check that scrollTop > 0?

and then we don't need to keep either minHeight or dims.

Comment 6

[Steve Chung [:steveck]]

(In reply to Julien Wajsberg [:julienw] from comment #5)
> Comment on attachment 8440622 [details] [diff] [review]
> Bug-1022644-Messages-Can-t-open-the-recipient-panel-.patch
> 
> Review of attachment 8440622 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: apps/sms/js/recipients.js
> @@ +740,5 @@
> >          //  2. The user is "pulling down" the recipient list.
> >  
> >          // #1
> > +        if (view.state.visible === 'singleline' &&
> > +            view.inner.scrollHeight > view.minHeight) {
> 
> here is another idea: since we scroll the inner container in .focus and
> .render (using different code by the way), maybe we only need to check that
> scrollTop > 0?
> 
> and then we don't need to keep either minHeight or dims.

The scrollTop would alway be 0 until recipient list longer that max container height, container position is controlled by transform, not by scroll position.

Another idea is we just show the multiline mode(maybe rename it like "showFullList") anyway no matter the container is only single line or not. Not many user will try to swipe container if there is only single line, but the only problem is we set additional padding bottom for container in multiline mode. If we don't set this padding for multiline mode, there is no difference for sinle line container under 2 modes.

Comment 7

[Steve Chung [:steveck]]

Hi Julien, you said you might have another idea for this case during the scrum. It would be great to have a simpler solution that can get rid of all the computed styling value.

Comment 8

[Julien Wajsberg [:julienw] (PTO -> may 13)]

Same issue happens in v1.4 => requesting 1.4 blocker status.

Comment 9

[Julien Wajsberg [:julienw] (PTO -> may 13)]

v1.3t has a limited and different version of the draft feature, and has the same issue with this STR:

1. start a new message
2. add more than 2 lines of recipients
3. add some text
4. kill the app
5. restart the app
6. try to slide down the recipient.

So nominating for v1.3t too.

Comment 10

[Julien Wajsberg [:julienw] (PTO -> may 13)]

Comment on attachment 8440622 [details] [diff] [review]
Bug-1022644-Messages-Can-t-open-the-recipient-panel-.patch

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

Now that I understand better the issue, I think your solution should work fine. Here are some comments.

Of course we'll need unit tests.

And since the bug is present in all branches that have drafts, I think we need to move forward quickly here.

::: apps/sms/js/recipients.js
@@ +374,4 @@
>      var template = setup.template;
>      var nodes = [];
>      var clone;
> +    var outerCss = window.getComputedStyle(outer, null);

nit: you don't need the second argument.

@@ +387,4 @@
>          isTransitioning: false,
>          visible: 'singleline'
>        },
> +      minHeight: parseInt(outerCss.getPropertyValue('min-height'), 10)

Why don't you use "height" here? I'd say the "height" when the app is starting would have the correct value? But "min-height" will keep a constant value so it's maybe safer.

In the end, your choice, I'll let you choose !

Comment 11

[Julien Wajsberg [:julienw] (PTO -> may 13)]

Ni Bhavana for the nomination, since I don't know if there still are v1.3t triages.

Comment 12

[bhavana bajaj [:bajaj]]

I've blocked this all the way upto 1.3T given comment #9. you will have to self land it on the branch once ready(central landing/testing completed). Here is the branch information : https://wiki.mozilla.org/Release_Management/B2G_Landing#v1.3T

For getting this fixed on 1.4/2.0, once it landed on central, please seek approval. Flip the approval‑mozilla‑b2g30 and fill in the attachment, and once approved landings will be taken care. 

Thanks !

Comment 13

[Steve Chung [:steveck]]

Attachment: Link to github

Hi Oleg, this patch basically modified form previous one that Julien already gave some feedback. Could you please take a look when he is OOO? Thanks.

Comment 14

[Oleg Zasypkin [:azasypkin]]

Comment on attachment 8446341 [details] [review]
Link to github

Don't have anything to add, looks good! :) Just tiny question at github.

Thanks!

Comment 15

[Steve Chung [:steveck]]

(In reply to Oleg Zasypkin [:azasypkin] from comment #14)
> Comment on attachment 8446341 [details] [review]
> Link to github
> 
> Don't have anything to add, looks good! :) Just tiny question at github.
> 
> Thanks!

I replied here: https://github.com/mozilla-b2g/gaia/pull/20973/files#r14277609 and I made a little adjustment. Do you think this change is fine?

Comment 16

[Oleg Zasypkin [:azasypkin]]

(In reply to Steve Chung [:steveck] from comment #15)
> (In reply to Oleg Zasypkin [:azasypkin] from comment #14)
> > Comment on attachment 8446341 [details] [review]
> > Link to github
> > 
> > Don't have anything to add, looks good! :) Just tiny question at github.
> > 
> > Thanks!
> 
> I replied here: https://github.com/mozilla-b2g/gaia/pull/20973/files#﷒0﷓ and
> I made a little adjustment. Do you think this change is fine?

Yes, I think it's fine!

Comment 17

[Steve Chung [:steveck]]

in master: b04d61c36ca7e0c0a949dbb1bf506b1220082c93

Will uplift this patch for v1.3T later.

Comment 18

[Steve Chung [:steveck]]

Attachment: Link to github(for v1.3t)

Hi Oleg, since the structure of recipient is different in v1.3t, I adjusted the patch a liitle bit for 1.3t branch. Could you please review it again? Thanks.

Comment 19

[Oleg Zasypkin [:azasypkin]]

Comment on attachment 8447930 [details] [review]
Link to github(for v1.3t)

r=me, just left small nit at GitHub. Thanks!

Comment 20

[Steve Chung [:steveck]]

Comments addressed and Travis is green now, thanks!
on v1.3t: 366bf651ac320c8ccc458f08ece01f2b8aadfa1c

Comment 21

[Julien Wajsberg [:julienw] (PTO -> may 13)]

Steve, you still need to raise approvals for 1.4 and 2.0 here.

I guess you can ask approval for the 2.0 on the master patch, and approval for 1.4 on the 1.3t patch.

Comment 22

[Steve Chung [:steveck]]

(In reply to Julien Wajsberg [:julienw] (away June 21 to 30) from comment #21)
> Steve, you still need to raise approvals for 1.4 and 2.0 here.
> 
> I guess you can ask approval for the 2.0 on the master patch, and approval
> for 1.4 on the 1.3t patch.

Ya I know, just verifying the uplifting works(got a small conflict when uplift to v1.4, will create another patch for that).

Comment 23

[Steve Chung [:steveck]]

Attachment: Link to github(for v1.4)

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): Regression from message draft feature.
[User impact] if declined: User could not drag down the recipient list to show the full list.
[Testing completed]: Yes
[Risk to taking this patch] (and alternatives if risky): Low
[String changes made]: No

Since this patch is uplifted from v1.3t(with one line conflict in test), I didn't request the review for it.

Comment 24

[Steve Chung [:steveck]]

Comment on attachment 8446341 [details] [review]
Link to github

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): Regression from message draft feature.
[User impact] if declined: User could not drag down the recipient list to show the full list.
[Testing completed]: Yes
[Risk to taking this patch] (and alternatives if risky): Low
[String changes made]: No

Comment 25

[bhavana bajaj [:bajaj]]

Comment on attachment 8448604 [details] [review]
Link to github(for v1.4)

Adding verifyme for the patch to be verified once it lands on 1.4 and 2.0

Comment 26

[Ryan VanderMeulen [:RyanVM]]

v2.0: https://github.com/mozilla-b2g/gaia/commit/2dfe43bdeeb09aecfccc8bb1cc27dcf8bd315d98
v1.4: https://github.com/mozilla-b2g/gaia/commit/cb1ad02ba2dc1f0a912153d660209dfad9d3bce4

Comment 27

[Duane Dixon [:ddixon]]

Issue is verified fixed in Flame 2.2, 2.1, 1.4 (Full Flash, nightly)

Actual Results: "To:" field is scrollable when multiple contacts are selected.

Device: Flame Master 
Build ID: 20141024040202
Gaia: d893a9b971a0f3ee48e5a57dca516837d92cf52b
Gecko: a5ee2769eb27
Version: 36.0a1 (Master)
Firmware Version: v188
User Agent: Mozilla/5.0 (Mobile; rv:36.0) Gecko/36.0 Firefox/36.0

Device: Flame 2.1
Build ID: 20141024001204
Gaia: 0f76e0baac733cca56d0140e954c5f446ebc061f
Gecko: 7d78ff7d25b6
Version: 34.0 (2.1)
Firmware Version: v188
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0

Device: Flame 1.4
Build ID: 20140814202332
Gaia: 129211661489feb60bbd6772a44081d23b374f17
Gecko: 1483bd406aad0b3b9e3bdb75c5238b787b5a0cd6
Version: 30.0 (1.4)
Firmware Version: v165
User Agent: Mozilla/5.0 (Mobile; rv:30.0) Gecko/30.0 Firefox/30.0

Comment 28

[Duane Dixon [:ddixon]]

Issue is verified fixed in 1.3t Tarako build. 

Actual Results: "To:" field is scrollable when multiple contacts are selected.

Device: Tarako
Build:20140708014001
Base: SP6821a-Gonk-4.0-5-12