Closed Bug 875338 Opened 7 years ago Closed 7 years ago

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

Categories

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

ARM
Gonk (Firefox OS)
defect

Tracking

(blocking-b2g:leo+, b2g18 fixed, b2g-v1.1hd fixed)

RESOLVED FIXED
1.1 QE4 (15jul)
blocking-b2g leo+
Tracking Status
b2g18 --- fixed
b2g-v1.1hd --- fixed

People

(Reporter: borjasalguero, Assigned: kaze)

References

Details

(Whiteboard: [u=commsapps-user c=messaging p=0])

Attachments

(3 files, 1 obsolete file)

When sending a new MMS the content is not scrolled properly automatically. See attachment
Assignee: nobody → mike
blocking-b2g: --- → leo?
Depends on: 840044
Assignee: mike → gnarf37
Attached patch patch v1 (obsolete) — Splinter Review
I can't think of a good way to unit test this.
Attachment #753343 - Flags: review?(felash)
Attachment #753343 - Flags: review?(fbsc)
Status: NEW → ASSIGNED
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.
Attachment #753343 - Flags: review?(felash)
Attachment #753343 - Flags: review?(fbsc)
Attachment #753343 - Flags: review-
(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.
(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.
(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...
(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. :)
(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?
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 ?
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
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.
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?
Flags: needinfo?(felash)
Flags: needinfo?(fbsc)
Neither of these bugs are leo+ yet... I agree that if both get their leo+ flag, we should do the other one only.
Flags: needinfo?(felash)
Flags: needinfo?(fbsc)
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 876467
blocking-b2g: leo? → leo+
Hi Borja, can you check that this issue has been fixed by bug 878042 please?
Flags: needinfo?(fbsc)
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.
Flags: needinfo?(fbsc)
Im gonna reopen because this should have a separate fix.
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Attached image Scroll issue.
Thanks for your test Borja! If I find a fix during my work on bug 876467, I’ll mention it here.
Whiteboard: [u=commsapps-user c=messaging p=0]
Hi Kaze, what're we going to do on this bug here? :)
Flags: needinfo?(kaze)
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.
Flags: in-moztrap-
Pulling myself off of this bug for now in case someone else has a better idea
Assignee: gnarf37 → nobody
Priority: -- → P1
Target Milestone: --- → 1.1 QE5
Target Milestone: 1.1 QE5 → 1.1 QE4 (15jul)
I'd like to know if that's still happening after bug 889899 has landed.
Target Milestone: 1.1 QE4 (15jul) → 1.1 QE5
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 :)
I’ll have a look.
Assignee: nobody → kaze
Flags: needinfo?(kaze)
Target Milestone: 1.1 QE5 → 1.1 QE4 (15jul)
Attachment #753343 - Attachment is obsolete: true
Duplicate of this bug: 895444
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.
Attachment #778033 - Flags: review?(gnarf37) → review?(fbsc)
borja is away now, should I have a look instead ?
Yes please! Welcome back. :-)
Comment on attachment 778033 [details]
link to pull request

will look at this monday :)
Attachment #778033 - Flags: review?(fbsc) → review?(felash)
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.
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 ?
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 on attachment 778033 [details]
link to pull request

please add unit tests and I'll r+ this patch
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.
What is the review patch for?
What is the risk of the patch?
Flags: needinfo?(felash)
Merged on master:
https://github.com/mozilla-b2g/gaia/commit/96e8da02b834bc15fc4adc402b40c08ba36b2a59
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
Comment on attachment 778033 [details]
link to pull request

r=me (I did it orally before kaze merged)
Attachment #778033 - Flags: review?(felash) → review+
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.
Flags: needinfo?(felash)
Duplicate of this bug: 902841
Blocks: 899451
Uplifted 9c39bef4a338123b9794a18658712b94c3e9a2ce to:
v1-train: 6457935e34bfff9f86b99402bf02d35541d1cab7
v1.1.0hd: 6457935e34bfff9f86b99402bf02d35541d1cab7
You need to log in before you can comment on or make changes to this bug.