952526 - [Messages] New sms appear outside the viewport/ below the screen

Status: RESOLVED FIXED

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

Description

[Zac C (:zac)]

STR
1. Grab a phone and a b2g phone
2. Send a message to the b2g phone
3. Open the SMS app in message thread view and leave it open
4. Send 5-6 more SMS to the b2g phone until thread view is filled

Actual: The SMS app in thread view does not scroll down to the newest received message. As there is no visual cue (ie scroll bar) to signify more content outside the viewport it is not clear there is a new message.
The status bar notification does not work when Messages app is open either so there is even less indication to the user that a new message has arrived.

Expected
Message app to scroll down to show the latest message at the bottom when a new message arrives

This bug is at least 12 days old as I found it on my dogfooding phone which has an almost 2 week old build on it. I have also verified it on today's build.


Gaia      574f79512a7b8a9ab99211b16a857ab812d7994e
Gecko     http://hg.mozilla.org/mozilla-central/rev/599100c4ebfe
BuildID   20131220040201
Version   29.0a1

Comment 1

[Julien Wajsberg [:julienw]]

So, that's expected we don't scroll, but you should see a notice that looks like https://bug905208.bugzilla.mozilla.org/attachment.cgi?id=810514 instead (implemented in v1.3).

Do you see this notice sometimes? Is there a specific situation that you don't see it?

Comment 2

[Julien Wajsberg [:julienw]]

Ok, I get it, we don't get the notice if we havent scrolled at all yet. And this happens if the thread is too small to scroll when we open the thread.

Comment 4

[Jason Smith [:jsmith]]

Moving over flags from the dupe.

Comment 5

[Gabriele Svelto [:gsvelto]]

Note that we did scroll to the new message in the past unless the user had explicitly scrolled up. The behavior should basically be:

- If the user is already at the bottom we display the new message and scroll it into view

- If the user has manually scrolled up we assume he/she is reading some old message and we don't want to distract him/her by scrolling down so we display a notification instead

Looking at the changelog it seems to me that we regressed in bug 929919, see this change that removed scrolling to the new message:

https://github.com/mozilla-b2g/gaia/commit/8115e89f4bd5b1f3cb68545f48aa20bb2d7dbe07#diff-74841e428dea67f7357f1893a4d26155L484

Taking this, I'll try and cook up a fix, it should be pretty easy.

Comment 6

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH] Fix automatic scrolling of the message view when a new SMS is received

This patch fixes the problem and introduces a couple of unit tests to make sure we don't regress anymore on this behavior.

Comment 7

[Gabriele Svelto [:gsvelto]]

Attachment: [PULL REQUEST] Fix automatic scrolling of the message view when a new SMS is received

Comment 8

[Rick Waldron [:rwaldron]]

Comment on attachment 8376698 [details] [diff] [review]
[PATCH] Fix automatic scrolling of the message view when a new SMS is received

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

Taking this review to lighten the load—everything looks good to me, nice and easy fix. Works well on device. Good tests :) r+

Comment 9

[Gabriele Svelto [:gsvelto]]

(In reply to Rick Waldron [:rwaldron] from comment #8)
> Taking this review to lighten the load—everything looks good to me, nice and
> easy fix. Works well on device. Good tests :) r+

Thanks a lot Rick! The Travis run was green: https://travis-ci.org/mozilla-b2g/gaia/builds/18928051 so pushed to gaia/master 61a9ba7dd6006a21a8bde4fd4c5d52c23caebf78

Comment 10

[Gabriele Svelto [:gsvelto]]

Comment on attachment 8376698 [details] [diff] [review]
[PATCH] Fix automatic scrolling of the message view when a new SMS is received

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): regressing bug 929919
[User impact] if declined: if the user receives a new message in the thread he's looking at, the view won't be scrolled down to show him the new message if he's already at the bottom of the thread (which was the standard behavior prior to bug 929919)
[Testing completed]: Landed and tested on master using both a device and the emulator, unit-tests were added to cover this scenario
[Risk to taking this patch] (and alternatives if risky): Practically none, it's a one-line fix which adds back a line that was erroneously removed
[String changes made]: none

Comment 11

[Julien Wajsberg [:julienw]]

Thanks to you both :)

Comment 12

[Ryan VanderMeulen [:RyanVM]]

v1.3: 38519ceb6ab5c46c35a3fae3a70f5f26e4b3c59c

Comment 15

[bhavana bajaj [:bajaj]]

https://github.com/mozilla-b2g/gaia/commit/38519ceb6ab5c46c35a3fae3a70f5f26e4b3c59c - merged on 1.3t as well.