Closed
Bug 1015867
Opened 11 years ago
Closed 11 years ago
[Messages] The editable composer should take more space
Categories
(Firefox OS Graveyard :: Gaia::SMS, defect)
Tracking
(blocking-b2g:2.0+, b2g-v1.4 unaffected, b2g-v2.0 fixed, b2g-v2.1 fixed)
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)
Assignee | ||
Updated•11 years ago
|
Assignee | ||
Updated•11 years ago
|
Assignee: nobody → felash
Comment 1•11 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.
Flags: needinfo?(vpg)
Assignee | ||
Comment 2•11 years ago
|
||
Victoria, I think we can also keep a wider gap when the keyboard is hidden; what do you think?
Flags: needinfo?(vpg)
Comment 3•11 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.
Flags: needinfo?(vpg)
Comment 4•11 years ago
|
||
Can we get a screenshot of the bug here & indication of impact of if we don't fix this?
Flags: needinfo?(felash)
Assignee | ||
Comment 6•11 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.
Assignee | ||
Comment 7•11 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.
Assignee | ||
Comment 8•11 years ago
|
||
When the keyboard is hidden, we have yet more space, and so we can show more of the message.
Assignee | ||
Comment 9•11 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
Flags: needinfo?(vpg)
Comment 10•11 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
Flags: needinfo?(vpg)
Assignee | ||
Comment 11•11 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...
Assignee | ||
Comment 12•11 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
Assignee | ||
Comment 13•11 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.
Assignee | ||
Comment 14•11 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 :)
Assignee | ||
Comment 15•11 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.
Attachment #8434162 -
Flags: review?(schung)
Assignee | ||
Comment 16•11 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 !
Comment 17•11 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.
Comment 18•11 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?
Attachment #8436999 -
Flags: ui-review?(vpg)
Assignee | ||
Comment 19•11 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.
blocking-b2g: --- → 2.0?
Assignee | ||
Comment 20•11 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.
Assignee | ||
Updated•11 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Updated•11 years ago
|
Whiteboard: [p=2] → [sprint2 p=2][sprint3 p=1]
Target Milestone: 2.0 S3 (6june) → 2.0 S4 (20june)
Assignee | ||
Updated•11 years ago
|
Blocks: sms-sprint-3
Updated•11 years ago
|
blocking-b2g: 2.0? → 2.0+
status-b2g-v1.4:
--- → unaffected
status-b2g-v2.0:
--- → affected
status-b2g-v2.1:
--- → affected
Assignee | ||
Comment 22•11 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?
Flags: needinfo?(schung)
Comment 23•11 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.
Flags: needinfo?(schung)
Assignee | ||
Comment 24•11 years ago
|
||
Yeah, I believe like you then, same than bug 1021788 :)
Should we go ahead and merge this patch then?
Comment 25•11 years ago
|
||
Comment on attachment 8434162 [details] [review]
github PR
I left some comments on github, just some nits and overall looks great!
Comment 26•11 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!
Attachment #8434162 -
Flags: review?(schung) → review+
Assignee | ||
Comment 27•11 years ago
|
||
Updated, tested on the device, now waiting for a green Travis (or Gaia Try) before merging.
Assignee | ||
Comment 28•11 years ago
|
||
master: 0905395339414feb099182c65f5c912a834eed82
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•11 years ago
|
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.
Attachment #8436999 -
Flags: ui-review?(vpg) → ui-review-
Comment 30•10 years ago
|
||
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.
You need to log in
before you can comment on or make changes to this bug.
Description
•