Closed Bug 1076729 Opened 5 years ago Closed 5 years ago

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

Categories

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

All
Gonk (Firefox OS)
defect
Not set

Tracking

(tracking-b2g:backlog, b2g-v2.0 affected, b2g-v2.1 affected, b2g-v2.2 fixed)

RESOLVED FIXED
tracking-b2g backlog
Tracking Status
b2g-v2.0 --- affected
b2g-v2.1 --- affected
b2g-v2.2 --- fixed

People

(Reporter: hi-kawashima, Assigned: julienw)

References

Details

(Whiteboard: [Tako_Blocker])

Attachments

(1 file)

46 bytes, text/x-github-pull-request
steveck
: review+
Details | Review
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.
Dear Julien,

How do you think about this behavior?

Regards.
Flags: needinfo?(felash)
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)?
Flags: needinfo?(felash)
Keywords: qawanted
QA Contact: 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
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(jmitchell)
Keywords: qawanted
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(jmitchell)
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
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?
Flags: needinfo?(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
Flags: needinfo?(ddixon)
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.
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"
blocking-b2g: --- → 2.1?
Whiteboard: [Tako_Blocker]
Olof, can you or someone from your team please test the patch in bug 983172?
Flags: needinfo?(olof.wickstrom)
Hi Julien, unfortunately I can't build the code myself. I'll check with the team.
Flags: needinfo?(olof.wickstrom)
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.
blocking-b2g: 2.1? → -
(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
Flags: needinfo?(felash)
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.
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 :)
Attached file github PR
I still need to try with a thread with lots of MMS
Assignee: nobody → felash
Flags: needinfo?(felash)
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.
Attachment #8511865 - Flags: review?(schung)
(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?
(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.
Flags: needinfo?(btseng)
(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.
Flags: needinfo?(btseng)
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.
Attachment #8511865 - Flags: review?(schung) → review+
Filed bug 1091441 to improve the reflow and free my mind ;)
master: 29625c4450f9a3c95c3eb8a287dbc0852818e7b1

thanks !
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Blocks: 1088026
Olof, I'd appreciate if you can add me in the Cc list for this bug. Thanks !
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?
Flags: needinfo?(felash)
Partner claims this fails their UX criteria. 
We should evaluate the efforts and risk to patch on v2.1.
blocking-b2g: - → backlog
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.
Flags: needinfo?(felash)
hi,

since 983172 is fixed, it should be low risk to include this fix.
ni Fabrice and bhavana for uplifting approval.
Flags: needinfo?(fabrice)
Flags: needinfo?(bbajaj)
983172 is not in v2.1.
i know that 983172 is landed to m-c, then i guess i have to request 983172 to 2.1 as well.
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.
(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.
Flags: needinfo?(fabrice) → needinfo?(pmathur)
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)
QA Whiteboard: [QAnalyst-Triage+]
Flags: needinfo?(pmathur)
Keywords: qawanted
QA Contact: 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
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(jmitchell)
Keywords: qawanted
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)
Keywords: qawanted
NI? for Duane to review comment 34.
Flags: needinfo?(ddixon)
: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
Flags: needinfo?(bbajaj)
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
Flags: needinfo?(ddixon)
Keywords: qawanted
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
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(jmitchell)
blocking-b2g: backlog → ---
You need to log in before you can comment on or make changes to this bug.