Closed Bug 1015867 Opened 5 years ago Closed 5 years ago

[Messages] The editable composer should take more space

Categories

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

x86_64
Linux
defect
Not set

Tracking

(blocking-b2g:2.0+, b2g-v1.4 unaffected, b2g-v2.0 fixed, b2g-v2.1 fixed)

RESOLVED FIXED
2.0 S4 (20june)
blocking-b2g 2.0+
Tracking Status
b2g-v1.4 --- unaffected
b2g-v2.0 --- fixed
b2g-v2.1 --- fixed

People

(Reporter: julienw, Assigned: julienw)

References

Details

(Keywords: regression, Whiteboard: [sprint2 p=2][sprint3 p=1])

Attachments

(5 files)

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.
Flags: needinfo?(vpg)
Blocks: sms-sprint-2
Whiteboard: [p=2]
Target Milestone: --- → 2.0 S3 (6june)
Assignee: nobody → felash
(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.
Flags: needinfo?(vpg)
Victoria, I think we can also keep a wider gap when the keyboard is hidden; what do you think?
Flags: needinfo?(vpg)
(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.
Flags: needinfo?(vpg)
Can we get a screenshot of the bug here & indication of impact of if we don't fix this?
Flags: needinfo?(felash)
We can see 6 lines of message only.
Flags: needinfo?(felash)
(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.
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.
When the keyboard is hidden, we have yet more space, and so we can show more of the message.
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
Flags: needinfo?(vpg)
(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
Flags: needinfo?(vpg)
Attached file github PR
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...
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
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.
Blocks: 1021648
Blocks: 1013296
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 :)
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.
Attachment #8434162 - Flags: review?(schung)
(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 !
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.
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?
Attachment #8436999 - Flags: ui-review?(vpg)
Blocks: 951672
(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.
blocking-b2g: --- → 2.0?
(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.
Status: NEW → ASSIGNED
Duplicate of this bug: 1021411
Whiteboard: [p=2] → [sprint2 p=2][sprint3 p=1]
Target Milestone: 2.0 S3 (6june) → 2.0 S4 (20june)
Blocks: sms-sprint-3
Blocks: 907103
blocking-b2g: 2.0? → 2.0+
(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?
Flags: needinfo?(schung)
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.
Flags: needinfo?(schung)
Yeah, I believe like you then, same than bug 1021788 :)
Should we go ahead and merge this patch then?
Depends on: 1021788
Comment on attachment 8434162 [details] [review]
github PR

I left some comments on github, just some nits and overall looks great!
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!
Attachment #8434162 - Flags: review?(schung) → review+
Updated, tested on the device, now waiting for a green Travis (or Gaia Try) before merging.
master: 0905395339414feb099182c65f5c912a834eed82
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
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.
Attachment #8436999 - Flags: ui-review?(vpg) → ui-review-
(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.
Depends on: 1027593
You need to log in before you can comment on or make changes to this bug.