Julien Wajsberg [:julienw] Assignee
	Description • 10 years ago

This was regressed by bug 949457.

* In the Composer panel, the message composer can take the whole available space
* In the Thread panel, the message composer can take the whole available space minus a fixed height so that the user can still see the thread.
* In the Thread panel still, when the keyboard is hidden, the part of the thread the user can see is bigger.

NI Victoria to fix the "fixed height" in the 2 last cases.

	Victoria Gerchinhoren [:vicky]
	Comment 1 • 10 years ago

(In reply to Julien Wajsberg [:julienw] from comment #0)
> This was regressed by bug 949457.
> 
> * In the Composer panel, the message composer can take the whole available
> space
> * In the Thread panel, the message composer can take the whole available
> space minus a fixed height so that the user can still see the thread.
> * In the Thread panel still, when the keyboard is hidden, the part of the
> thread the user can see is bigger.
> 
> NI Victoria to fix the "fixed height" in the 2 last cases.

Please leave a 4 rem gap for this cases, it could be also 3 but in the thread, I think it worths to have more space to see a few lines of text from the bubbles behind the to and compose area.
Thanks.

	Julien Wajsberg [:julienw] Assignee
	Comment 2 • 10 years ago

Victoria, I think we can also keep a wider gap when the keyboard is hidden; what do you think?

	Victoria Gerchinhoren [:vicky]
	Comment 3 • 10 years ago

(In reply to Julien Wajsberg [:julienw] from comment #2)
> Victoria, I think we can also keep a wider gap when the keyboard is hidden;
> what do you think?

In the case of the thread? Yes, for sure. I think that in this scenario is important to have a bigger area where the user can go through the conversation. If we keep 8 rem for the gap, the user can visualize 3.5 lines aprox of the text in the bubbles, and also allows the composer area to hold at least 3 lines as well before scrolling.

	Jason Smith [:jsmith]
	Comment 4 • 10 years ago

Can we get a screenshot of the bug here & indication of impact of if we don't fix this?

	Julien Wajsberg [:julienw] Assignee
	Comment 5 • 10 years ago

We can see 6 lines of message only.

	Julien Wajsberg [:julienw] Assignee
	Comment 6 • 10 years ago

(In reply to Julien Wajsberg [:julienw] from comment #5)
> Created attachment 8429284 [details]
> screenshot on buri with issue
> 
> We can see 6 lines of message only.

And this is independent on the device: HD devices that have more space are also limited to 6 lines.

	Julien Wajsberg [:julienw] Assignee
	Comment 7 • 10 years ago

On peak, we have more available space, so we can see more lines. On Buri, we would probably see only 6 lines like on the previous screenshot.

	Julien Wajsberg [:julienw] Assignee
	Comment 8 • 10 years ago

When the keyboard is hidden, we have yet more space, and so we can show more of the message.

	Julien Wajsberg [:julienw] Assignee
	Comment 9 • 10 years ago

So, I took the various measurements, and based on the resolution for the Buri, I ended up using this formula:

composer maximum height = 84.6vh - 7.62rem
(reminder: 100vh = height of the available space -> it changes when the keyboard is displayed)

On Buri, it makes exactly a gap of 4rem when there is a carrier subheader and the keyboard is displayed with suggestions, and 8rem when there is a carrier subheader and the keyboard is hidden.

This won't make exactly 4rem/8rem when there is no suggestion or when there is no carrier subheader, but then the gap will be wider, so I think it's good.

Also, this adapts automatically to HD phones that will have both a bigger gap and a bigger composer maximum height. This would also adapt if the keyboard is bigger/smaller.

Victoria, would this logic work for you?



For the record, here are the calculations:

Buri's height is 48rem.
Application's height is 22rem when the default keyboard is displayed with suggestions (directly measured on the device using App Manager).

We want:
composer max height = 100vh - header - subheader - gap = 100vh - 5rem - 2rem - gap

Which means, on Buri:
When keyboard is hidden: gap = 8rem => composer max height = 100vh - 5 - 2 - 8 = 100vh - 15rem = 48rem - 15rem = 33rem
When keyboard is displayed: gap = 4rem => composer max height = 100vh - 5 - 2 - 4 = 100vh - 11rem = 22rem - 11rem = 11rem

Now, I want to find a and b in this formula:
h = a * v + b

where c = composer height, and v is the viewport height.

when viewport = 22rem, composer = 11rem
when wiewport = 48rem, composer = 33rem

So here are my equations:

11 = a * 22 + b
33 = a * 48 + b

a = 22 / 26 = .846
b = -7.62

And so: max-height = 84.6vh - 7.62rem

	Victoria Gerchinhoren [:vicky]
	Comment 10 • 10 years ago

(In reply to Julien Wajsberg [:julienw] from comment #9)
> So, I took the various measurements, and based on the resolution for the
> Buri, I ended up using this formula:
> 
> composer maximum height = 84.6vh - 7.62rem
> (reminder: 100vh = height of the available space -> it changes when the
> keyboard is displayed)
> 
> On Buri, it makes exactly a gap of 4rem when there is a carrier subheader
> and the keyboard is displayed with suggestions, and 8rem when there is a
> carrier subheader and the keyboard is hidden.
> 
> This won't make exactly 4rem/8rem when there is no suggestion or when there
> is no carrier subheader, but then the gap will be wider, so I think it's
> good.
> 
> Also, this adapts automatically to HD phones that will have both a bigger
> gap and a bigger composer maximum height. This would also adapt if the
> keyboard is bigger/smaller.
> 
> Victoria, would this logic work for you?
> 

Sounds good. Thanks!

> 
> For the record, here are the calculations:
> 
> Buri's height is 48rem.
> Application's height is 22rem when the default keyboard is displayed with
> suggestions (directly measured on the device using App Manager).
> 
> We want:
> composer max height = 100vh - header - subheader - gap = 100vh - 5rem - 2rem
> - gap
> 
> Which means, on Buri:
> When keyboard is hidden: gap = 8rem => composer max height = 100vh - 5 - 2 -
> 8 = 100vh - 15rem = 48rem - 15rem = 33rem
> When keyboard is displayed: gap = 4rem => composer max height = 100vh - 5 -
> 2 - 4 = 100vh - 11rem = 22rem - 11rem = 11rem
> 
> Now, I want to find a and b in this formula:
> h = a * v + b
> 
> where c = composer height, and v is the viewport height.
> 
> when viewport = 22rem, composer = 11rem
> when wiewport = 48rem, composer = 33rem
> 
> So here are my equations:
> 
> 11 = a * 22 + b
> 33 = a * 48 + b
> 
> a = 22 / 26 = .846
> b = -7.62
> 
> And so: max-height = 84.6vh - 7.62rem

	Julien Wajsberg [:julienw] Assignee
	Comment 11 • 10 years ago

WIP

I've seen a huge performance issue when using vh; simply replacing with % works better.

I'll do a profile and possibly use % instead, using another formula...

	Julien Wajsberg [:julienw] Assignee
	Comment 12 • 10 years ago

Filed bug 1020469 for the performance issue with using vh. 

As a result I'll use % instead, and also, instead of setting max-height on the composer, I'll set the flex-basis on the element for the messages list.

Here is the new calculation:

The flex-composer height is the basis because the messages list element is its direct child.


when the flex-composer height is s = buri_height - header - keyboard_height, we keep gap1 = 4rem
when the flex-composer height is s = buri_height - header, we keep gap2 = 8rem

The gap is the target height for the messages list element (#composer-messages-container), but we need to add the subheader height because it's absolutely positioned.

Here are the equations:

gap1 + subheader = a * (buri_height - header - keyboard_height) + b
gap2 + subheader = a * (buri_height - header) + b

a = (gap2 - gap1) / keyboard_height
b = gap2 - (gap2 - gap1) * (buri_height - header) / keyboard_height + subheader

a = 0.154
b = 3.38rem

	Julien Wajsberg [:julienw] Assignee
	Comment 13 • 10 years ago

I pushed the new version to github but there is a strange things happening on the device only: the reflow is not done properly when the keyboard is hidden.

If the app manager is connected to the phone, the reflow is done as soon as I select a visible element in the inspector.

I couldn't reproduce on a simpler page yet.

	Julien Wajsberg [:julienw] Assignee
	Comment 14 • 10 years ago

Found the issue, and I filed Bug 1022240 as a result. The problem was that I was using "height: 100%" but its container had "flex-basis: auto", which is wrong.

I resolved the issue using nested flex containers, which seems to work perfectly well in Firefox. Will test on a device tomorrow, and hopefully Steve will be able to review monday :)

	Julien Wajsberg [:julienw] Assignee
	Comment 15 • 10 years ago

Comment on attachment 8434162 [details] [review]
github PR

Hey Steve !

I think it's ready for review.

There is only one thing t hat I can't find, maybe you'll know: 
* tap "new message" button
* add at least 3 lines of text
* press the "send" button, or even the empty space above

=> the send button gets a greyer background

I don't find where this comes from, this does not happen on master.

	Julien Wajsberg [:julienw] Assignee
	Comment 16 • 10 years ago

(In reply to Julien Wajsberg [:julienw] from comment #15)

> There is only one thing t hat I can't find, maybe you'll know: 
> * tap "new message" button
> * add at least 3 lines of text
> * press the "send" button, or even the empty space above
> 
> => the send button gets a greyer background
> 
> I don't find where this comes from, this does not happen on master.

Found that we were selecting the button for some reason, so I've added "-moz-user-select: none" on the <div> that contains the button, and it seems to do the trick :)

So it's ready for review !

	Steve Chung [:steveck]
	Comment 17 • 10 years ago

Comment on attachment 8434162 [details] [review]
github PR

This patch looks great! only some issue that unrelated to this patch but affect the result:

1) When creating a message with super long text context that fills full view.

2) Add subject, type some words in subjuct field. The notification will cover the subject field, ni? Vicky that whether we should leave a space for notification or just cover it.

3) When focus on recipient or message text field, it's not possible to focus on subject again because of the subhead's height exceeds to-field's, and it makes user always focus on recipient instead of subject. Hi Julien, do you think we can have a quick fix for the subhead in this patch? If the fix is complicate, I'm fine that creating another bug for addressing this issue.

	Steve Chung [:steveck]
	Comment 18 • 10 years ago

The message convert notification covers the subject field, should we leave a space for it or just cover it anyway since it will dismiss later?

	Julien Wajsberg [:julienw] Assignee
	Comment 19 • 10 years ago

(In reply to Steve Chung [:steveck] from comment #18)
> Created attachment 8436999 [details]
> Screen Shot 2014-06-02 at 23.44.48.png
> 
> The message convert notification covers the subject field, should we leave a
> space for it or just cover it anyway since it will dismiss later?

FTR Ayman made the "max length" notice to dismiss after a timeout exactly because of this.

The case you're saying is an issue because the user is especially writing the subject at that moment. Note that it probably happened on master before bug 949457 landed, so it probably happens on v1.4.

	Julien Wajsberg [:julienw] Assignee
	Comment 20 • 10 years ago

(In reply to Steve Chung [:steveck] from comment #17)

> 3) When focus on recipient or message text field, it's not possible to focus
> on subject again because of the subhead's height exceeds to-field's, and it
> makes user always focus on recipient instead of subject. Hi Julien, do you
> think we can have a quick fix for the subhead in this patch? If the fix is
> complicate, I'm fine that creating another bug for addressing this issue.

Yep, will definitely take a look.

	Julien Wajsberg [:julienw] Assignee
	Comment 22 • 10 years ago

(In reply to Julien Wajsberg [:julienw] from comment #20)
> (In reply to Steve Chung [:steveck] from comment #17)
> 
> > 3) When focus on recipient or message text field, it's not possible to focus
> > on subject again because of the subhead's height exceeds to-field's, and it
> > makes user always focus on recipient instead of subject. Hi Julien, do you
> > think we can have a quick fix for the subhead in this patch? If the fix is
> > complicate, I'm fine that creating another bug for addressing this issue.
> 
> Yep, will definitely take a look.

Steve, I'm sorry, I don't reproduce this, so I think I don't understand what you mean.

Can you please describe with a step-by-step STR, possibly a video?

	Steve Chung [:steveck]
	Comment 23 • 10 years ago

I'm terribly sorry that I missed key point in the steps: This behavior exist in compose page and the recipient list should be long enough because the subheader's height will increased depends on the height of the list container.

1) Enter the new message page and add tons of recipients.
2) Text a super long message, and add the subject
3) Focus on somewhere else, than focus on subject again(but it is not possible to focus on it now)

I think the root cause is the same as bug 1021788 because of subheader's height. Maybe I can solve it first.

	Julien Wajsberg [:julienw] Assignee
	Comment 24 • 10 years ago

Yeah, I believe like you then, same than bug 1021788 :)
Should we go ahead and merge this patch then?

	Steve Chung [:steveck]
	Comment 25 • 10 years ago

Comment on attachment 8434162 [details] [review]
github PR

I left some comments on github, just some nits and overall looks great!

	Steve Chung [:steveck]
	Comment 26 • 10 years ago

Comment on attachment 8434162 [details] [review]
github PR

Since they are just small nits in css, r=me and let's kick off the following visual refresh tasks!

	Julien Wajsberg [:julienw] Assignee
	Comment 27 • 10 years ago

Updated, tested on the device, now waiting for a green Travis (or Gaia Try) before merging.

	Julien Wajsberg [:julienw] Assignee
	Comment 28 • 10 years ago

master: 0905395339414feb099182c65f5c912a834eed82

	Victoria Gerchinhoren [:vicky]
	Comment 29 • 10 years ago

Comment on attachment 8436999 [details]
Screen Shot 2014-06-02 at 23.44.48.png

MMS indicator should be lined up to the top of the box.

	Ryan VanderMeulen [:RyanVM]
	Comment 30 • 10 years ago

v2.0: https://github.com/mozilla-b2g/gaia/commit/e1e71c244d1af9afa88d1334b60e0c8a8deff68f

	Julien Wajsberg [:julienw] Assignee
	Comment 31 • 10 years ago

(In reply to Victoria Gerchinhoren [:vicky] from comment #29)
> Comment on attachment 8436999 [details]
> Screen Shot 2014-06-02 at 23.44.48.png
> 
> MMS indicator should be lined up to the top of the box.

Yep, this is one of the goals of bug 1008127.

The goal of this bug is to let the composer element grow.