1076729 - [MMS]it is not scrolled to the most recent message.

Status: RESOLVED FIXED

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

Description

[hikawashima]

Steps to reproduce:
1. receive 20 messages(mms).
   ※mms message must have many attachments and many new lines.
2. "thread list" screen > "chat" screen

Actual results:
it is not scrolled to the most recent message in "chat" screen.

Expected results:
it is scrolled to the most recent message in "chat" screen.

Comment 1

[hikawashima]

Dear Julien,

How do you think about this behavior?

Regards.

Comment 2

[Julien Wajsberg [:julienw]]

I know we used to have this annoying issue but this may be fixed in v2.2 (bug 983172).

QA, can you please try this on various branches (1.4 to v2.2)?

Comment 3

[Duane Dixon [:ddixon]]

Branch Check

STR: 
1. Send DUT at least 5 text messages (some with images). 
2. Access received message via tapping on Notification and/or opening Messages app. 

Actual Results: 
Messages app does not automatically scroll to the newest message in the conversation after it is received. 

Issue DOES occur in Flame 2.1, 2.0, 2.0 Base Image.  (Shallow Flash) 

Device: Flame 2.1
Build ID: 20141007075118
Gaia: da328c6cbabf2cffc2d362e282cacc93325d1f43
Gecko: aebe54593d60
Version: 34.0a2 
Firmware Version: v180
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0
--------------------------------------------------
Device: Flame 2.0
Build ID: 20141007065153
Gaia: 31a49c7932c7085961760a6bef9ed381ea38d7e3
Gecko: 390acb9d220d
Version: 32.0 (2.0)
Firmware Version: v180
User Agent: Mozilla/5.0 (Mobile; rv:32.0) Gecko/32.0 Firefox/32.0
--------------------------------------------------
Device: Flame 2.0 (base image)
Gaia: 506da297098326c671523707caae6eaba7e718da
Gecko: 2b27becae85092d46bfadcd4fb5605e82e1e1093
Version: 32.0 (2.0)
Firmware Version: v180
User Agent: Mozilla/5.0 (Mobile; rv:32.0) Gecko/32.0 Firefox/32.0
--------------------------------------------------
--------------------------------------------------
Issue DOES NOT occur in Flame 2.2

Device: Flame Master
Build ID: 20141007054552
Gaia: 9306e431d5b559035d9656902e413816b6ca3c26
Gecko: 25a98bac9264
Version: 35.0a1 (Master)
Firmware Version: v180
User Agent: Mozilla/5.0 (Mobile; rv:35.0) Gecko/35.0 Firefox/35.0

Comment 4

[Duane Dixon [:ddixon]]

Correction:  Issue DOES occur in Flame 2.2 (Shallow Flash). 

Device: Flame Master
Build ID: 20141007054552
Gaia: 9306e431d5b559035d9656902e413816b6ca3c26
Gecko: 25a98bac9264
Version: 35.0a1 (Master)
Firmware Version: v180
User Agent: Mozilla/5.0 (Mobile; rv:35.0) Gecko/35.0 Firefox/35.0

Comment 5

[Julien Wajsberg [:julienw]]

Duane, would you say the problem is different/not as visible in v2.2? Just want to check something's been fixed.

Also, note that the STR was more about "clicking on the thread in the thread list -> loading the messages in the thread" than clicking on a notification. Thanks for this though, this is useful, but can you try again with the intended STR?

Comment 6

[Duane Dixon [:ddixon]]

Julien,
Yes, the issue seems fixed when following the intended STR. 

Actual Results: Messages app automatically scrolls to most recent message after user taps on the conversation or "chat" thread. 

Environmental Variables for 2.2 build: 

Device: Flame Master
Build ID: 20141008065408
Gaia: 2665e714beea5dc433862ca6bb8d2b47ffe2f2d1
Gecko: 4bad24a306b2
Version: 35.0a1 (Master)
Firmware Version: v180
User Agent: Mozilla/5.0 (Mobile; rv:35.0) Gecko/35.0 Firefox/35.0

Comment 7

[Julien Wajsberg [:julienw]]

OK thanks.

We'll keep this bug open to look the notification issue further. I've also noticed that the scroll "jumps" if there is some MMS with attachments in the thread, this is wrong as well.

Comment 8

[Olof Wickström]

Major impact on messaging usability. The inflow of messages is large and many users don't care about old messages. They need to see the latest, i.e. most relevant, information first.

"Major Dialer, SMS, and VM communication issues"

Comment 9

[Julien Wajsberg [:julienw]]

Olof, can you or someone from your team please test the patch in bug 983172?

Comment 10

[Olof Wickström]

Hi Julien, unfortunately I can't build the code myself. I'll check with the team.

Comment 11

[Wilfred Mathanaraj [:WDM]]

this implementation was by design and we have plans to change it as you can see from the patch-

lets first test patch and then we can see if this can be picked just for tako. it will not block 2.1 general release.

Comment 12

[Olof Wickström]

(In reply to Julien Wajsberg [:julienw] from comment #9)
> Olof, can you or someone from your team please test the patch in bug 983172?

Hi Julien!

We managed to move the patches to Tako 2.1 branch. But we can't verify them as this branch currently do not support data over mobile. This as we are waiting for a delivery from another company. 
We have investigated if we can move the patches to Tako 2.0 branch but that seems to be a lot of work (different file structures and massive difference in the code). 

So please verify on Flame 2.1 so we can merge it.

Best regards

Comment 13

[Julien Wajsberg [:julienw]]

Olof, I think the behavior is not perfect with this patch yet and I'd really want your opinion on this...

I can try to do a patch for v2.0 by taking some other patches. Keeping my NI.

Comment 14

[Julien Wajsberg [:julienw]]

So I think the patch in bug 983172 does not resolve this completely.

I have a simple patch that builds on top of the patch for bug 1071514 and makes this a lot better. Still not perfect but I think it's nearly perfect :)

Comment 15

[Julien Wajsberg [:julienw]]

Attachment: github PR

I still need to try with a thread with lots of MMS

Comment 16

[Julien Wajsberg [:julienw]]

Comment on attachment 8511865 [details] [review]
github PR

Hey Steve,

can you look at the latest commit?

It's still not perfect, especially:
* we clearly see the reflow when the MMS content is "appended" to the DOM, but I don't see how to improve this. In smil.js everything is synchronous except the text reader part, so maybe we could try to append attachments synchronously and reserve some space for the text according to the text blob.size property?
* when testing in Firefox Nightly, I see it's sometimes still not scrolled to the bottom, but on Buri I never saw the behavior.

But it's still a lot better than the current behavior IMO.

Comment 17

[Steve Chung [:steveck]]

(In reply to Julien Wajsberg [:julienw] from comment #16) 
> It's still not perfect, especially:
> * we clearly see the reflow when the MMS content is "appended" to the DOM,
> but I don't see how to improve this. In smil.js everything is synchronous
> except the text reader part, so maybe we could try to append attachments
> synchronously and reserve some space for the text according to the text
> blob.size property?

It's really a good question about how to reduce reflow in this case. About your synchronous message append idea, not sure if it's easy to to reserve space properly, since the char width might affect the result. I think it's still not easy to predict the size of the message bubble correctly. Another idea is to have another async way to append the mms message node, and use promise.all to append all the mms message at once when we need to show the chunk, wdyt?

Comment 18

[Julien Wajsberg [:julienw]]

(In reply to Steve Chung [:steveck] from comment #17)
> (In reply to Julien Wajsberg [:julienw] from comment #16) 
> > It's still not perfect, especially:
> > * we clearly see the reflow when the MMS content is "appended" to the DOM,
> > but I don't see how to improve this. In smil.js everything is synchronous
> > except the text reader part, so maybe we could try to append attachments
> > synchronously and reserve some space for the text according to the text
> > blob.size property?
> 
> It's really a good question about how to reduce reflow in this case. About
> your synchronous message append idea, not sure if it's easy to to reserve
> space properly, since the char width might affect the result. I think it's
> still not easy to predict the size of the message bubble correctly. Another
> idea is to have another async way to append the mms message node, and use
> promise.all to append all the mms message at once when we need to show the
> chunk, wdyt?

We can try this; but maybe we'll still see the reflow when we append the mms message?
Another possibility is to prevent appending the previous ones until the more recent message is appended.
While we're here, I wonder if we could call the "cursor.continue" before we even try to append the thread to the DOM. Maybe we could load the thread quicker?

And a last (imo simpler) possibility is to make gecko replace the text/plain blob by a string when the message is received, or when it's retrieved. NI Bevis to know if this is doable.
If we replace it in the DB, we'll likely need migration, we could have a "on demand" migration: when we retrieve the message the first time it's converted.

Comment 19

[Bevis Tseng[:bevis][:btseng](Exited)]

(In reply to Julien Wajsberg [:julienw] from comment #18)
> (In reply to Steve Chung [:steveck] from comment #17)
> > (In reply to Julien Wajsberg [:julienw] from comment #16) 
> > > It's still not perfect, especially:
> > > * we clearly see the reflow when the MMS content is "appended" to the DOM,
> > > but I don't see how to improve this. In smil.js everything is synchronous
> > > except the text reader part, so maybe we could try to append attachments
> > > synchronously and reserve some space for the text according to the text
> > > blob.size property?
> > 
> > It's really a good question about how to reduce reflow in this case. About
> > your synchronous message append idea, not sure if it's easy to to reserve
> > space properly, since the char width might affect the result. I think it's
> > still not easy to predict the size of the message bubble correctly. Another
> > idea is to have another async way to append the mms message node, and use
> > promise.all to append all the mms message at once when we need to show the
> > chunk, wdyt?
> 
> We can try this; but maybe we'll still see the reflow when we append the mms
> message?
> Another possibility is to prevent appending the previous ones until the more
> recent message is appended.
> While we're here, I wonder if we could call the "cursor.continue" before we
> even try to append the thread to the DOM. Maybe we could load the thread
> quicker?
> 
> And a last (imo simpler) possibility is to make gecko replace the text/plain
> blob by a string when the message is received, or when it's retrieved. NI
> Bevis to know if this is doable.
> If we replace it in the DB, we'll likely need migration, we could have a "on
> demand" migration: when we retrieve the message the first time it's
> converted.

It's doable but some DOM API change for MmsAttachment or MmsMessage is needed.
I still prefer to treat all mms attachment equally from gecko's perspective.

Comment 20

[Steve Chung [:steveck]]

Comment on attachment 8511865 [details] [review]
github PR

Besides the reflow, this small patch is pretty neat and improved a lot, thanks :)
r=me but you might need to resolved conflicts first.

Comment 21

[Julien Wajsberg [:julienw]]

Filed bug 1091441 to improve the reflow and free my mind ;)

Comment 22

[Julien Wajsberg [:julienw]]

master: 29625c4450f9a3c95c3eb8a287dbc0852818e7b1

thanks !

Comment 23

[Julien Wajsberg [:julienw]]

Olof, I'd appreciate if you can add me in the Cc list for this bug. Thanks !

Comment 24

[Francis Lee [:frlee]]

hi Olof,

please add julien and me to the bug 1088026. i can't access it either.

hi Julien,

could you please let me know the risk to uplift your patch to 2.1?

Comment 25

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

Partner claims this fails their UX criteria. 
We should evaluate the efforts and risk to patch on v2.1.

Comment 26

[Julien Wajsberg [:julienw]]

Wesley, yeah, see bug 1091441 to improve this more (with a plan!). We felt it was better with this patch than without, but it's still not good enough.

Francis: the patch itself has no risk, but I'm not sure it will have an effect without bug 983172. Maybe yes, maybe no, someone needs to try.

Comment 27

[Francis Lee [:frlee]]

hi,

since 983172 is fixed, it should be low risk to include this fix.
ni Fabrice and bhavana for uplifting approval.

Comment 28

[Julien Wajsberg [:julienw]]

983172 is not in v2.1.

Comment 29

[Francis Lee [:frlee]]

i know that 983172 is landed to m-c, then i guess i have to request 983172 to 2.1 as well.

Comment 30

[Julien Wajsberg [:julienw]]

983172 is considerably more risky though.

I think we should try this bug only on v2.1 and see if this makes things better. I don't have time to test it though so please ask the partners to do it.

Comment 31

[[:fabrice] Fabrice Desré]

(In reply to Julien Wajsberg [:julienw] from comment #30)
> 983172 is considerably more risky though.
> 
> I think we should try this bug only on v2.1 and see if this makes things
> better. I don't have time to test it though so please ask the partners to do
> it.

Right, I think QA can test that.

Comment 32

[Joshua Mitchell (Inactive)]

Qa-Wanted: to test the patch from bug 983172 on top of the latest 2.1 build and see if it fixes THIS issue. (please thoroughly halo-test to ensure no fall-out)

Comment 33

[Duane Dixon [:ddixon]]

Applying the patch "bug-983172" to the latest Flame 2.1 build causes the device to malfunction. 

Actual Results: Only 3 apps are displayed at the homescreen: Music, Games, Social.  Text messages can be received, but they are not displayed when the conversation page is opened.  

Note: I also tested an earlier Flame 2.2 build (listed below) and observed the same result.  In conclusion, patch "bug-983172" causes the phone to malfunction when applied. 

(Full Flash, nightly, 319 MB memory)
Device: Flame 2.2
BuildID: 20141028040202
Gaia: 4391acf4f3b5c38b9803adba758995708c52e342
Gecko: a255a234946e
Gonk: 6e51d9216901d39d192d9e6dd86a5e15b0641a89
Version: 36.0a1 (2.2)
Firmware: V188-1
User Agent: Mozilla/5.0 (Mobile; rv:36.0) Gecko/36.0 Firefox/36.0

Comment 34

[Julien Wajsberg [:julienw]]

Let's try again.

What we really want to know is if the patch in _this_ bug (without the patch from bug 983172) fixes the issue in v2.1 without side effects.

Thanks.

(On a side note, I don't think the patch from bug 983172 can change anything in the homescreen)

Comment 35

[Cody Roesch [:croesch]]

NI? for Duane to review comment 34.

Comment 36

[bhavana bajaj [:bajaj]]

:frlee, I am leaning towards not fixing this given the risk and keeping comment #11 in mind. Nevertheless lets wait for QA testing to confirm Julien's patch and re-evaluate this

Comment 37

[Duane Dixon [:ddixon]]

Issue appears to be fixed with patch "1076729-scroll-mms-correctly" applied to latest Flame 2.2 build (shallow flash, 319 MB, eng. build). 

Actual Results: Messages app scrolls to latest message in conversation thread.  

Environmental Variables:
Device: Flame 2.2
Build ID: 20141117132855
Gaia: 84e3398795a638f99f45e06b7a8a61e216a3b34c
Gecko: 2d0a51ef828d
Version: 36.0a1 (2.2)
Firmware Version: v188
User Agent: Mozilla/5.0 (Mobile; rv:36.0) Gecko/36.0 Firefox/36.0

Comment 38

[Duane Dixon [:ddixon]]

Actual Results for Flame 2.1 (shallow flash, 319 MB, eng. build) are the same as the above comment (Comment 37). 

Device: Flame 2.1
BuildID: 20141117201226
Gaia: 84e3398795a638f99f45e06b7a8a61e216a3b34c
Gecko: 95fbd7635152
Version: 34.0 (2.1)
Firmware: V188
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0