Closed Bug 1003820 Opened 8 years ago Closed 8 years ago

[Messages] Recipients container grows without limit breaking the layout

Categories

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

x86
macOS
defect
Not set
normal

Tracking

(blocking-b2g:2.0+, b2g-v2.0 fixed)

RESOLVED FIXED
2.0 S3 (6june)
blocking-b2g 2.0+
Tracking Status
b2g-v2.0 --- fixed

People

(Reporter: joan, Assigned: steveck)

References

Details

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

Attachments

(6 files, 2 obsolete files)

When adding a lot of recipients, the container don't have a max height

1. Add a lot of recipients
2. Tap to focus on Message field
3. Swipe down on recipients container
4. The container don't have max height

See screenshots
Summary: [Messages] Limit height recipients container → [Messages] Recipients container grows without limit breaking the layout
Attached image sms_10_recipients.png
Attached image sms_lot_recipients.png
Blocks: 963018
Assignee: nobody → borja.bugzilla
Target Milestone: --- → 2.0 S1 (9may)
Probably a regression from bug 949457. I remember I wanted to check this but forgot ;)
Blocks: 949457
Keywords: regression
blocking-b2g: --- → 2.0?
triage: in this case, users are not able to type message
blocking-b2g: 2.0? → 2.0+
Target Milestone: 2.0 S1 (9may) → 2.0 S2 (23may)
Borja is currently working on other bugs so releasing "assigned" field in order to allow anyone else to pick this one up.
Assignee: borja.bugzilla → nobody
I could take a look first.
Assignee: nobody → schung
I think we can have a fixed max-height using "calc(100% - XXXrem)".
(In reply to Julien Wajsberg [:julienw] from comment #8)
> I think we can have a fixed max-height using "calc(100% - XXXrem)".

Sadly, it's not working because the container height is expanded by list, so setting percentage has no effect here. I think that's why we apply js solution for the max height computation before. I'll try any posibble css solution first.
Maybe using the vh unit then? Lile calc(100vh - XXXrem) ?
Attached file Link to github
vh unit saves my day, thanks!
Attachment #8426142 - Flags: review?(felash)
Blocks: sms-sprint-1
Whiteboard: [not-part-of-initial-sprint]
Attached image 2014-05-22-17-57-45.png (obsolete) —
Hey Victoria,

I'd like to know if the max-height is correctly set, or should it be a little smaller.
Or should we wait for the final composer refresh to decide exactly?
Attachment #8427086 - Flags: ui-review?(vpg)
Comment on attachment 8427086 [details]
2014-05-22-17-57-45.png

The to field should never overlap the input area, but only in the new message case, we don't need to leave a gap btween the to field and the input. 
Moreover, the "to" word and the "+" button should stay at the bottom of the field since the new recipient will be placed there.
Attachment #8427086 - Flags: ui-review?(vpg) → ui-review-
(In reply to Victoria Gerchinhoren [:vicky] from comment #13)
> Comment on attachment 8427086 [details]
> 2014-05-22-17-57-45.png
> 
> The to field should never overlap the input area, but only in the new
> message case, we don't need to leave a gap btween the to field and the
> input. 

So like I thought, we need the max-height to be somewhat smaller, thanks !

> Moreover, the "to" word and the "+" button should stay at the bottom of the
> field since the new recipient will be placed there.

The To/+ issue will be done in the more general bug 963018.
Comment on attachment 8426142 [details] [review]
Link to github

Removing the review request then, the value needs to be a little smaller. Maybe we should wait for bug 963018 though...
Attachment #8426142 - Flags: review?(felash)
Victoria, can you give us precisely how much space we need to leave at the bottom, to see the input correctly? Thanks!
Flags: needinfo?(vpg)
Blocks: sms-sprint-2
No longer blocks: sms-sprint-1
Whiteboard: [not-part-of-initial-sprint] → [p=1]
Target Milestone: 2.0 S2 (23may) → 2.0 S3 (6june)
(In reply to Julien Wajsberg [:julienw] from comment #16)
> Victoria, can you give us precisely how much space we need to leave at the
> bottom, to see the input correctly? Thanks!

Hi Julien,
Isn't the input comming from BBs? Anyways, the height of that input should be 3.5 rem.

Thanks!
Flags: needinfo?(vpg)
Attached image 2014-05-22-20-51-58.png (obsolete) —
Hi Vicky, I adjust the height of recipient container a little bit(considering the 3.5rem composer height you said). Does it looks better than old one?
Attachment #8429904 - Flags: ui-review?(vpg)
Comment on attachment 8429904 [details]
2014-05-22-20-51-58.png

Please, make it 3.7 rem. thanks!
Attachment #8429904 - Flags: ui-review?(vpg) → ui-review-
Attachment #8427086 - Attachment is obsolete: true
Attachment #8429904 - Attachment is obsolete: true
Attachment #8430570 - Flags: ui-review?(vpg)
Hi Vicky, since the actual height of recipient list is 4 rem, I take screenshot for both case(3.7rem and 4 rem to the bottom) and let you decide which is better.
Attachment #8430577 - Flags: ui-review?(vpg)
Attachment #8430577 - Flags: ui-review?(vpg) → ui-review-
Attachment #8430570 - Flags: ui-review?(vpg) → ui-review+
Comment on attachment 8426142 [details] [review]
Link to github

Adjust bottom space to 3.7 rem for showing composer since visual prefers this value.
Attachment #8426142 - Flags: review?(felash)
Comment on attachment 8426142 [details] [review]
Link to github

I think you need to check with a newer master because your screenshots does not take the composer refresh (landed last week) into account.

As a result, it seems 3.7rem is very too big (it's about 2 lines), from my investigations I think it should be 2.2rem instead with the current master.

Also, it seems we miss a padding or margin bottom when scrolling, because when we scroll inside the recipient panel, the last line is not aligned with the "To" line.
Attachment #8426142 - Flags: review?(felash)
(In reply to Julien Wajsberg [:julienw] from comment #23)
> Comment on attachment 8426142 [details] [review]
> Link to github
> 
> I think you need to check with a newer master because your screenshots does
> not take the composer refresh (landed last week) into account.
> 
> As a result, it seems 3.7rem is very too big (it's about 2 lines), from my
> investigations I think it should be 2.2rem instead with the current master.
> 
> Also, it seems we miss a padding or margin bottom when scrolling, because
> when we scroll inside the recipient panel, the last line is not aligned with
> the "To" line.

Thanks for reminder, the problem should be the list padding removed in the latest vr, so I made a little adjustment that having a margin for alignment.
Attachment #8426142 - Flags: review?(felash)
Duplicate of this bug: 1019947
Comment on attachment 8426142 [details] [review]
Link to github

r=me

Still there is a weird behavior: when there are enough recipients to make the recipient panel scrollable, when I swipe down the recipient panel, the recipient panel is scrolled up too.

I wonder if we can prevent it from scrolling until it's open? Maybe having "overflow: hidden" by default, and "overflow: auto" when it's open (but in that case we may have issues when we call scrollIntoView on the last element...); or maybe GestureDetector should preventDefault when it finds a gesture?

So if you have a great idea now, please do it and request another review, otherwise please file a follow-up...
Attachment #8426142 - Flags: review?(felash) → review+
In master:902b0cdfec14dd578080e1eb318872943e6dec80

Sorry I could not find a quick solution right away and it seems not apz issue either :-/ . But I have some discovery about the issue and I will note this on new created bug.
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Blocks: 1021513
No longer blocks: 1021513
Depends on: 1021513
You need to log in before you can comment on or make changes to this bug.