[Messages] the thread view is flashing while loading if there are MMS

RESOLVED FIXED in Firefox OS v2.2

Status

Firefox OS
Gaia::SMS
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: julienw, Assigned: steveck, Mentored)

Tracking

unspecified
2.2 S4 (23jan)
x86_64
Linux
Dependency tree / graph

Firefox Tracking Flags

(b2g-v2.1 affected, b2g-v2.2 fixed, b2g-master fixed)

Details

(Whiteboard: [sms-most-wanted][lang=js][p=2][sms-sprint-2.2S4])

Attachments

(2 attachments)

(Reporter)

Description

4 years ago
Follow-up to bug 1076729: when we load a thread that has a MMS, there are reflows happening that are quite visually visible.

The idea we have is:
* in renderMessages, we don't call appendMessage directly, but rather we push the message to be displayed to a FIFO. If the FIFO was empty, we start the rendering process.
* appendMessage will return a promise, so that we know when all the content (including MMS content) is appended
* messages will be appended one by one, taken from the FIFO, until the FIFO is empty.
* we can experiment starting the rendering process only after 100ms so that we can try to append several messages at once, and see how it looks like. (could be a separate bug)

Another approach is looking if we have some slow CSS here (especially last-child and nth-child) that can be a culprit. We have especially a div:last-child somewhere in our CSS, maybe that's making things bad here.
(Reporter)

Updated

4 years ago
Whiteboard: [sms-most-wanted]
(Reporter)

Comment 1

4 years ago
Making this a mentored bug. It's not that easy but it's an interesting one, and is quite well constrained. Please take if you already did some other bugs though.
Mentor: felash
Whiteboard: [sms-most-wanted] → [sms-most-wanted][lang=js]
(Reporter)

Updated

4 years ago
Blocks: 1112127
Whiteboard: [sms-most-wanted][lang=js] → [sms-most-wanted][lang=js][p=2]
Target Milestone: --- → 2.2 S3 (9jan)
(Assignee)

Comment 2

4 years ago
I just made some quick experiment about the flashing. I think the major issue is we abuse the ThreadUI.scrollViewToBottom while parsing mms. If we don't scrollViewToBottom after mms content ready, the scroll position would be wrong but the message thread will no flashing. If we can moving the appendMessage to promise and handle the scrolling all at once, the flashing issue should be reduced obviously.
Assignee: nobody → schung
(Reporter)

Comment 3

4 years ago
Actually, here is exactly what provokes the issue:

when we insert a MMS, because we insert the content asynchronously, the page mecanically scrolls up when we insert the content, and when we call scrollViewToBottom, it scrolls down again. This is the flash we see.

This also happens because we insert more messages in the same time (because the MMS content is asynchronous, again).

That's why the fix, in my opinion, is to wait that the whole content is appended for a MMS, before appending the next message. And we can call scrollViewToBottom only when the content is appended.

Let's see what you'll come with :)
(Assignee)

Comment 4

4 years ago
Created attachment 8541496 [details] [review]
Link to github

Hi Oleg, it's a WIP for reducing the flashing. Basically we'll only scroll once while showing the first chunk, and the rest od the item will be added to hidden items. So the overall result should look better than orignal one.

However, since I fixed the chunk issue(the chunk size has no effect originally), we will have more chances to show the next chunk, but the effect of showing the next chunk is really unpleasant and Jenny thought we should improve this as well. I didn't get any good idea or this yet, so any suggestion is wellcome!
Attachment #8541496 - Flags: feedback?(azasypkin)
Comment on attachment 8541496 [details] [review]
Link to github

Thread panel looks much better now! Left just few nits at Github.

Thanks!
Attachment #8541496 - Flags: feedback?(azasypkin) → feedback+
(Reporter)

Comment 6

4 years ago
Comment on attachment 8541496 [details] [review]
Link to github

I don't really like the approach :/

I like the effect though, so I'm fine if we want to land this approach for v2.2 and rework it then.
Attachment #8541496 - Flags: feedback-
(Assignee)

Comment 7

4 years ago
Comment on attachment 8541496 [details] [review]
Link to github

Thanks for all the feedback! I made an adjustment based on your suggestions. I don't think using TaskRunner would be much simpler(if we want to hanle the chunk showing properly) then having a promise array for scrolling to bottom. I left the taskRunner WIP in github as well, and maybe you have other suggestion for it.
Attachment #8541496 - Flags: feedback?(felash)
Attachment #8541496 - Flags: feedback?(azasypkin)
Attachment #8541496 - Flags: feedback-
Attachment #8541496 - Flags: feedback+
(Reporter)

Comment 8

4 years ago
Comment on attachment 8541496 [details] [review]
Link to github

I left the feedback for the Taskrunner WIP in a comment. If it doesn't work correctly please move forward with whatever solution ! I'll let you and Oleg discuss about this, as I won't have the time to look at it more thoroughly.
Attachment #8541496 - Flags: feedback?(felash)
Comment on attachment 8541496 [details] [review]
Link to github

Looks good, left few suggestions at Github.

Thanks!
Attachment #8541496 - Flags: feedback?(azasypkin) → feedback+
(Assignee)

Comment 10

4 years ago
Created attachment 8545740 [details] [review]
Link to github(taskRunner version)

Hi Oleg, it's taskRunner version patch with mostly unit tests changes, the taskRunner is mainly used for first chunck like you suggested. Sorry that wasting your time on review previous version :(
Attachment #8545740 - Flags: review?(azasypkin)
(Assignee)

Comment 11

4 years ago
Add more fix per previous comments from old branch. Sorry for the missing and these suggestions are really good.
Comment on attachment 8545740 [details] [review]
Link to github(taskRunner version)

Hey Steve, I've left several suggestions for the unit tests, but the rest of the code looks pretty good to me!

Thanks!
Attachment #8545740 - Flags: review?(azasypkin)
(Assignee)

Comment 13

4 years ago
Comment on attachment 8545740 [details] [review]
Link to github(taskRunner version)

More unit tests fixing, thanks for the detailed review :)
Attachment #8545740 - Flags: review?(azasypkin)
Hey Steve, sorry for a long review (I've left few more minor unit test nits), now everything looks good, except for the issue I've just noticed:

If you enter MMS thread and quickly go back to thread list and then navigate to another thread - threads can get mixed. Could you please take a look?

See video: https://www.youtube.com/watch?v=pXmdFmMnQFI&feature=youtu.be
Flags: needinfo?(schung)
(Assignee)

Comment 15

4 years ago
Ah, that's a nice catch! Since message appending becomes async, the original stoprendering checking can't prevent the queued appending action and ongoing dom element building. I'll add more checking to avoid this issue, thanks!
Flags: needinfo?(schung)
(Assignee)

Comment 16

4 years ago
Patch updated, hope this solution won't introduce other side effects(but it will need initializeRendering for unit tests at first)
Flags: needinfo?(azasypkin)
Comment on attachment 8545740 [details] [review]
Link to github(taskRunner version)

Thanks for the great work (minor suggestion at github)!

The last thing is that I see security exceptions like [1] in adb log when I enter MMS thread and almost immediately leave it (with a small delay):

"I/Messages(16320): Security Error: Content at app://sms.gaiamobile.org/index.html may not load data from blob:app://sms.gaiamobile.org/e07e3ce1-1e5f-4771-9af2-56db81623ebf."

Do you see that too?
Flags: needinfo?(azasypkin) → needinfo?(schung)
Attachment #8545740 - Flags: review?(azasypkin) → review+
(Assignee)

Comment 18

4 years ago
(In reply to Oleg Zasypkin [:azasypkin] from comment #17)
> Comment on attachment 8545740 [details] [review]
> Link to github(taskRunner version)
> 
> Thanks for the great work (minor suggestion at github)!
> 
> The last thing is that I see security exceptions like [1] in adb log when I
> enter MMS thread and almost immediately leave it (with a small delay):
> 
> "I/Messages(16320): Security Error: Content at
> app://sms.gaiamobile.org/index.html may not load data from
> blob:app://sms.gaiamobile.org/e07e3ce1-1e5f-4771-9af2-56db81623ebf."
> 
> Do you see that too?

Ya, I can see those log too... I didn't notice it before you pointed out. These blob should be attachment thumbnail blob, which is set before stop rendering and back. Not sure if it's harmful, but I'll create a follow up and raise this question in gaia channel.
Flags: needinfo?(schung)
(Assignee)

Comment 19

4 years ago
In master : https://github.com/mozilla-b2g/gaia/commit/003fda1d23a60107b9ae72135898ea4fc76cc251
Will create a follow up for comment 17 later(ni? myself to create bug).
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Flags: needinfo?(schung)
Resolution: --- → FIXED
(Reporter)

Updated

4 years ago
Whiteboard: [sms-most-wanted][lang=js][p=2] → [sms-most-wanted][lang=js][p=2][sms-sprint-2.2S4]
(Reporter)

Updated

4 years ago
Target Milestone: 2.2 S3 (9jan) → 2.2 S4 (23jan)
(Reporter)

Comment 20

4 years ago
Steve, can you please request an approval for 2.2 as well?
(Assignee)

Comment 21

4 years ago
Comment on attachment 8545740 [details] [review]
Link to github(taskRunner version)

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): Bug 875338(Issue before 2.0)
[User impact] if declined: Unpleasant flashing while entering the thread contains lots of mms attachments.
[Testing completed]: Yes
[Risk to taking this patch] (and alternatives if risky): Low
[String changes made]: N/A
Flags: needinfo?(schung)
Attachment #8545740 - Flags: approval-gaia-v2.2?
(Assignee)

Updated

4 years ago
Blocks: 1125723
(Assignee)

Comment 22

4 years ago
Create a follow up bug in bug 1125723(see comment 17), and that's why I didn't ask for approval at first. But please note that this doesn't mean user will face any unexpected behavior, it just a error log and we're still not sure why it happens.

Updated

4 years ago
Attachment #8545740 - Flags: approval-gaia-v2.2? → approval-gaia-v2.2+
Needs rebasing for v2.1/v2.2 uplift.
status-b2g-v2.1: --- → affected
status-b2g-v2.2: --- → affected
status-b2g-master: --- → fixed
Flags: needinfo?(schung)
Keywords: branch-patch-needed
(Assignee)

Comment 24

4 years ago
On v2.2: https://github.com/mozilla-b2g/gaia/commit/6e494f1d2676d231abba7dcc2e2822d1170d2d02

We don't plan to uplift it to 2.1 because it's nice to have refinement while rendering the messages, not significant performance improvement. Please ping me if you still prefer 2.1 uplifting.
status-b2g-v2.2: affected → fixed
Flags: needinfo?(schung)
Keywords: branch-patch-needed
(Reporter)

Updated

3 years ago
Depends on: 1172896
(Reporter)

Updated

3 years ago
Depends on: 1181136
You need to log in before you can comment on or make changes to this bug.