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

VERIFIED FIXED in Firefox OS v1.1hd

Status

P2
normal
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: jugglinmike, Assigned: jugglinmike)

Tracking

unspecified
1.1 QE4 (15jul)
x86_64
Linux

Firefox Tracking Flags

(blocking-b2g:leo+, b2g18 fixed, b2g-v1.1hd fixed)

Details

(Whiteboard: [u=commsapps-user c=messaging p=0],[LeoVB+] )

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

5 years ago
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

5 years ago
Created attachment 765275 [details]
not possible to scroll till the bottom

Comment 2

5 years ago
Adding an screenshoot. 
Below the last MMMS, there is a SMS which will not appear although the user scrolls.
seems important enough to block here.


will be fixed by patch in bug 882086
blocking-b2g: --- → leo?
this is important agreed in triage, please fix here. the other bug is polish.
blocking-b2g: leo? → leo+
Assignee: nobody → janjongboom
sorry jan, it's done i bug 882806 as written in comment 3.
Assignee: janjongboom → mike
Yeah, I see it now as well.
(Assignee)

Comment 7

5 years ago
Created attachment 769833 [details] [diff] [review]
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.
Attachment #769833 - Flags: review?(felash)

Updated

5 years ago
Priority: -- → P1
Target Milestone: --- → 1.1 QE4 (15jul)
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.
Attachment #769833 - Flags: review?(felash)
(Assignee)

Comment 9

5 years ago
Created attachment 770295 [details] [diff] [review]
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.
Attachment #769833 - Attachment is obsolete: true
Attachment #770295 - Flags: review?(felash)
How about setting:

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

Instead of using the border?
Whiteboard: [u=commsapps-user c=messaging p=0]
Created attachment 771498 [details] [diff] [review]
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.
Attachment #770295 - Attachment is obsolete: true
Attachment #770295 - Flags: review?(felash)
Attachment #771498 - Flags: review?(mike)

Updated

5 years ago
Priority: P1 → P2
(Assignee)

Comment 12

5 years ago
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`!
Attachment #771498 - Flags: review?(mike)
I will definitely fix all your nit, could you please put "r+ with nits" so that I can merge monday without another review ? :)
(Assignee)

Comment 14

5 years ago
Comment on attachment 771498 [details] [diff] [review]
patch v3

Sure thing!

r+ provided the nitpicks on the GitHub pull request are addressed
Attachment #771498 - Flags: review+
master: acae7b7e5eb6a053003cd8b894a87bfe03bcd4eb
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Uplifted acae7b7e5eb6a053003cd8b894a87bfe03bcd4eb to:
v1-train: 435454a3b2d0bd23aa26145feabe4abcdcbbb315
status-b2g18: --- → fixed

Comment 17

5 years ago
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
Status: RESOLVED → VERIFIED

Updated

5 years ago
Whiteboard: [u=commsapps-user c=messaging p=0] → [u=commsapps-user c=messaging p=0],[LeoVB+]
v1.1.0hd: 435454a3b2d0bd23aa26145feabe4abcdcbbb315
status-b2g-v1.1hd: --- → fixed
You need to log in before you can comment on or make changes to this bug.