875338 - [MMS] When sending a new MMS the content is not scrolled properly automatically.

Status: RESOLVED FIXED

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

Description

[Borja Salguero [:borjasalguero]]

Attachment: Scroll not updated properly.

When sending a new MMS the content is not scrolled properly automatically. See attachment

Comment 1

[Corey Frang [:gnarf]]

Attachment: patch v1

I can't think of a good way to unit test this.

Comment 2

[Julien Wajsberg [:julienw]]

Comment on attachment 753343 [details] [diff] [review]
patch v1

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

::: apps/sms/js/thread_ui.js
@@ +777,5 @@
> +              var container = ThreadUI.container;
> +              var myRect = this.getBoundingClientRect();
> +              if (container.getBoundingClientRect().bottom > myRect.top) {
> +                container.scrollTop += myRect.height;
> +              }

can't we do this in buildMessageDOM instead ?

if you're doing it here, you'll trigger a synchronous reflow for each attachment.

buildMessageDOM is not particularly better, as we'll do it for each message...

Other ideas :
* Can't we know the size in advance ? then we could assign width/height to the mediaElement and get done with it
* use scrollIntoView
* we probably already do this for SMS, because we scroll whatever the message length is. Is the difference between these cases that we already know the SMS text whereas here the loading is asynchronous ? Another idea could be to attach the mediaElement once it's loaded ? I'm not sure it would do what we want though.

I'd really like to know how it works for SMS before going forward here. I mean, it works, but the performance will be awful as soon as we'll have 10 MMS.

Comment 3

[Corey Frang [:gnarf]]

(In reply to Julien Wajsberg [:julienw] from comment #2)
> Other ideas :
> * Can't we know the size in advance ? then we could assign width/height to
> the mediaElement and get done with it

How? This is just a blob to us, I don't think we can know its size before it loads.  Because there is a max-width, and a max-height it will auto size.

> * use scrollIntoView

This is going to be strange

> * we probably already do this for SMS, because we scroll whatever the
> message length is. Is the difference between these cases that we already
> know the SMS text whereas here the loading is asynchronous ?

yes, exactly

>  Another idea could be to attach the mediaElement once it's loaded ? I'm not sure it would
> do what we want though.

this doesn't help with scroll, when we attach it we would need to do the same scrollHeight manipulation

> I'd really like to know how it works for SMS before going forward here. I
> mean, it works, but the performance will be awful as soon as we'll have 10
> MMS.

It works because everything renders then it pushes the scroll.  With this case the image size is a variable we just can't know for sure, and no matter what when we add it we will need to compensate.

Comment 4

[Julien Wajsberg [:julienw]]

(In reply to Corey Frang [:gnarf] [:gnarf37] from comment #3)
> (In reply to Julien Wajsberg [:julienw] from comment #2)
> > Other ideas :
> > * Can't we know the size in advance ? then we could assign width/height to
> > the mediaElement and get done with it
> 
> How? This is just a blob to us, I don't think we can know its size before it
> loads.  Because there is a max-width, and a max-height it will auto size.

afaik we're displaying the image in the compose area before. Using the naturalWidth and naturalHeight properties we can know the real size of the image and therefore be more clever.

Another idea is to put the media element inside a container, whose we force the height to the same as the image's max-height, and at onload we put back the height to auto. (I think this can all be done in CSS with classes). If the image hits max-height -> it will be correctly scrolled anyway; if the image is smaller -> it should be automatocally scrolled down as the page shrinks.

Not scrolling at onload has an additional benefit: if the user scrolls while the image is loading, we should not tamper with this.

Comment 5

[Corey Frang [:gnarf]]

(In reply to Julien Wajsberg [:julienw] from comment #4)
> (In reply to Corey Frang [:gnarf] [:gnarf37] from comment #3)
> 
> afaik we're displaying the image in the compose area before. Using the
> naturalWidth and naturalHeight properties we can know the real size of the
> image and therefore be more clever.

sure, for sent messages, but this is a lot of passing data along channels that don't exist currently.  We can't ever do this for received blobs, so, I don't see the point.

> Another idea is to put the media element inside a container, whose we force
> the height to the same as the image's max-height, and at onload we put back
> the height to auto. (I think this can all be done in CSS with classes). If
> the image hits max-height -> it will be correctly scrolled anyway; if the
> image is smaller -> it should be automatocally scrolled down as the page
> shrinks.
> 
> Not scrolling at onload has an additional benefit: if the user scrolls while
> the image is loading, we should not tamper with this.

So now.... Lets say the user is scrolling up through messages and we decide to load more of the messages (as our infinite scrolling code in thread_ui does...)  These images are above the screen and when they shrink down, will suck content up above the scroll point after we adjusted the height already for adding new messages...

Comment 6

[Julien Wajsberg [:julienw]]

(In reply to Corey Frang [:gnarf] [:gnarf37] from comment #5)
> 
> So now.... Lets say the user is scrolling up through messages and we decide
> to load more of the messages (as our infinite scrolling code in thread_ui
> does...)  These images are above the screen and when they shrink down, will
> suck content up above the scroll point after we adjusted the height already
> for adding new messages...

Fair point.

Then, how about scaling the image to always the same height ? In real world it will never be smaller than the available height we have anyway. KISS. :)

Comment 7

[Corey Frang [:gnarf]]

(In reply to Julien Wajsberg [:julienw] from comment #6)
> Fair point.
> 
> Then, how about scaling the image to always the same height ? In real world
> it will never be smaller than the available height we have anyway. KISS. :)

Now what if someone sends a panoramic image, with a ratio of 4:1 - the max-width of the area is only 12rem (120px) and max-height is also 12rem (120px) --- You want us to take up 120px worth of space to display 30px tall info?

Also, what if someone sends a really tiny image, say only 50x50px, do you want to also make it go to 120px tall and then get pixelated from being zoomed up to size?

Comment 8

[Julien Wajsberg [:julienw]]

The 50x50px case won't likely happen. I don't mind that if this happens, this will be pixelated.

I understand the other issue though.

Do you have ideas ?

Comment 9

[Corey Frang [:gnarf]]

I wish I did, but the only idea I had was to update scrollTop onload of the image...  I know this could result in a few reflow based delays, but I can't think of a way around the problem of not knowing how tall the image is.

The only other thing I can think of requires reworking too much of the way the code works right now, which would be to pre-load the image, so we can determine it's true height before adding it.

I could try my hardest to get the underlying problem (ThreadUI treats rendering a message as a sync operation instead of an async one) when refactoring Messages later

Comment 10

[Julien Wajsberg [:julienw]]

Thought about this with a duck (:rik).

Long term:
* we'll need to generate and store thumbnail images, and use them here. We can't display all big images in a thread, that would use too much memory and impair performance.

* We should use "background-size:cover" style thumbnails, for the panorama case, but still have landscape and portrait thumbnail form factors

Short term:
* let's use a fixed height container for our images.

I had other crazy-but-not-so-easy ideas, but none of them fix all the issues (in particular the progressive rendering issue).

I'm quite sure we'll have to do the long term issue for leo anyway, because what we have now doesn't scale. I'll file a bug for this.

Comment 11

[Julien Wajsberg [:julienw]]

Filed Bug 876467

Comment 12

[Corey Frang [:gnarf]]

If we have to do the long term for leo+ anyway, I vote we just accept the bug in the short term?

Mark as a dupe/depends on 876467?

Comment 13

[Julien Wajsberg [:julienw]]

Neither of these bugs are leo+ yet... I agree that if both get their leo+ flag, we should do the other one only.

Comment 15

[Fabien Cazenave [:kaze]]

Hi Borja, can you check that this issue has been fixed by bug 878042 please?

Comment 16

[Borja Salguero [:borjasalguero]]

Kaze! The scroll is not working as expected... When you have a large thread (for example 10 previously sent MMS) and you send a new one with an image, the scroll is not working... :(. I will attach an image for showing you the result.

Comment 17

[Borja Salguero [:borjasalguero]]

Im gonna reopen because this should have a separate fix.

Comment 18

[Borja Salguero [:borjasalguero]]

Attachment: Scroll issue.

Comment 19

[Fabien Cazenave [:kaze]]

Thanks for your test Borja! If I find a fix during my work on bug 876467, I’ll mention it here.

Comment 20

[Wayne Chang [:wchang]]

Hi Kaze, what're we going to do on this bug here? :)

Comment 21

[Julien Wajsberg [:julienw]]

we're still not sure of the right way to resolve this bug yet.

I'd like to move forwards with bug 889899 first, maybe we'll see better after this.

Comment 22

[Corey Frang [:gnarf]]

Pulling myself off of this bug for now in case someone else has a better idea

Comment 23

[Julien Wajsberg [:julienw]]

I'd like to know if that's still happening after bug 889899 has landed.

Comment 24

[Julien Wajsberg [:julienw]]

kaze, can you try to take this next week if that's still not fixed ?

The gotcha are: we don't want to scroll if the user has already scrolled while it's loading; we don't want sync reflows. And... that's it :)

Comment 25

[Fabien Cazenave [:kaze]]

I’ll have a look.

Comment 26

[Fabien Cazenave [:kaze]]

Attachment: link to pull request

https://github.com/mozilla-b2g/gaia/pull/11055

Comment 28

[Corey Frang [:gnarf]]

Comment on attachment 778033 [details]
link to pull request

I'm kind of +1 and -1 on this one. I feel like this solution still has problems, (loading additional messages in after you've scrolled...) and the bug just can't be solved entirely at the moment., but we need to pick the best solution we can get at the moment, and this might just be it.  I'm going to ask borja to take a second look at this approach before we approve it.

Comment 29

[Julien Wajsberg [:julienw]]

borja is away now, should I have a look instead ?

Comment 30

[Fabien Cazenave [:kaze]]

Yes please! Welcome back. :-)

Comment 31

[Julien Wajsberg [:julienw]]

Comment on attachment 778033 [details]
link to pull request

will look at this monday :)

Comment 32

[Julien Wajsberg [:julienw]]

Loading a thread containing MMS is still broken, but maybe we can do that in another bug.

We need to consider what exactly we need to do:
* where we are in the page
* which (new) image has been displayed (higher or lower)
* should we scroll so that where we are don't seem to move ?
* how much should we scroll ?

I think we can try to do this now that we use div+img: we know the new height as we set the container height, we know the old height as we have it in css (I think), so we know the difference (without a sync reflow).

Then we need to know if we should scroll (increasing the container's scrollTop by the difference). We should take the image container offsetTop _before_ setting the container's height, so that we don't trigger a synchronous workflow. With this offsetTop and the current scrollTop we can probably know if it's higher or lower our current scroll position, and therefore know if we should scroll.

Kaze, do you feel like doing this now, or should it be in another bug ?

To reproduce, simply push the medium reference workload and display one of the MMS threads.

Comment 33

[bhavana bajaj [:bajaj]]

Julien , is the original issue as reported fixed by the patch under review ?

And the suggestions you are making to improve further can be done in 1.2 ?

Comment 34

[Julien Wajsberg [:julienw]]

Bhavana> Yep I'll check.

I sincerely think what I am saying is related, and that the current patch is more a workaround than fixing the root cause, especially that I don't think it would be that long to do it correctly (from start to end: around 1 week including reviews).

Comment 35

[Julien Wajsberg [:julienw]]

will file a bug for comment 32

Comment 36

[Julien Wajsberg [:julienw]]

Comment on attachment 778033 [details]
link to pull request

please add unit tests and I'll r+ this patch

Comment 37

[Julien Wajsberg [:julienw]]

Note that I can't reproduce comment 32 with a "normal" set, will try later with a bigger workload to see if that still happens.

Comment 38

[Preeti Raghunath(:Preeti)]

What is the review patch for?
What is the risk of the patch?

Comment 39

[Fabien Cazenave [:kaze]]

Merged on master:
https://github.com/mozilla-b2g/gaia/commit/96e8da02b834bc15fc4adc402b40c08ba36b2a59

Comment 40

[Fabien Cazenave [:kaze]]

Ooops. Reverted:
https://github.com/mozilla-b2g/gaia/commit/738c7ffdfbcf0c13d2768237e590f17faef2b405

… and merged again on master:
https://github.com/mozilla-b2g/gaia/commit/9c39bef4a338123b9794a18658712b94c3e9a2ce

Comment 41

[Julien Wajsberg [:julienw]]

Comment on attachment 778033 [details]
link to pull request

r=me (I did it orally before kaze merged)

Comment 42

[Julien Wajsberg [:julienw]]

Preeti> the reviewed and landed patch is for the original bug only. So comment 32 was not followed at all here. I've not filed a new bug because I couldn't reproduce it recently, but it may be reproduced more easily with a big reference workload. So will try this eventually later.

The risk of the patch is very low. There is zero chance to break something important in Sms.

Comment 44

[John Ford [:jhford] CET/CEST Berlin Time]

Uplifted 9c39bef4a338123b9794a18658712b94c3e9a2ce to:
v1-train: 6457935e34bfff9f86b99402bf02d35541d1cab7

Comment 45

[Tim Guan-tin Chien [:timdream] (please needinfo)]

v1.1.0hd: 6457935e34bfff9f86b99402bf02d35541d1cab7