875362 - [MMS] Cannot scroll to bottom of thread list as input field grows

Status: VERIFIED FIXED

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

Description

[Mike Pennisi [:jugglinmike]]

As the input field expands to accommodate user input, the underlying thread view is partially obscured. Although it is still possible to scroll the thread view in this state, it is impossible to scroll to the bottom of the thread view.

Comment 1

[isabelrios]

Attachment: not possible to scroll till the bottom

Comment 2

[isabelrios]

Adding an screenshoot. 
Below the last MMMS, there is a SMS which will not appear although the user scrolls.

Comment 3

[Julien Wajsberg [:julienw]]

seems important enough to block here.


will be fixed by patch in bug 882086

Comment 4

[Dietrich Ayala (:dietrich)]

this is important agreed in triage, please fix here. the other bug is polish.

Comment 5

[Julien Wajsberg [:julienw]]

sorry jan, it's done i bug 882806 as written in comment 3.

Comment 6

[Jan Jongboom [:janjongboom] (inactive)]

Yeah, I see it now as well.

Comment 7

[Mike Pennisi [:jugglinmike]]

Attachment: Prevent message occlusion as Compose field grows

As requested, I am moving the work originally started in Bug 882086 to this bug.

Julien: that while implementing your feedback, I found another edge case. The "Edit" mode menu was occluding the final message. This patch addresses that case.

Comment 8

[Julien Wajsberg [:julienw]]

Comment on attachment 769833 [details] [diff] [review]
Prevent message occlusion as Compose field grows

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

::: apps/sms/js/thread_ui.js
@@ +519,5 @@
> +      // The height of the absolutely-position sub-header element
> +      this.subheader.offsetHeight +
> +      // The vertical margin of the input field
> +      this.INPUT_MARGIN;
> +    var viewHeight, tmpBorderWidth;

nit: since we don't do any calculation to add values to these vars, I'd rather have them at the very top of the function, for readability sake.

@@ +536,5 @@
> +    // the client height would be *without* the overridden bottom border.
> +    tmpBorderWidth = this.container.style.borderBottomWidth;
> +    this.container.style.borderBottomWidth = null;
> +    viewHeight = this.container.offsetHeight;
> +    this.container.style.borderBottomWidth = tmpBorderWidth;

this will trigger a sync reflow, I don't like that ;)

as discussed on IRC, please try to find something else here.

@@ +1213,5 @@
> +
> +    // Ensure the Edit Mode menu does not occlude the final messages in the
> +    // thread.
> +    this.container.style.borderBottomWidth =
> +      this.editForm.getElementsByTagName('menu')[0].offsetHeight + 'px';

use querySelector instead of getElementsByTagName

I don't like having this here but I don't have any better idea.

::: apps/sms/style/sms.css
@@ +332,5 @@
>    text-transform: uppercase;
>  }
>  
>  .article-list[data-type="list"] > ul:last-child {
> +  margin-bottom: 1.5rem;

<li> already have 1rem, therefore you should add .5rem here to achieve 1.5rem overall.

Comment 9

[Mike Pennisi [:jugglinmike]]

Attachment: Prevent message occlusion as Compose field grows v2

This version incorporates most of Julien's feedback in comment 8. Specifically, I have not been able to resolve this inefficiency:

(In reply to Julien Wajsberg [:julienw] from comment #8)
> @@ +536,5 @@
> > +    // the client height would be *without* the overridden bottom border.
> > +    tmpBorderWidth = this.container.style.borderBottomWidth;
> > +    this.container.style.borderBottomWidth = null;
> > +    viewHeight = this.container.offsetHeight;
> > +    this.container.style.borderBottomWidth = tmpBorderWidth;
> 
> this will trigger a sync reflow, I don't like that ;)
> 
> as discussed on IRC, please try to find something else here.

I have not been able to find a more efficient solution to this problem. I have noticed a runtime CSS error, and I suspect it may be the root cause of the bug you identified in https://bugzilla.mozilla.org/show_bug.cgi?id=882086#c9 (and addressing this error may be the more efficient solution we're looking for).

Using the following steps:

1) enter many (10+) lines of input into the Compose field
2) Dismiss the Compose field by tapping on the "sliver" of space afforded to the underlying thread
3) Re-expose the Compose field by tapping on it.

The following error occurs:

> E/GeckoConsole(13390): [JavaScript Warning: "Error in parsing value for 'height'.  Declaration dropped." {file: "app://sms.gaiamobile.org/index.html" line: 0}]

(Note that this message does not specify a useful line number or an originating JavaScript file.)

This error does not occur on `master`; it is somehow related to setting the `borderBottomWidth` property of Messages `container` element. I have identified three locations where the CSS "height" attribute is set via JavaScript, but the error persists if it is removed.

Julien: Feel free to block on this detail, but I do not expect I will have time to diagnose and resolve this until next week.

Comment 10

[Jan Jongboom [:janjongboom] (inactive)]

How about setting:

this.container.style.height = 'calc(100% - 5rem - ' + newHeight + 'px)'

Instead of using the border?

Comment 11

[Julien Wajsberg [:julienw]]

Attachment: patch v3

see also PR at https://github.com/mozilla-b2g/gaia/pull/10803

I kept you as author because, well, I haven't added much.

Comment 12

[Mike Pennisi [:jugglinmike]]

Comment on attachment 771498 [details] [diff] [review]
patch v3

Hey Julien,

I left some feedback on the GitHub pull request (I didn't realize that you also upload a patch file until just now). I especially like the work you've done to streamline `updateInputHeight`!

Comment 13

[Julien Wajsberg [:julienw]]

I will definitely fix all your nit, could you please put "r+ with nits" so that I can merge monday without another review ? :)

Comment 14

[Mike Pennisi [:jugglinmike]]

Comment on attachment 771498 [details] [diff] [review]
patch v3

Sure thing!

r+ provided the nitpicks on the GitHub pull request are addressed

Comment 15

[Julien Wajsberg [:julienw]]

master: acae7b7e5eb6a053003cd8b894a87bfe03bcd4eb

Comment 16

[John Ford [:jhford] CET/CEST Berlin Time]

Uplifted acae7b7e5eb6a053003cd8b894a87bfe03bcd4eb to:
v1-train: 435454a3b2d0bd23aa26145feabe4abcdcbbb315

Comment 17

[isabelrios]

Verified with 07/09 unagi v1-train build: 
Gecko-e78450a
Gaia-e251ee6
Ref ril

It is possible to scroll in order to see all the messages in the thread

Comment 18

[John Ford [:jhford] CET/CEST Berlin Time]

v1.1.0hd: 435454a3b2d0bd23aa26145feabe4abcdcbbb315