902500 - [sms][mms] makes scrollViewToBottom synchronous-reflow-free

Status: RESOLVED FIXED

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

Description

[Julien Wajsberg [:julienw]]

Gabriele's profile showed that the existing scrollViewToBottom triggers a synchronous reflow because of the scrollHeight use.

We tryed using a scrollIntoView call instead which seemed to do the trick and triggers an interruptible reflow instead, which is way better.

Put Gabriele as assignee as he has a patch already, but we should wait for leo+ bug 875338 landing before as it's also changing that part of the code.

Comment 1

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH] Prevent synchronous reflows when scrolling to the bottom of the message list

This patch uses scrollIntoView() to scroll to the last message in the thread instead of setting the thread container's scrollTop variable directly. The net effect is that the reflow caused by this operation is now interruptible and in my limited testing better behaved. I could measure a ~10ms performance difference but with a large error margin so I don't think this is conclusive; what's better is that an interruptible reflow doesn't stop JS execution which is a Good Thing™.

Comment 2

[Gabriele Svelto [:gsvelto]]

Attachment: [PULL REQUEST] Prevent synchronous reflows when scrolling to the bottom of the message list

Pointer to Github pull-request

Comment 3

[Rick Waldron [:rwaldron]]

The patch looks good to me—I wish there was a way to test it, but I don't think there is

Comment 4

[Gabriele Svelto [:gsvelto]]

(In reply to Rick Waldron from comment #3)
> The patch looks good to me—I wish there was a way to test it, but I don't
> think there is

I've tested it actually by printing out the contents of this.container.scrollTop in scrollViewToBottom() before and after the patch :) It turns out there's a 5px difference between the two though it's hard to spot by just looking at the screen. We're trying to get rid of it by playing with the padding of the last element so that the look remains 100% consistent.

Comment 5

[Rick Waldron [:rwaldron]]

I meant unit or integration tests that can be added to the app's existing tests to prevent regressions

Comment 6

[Gabriele Svelto [:gsvelto]]

(In reply to Rick Waldron from comment #5)
> I meant unit or integration tests that can be added to the app's existing
> tests to prevent regressions

Ah, right. Do you think it would make sense to check the relative positions of the elements in a test? Admittedly the DPI-independent changes would make this tricky to get right.

Comment 7

[Julien Wajsberg [:julienw]]

A unit test would basically be: call scrollViewToBottom, check that it's at the bottom.

Not very useful imho.

Comment 8

[Gabriele Svelto [:gsvelto]]

Bug 875338 adds some logic that depends on the thread container's scrollTop and scrollHeight properties so it might be worth to add a test in the end. I'll revise my patch and make sure that the behavior before and after my change is the same (and I'll add a unit test).

Comment 9

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH v2] Prevent synchronous reflows when scrolling to the bottom of the message list

Refreshed patch, this changes the CSS bottom margin attribute of the last message into a bottom padding attribute making the scrolling exactly the same as before. I've tried writing a unit test for this but unfortunately this depends entirely on the CSS properties of the various elements and those are not applied in the tests making writing one impractical. You'll have to trust me on this one :(

Comment 10

[Rick Waldron [:rwaldron]]

(In reply to Julien Wajsberg [:julienw] from comment #7)
> A unit test would basically be: call scrollViewToBottom, check that it's at
> the bottom.

That would work for me.

> 
> Not very useful imho.

Useful for preventing someone from coming into the code and changing it to use some home-grown operation ;)

Comment 11

[Gabriele Svelto [:gsvelto]]

As I mentioned in comment 9 the lack of the CSS styles in the tests makes adding a unit-test for this (and adjusting the existing one) tricky. I could try and add the CSS code and styles to the test but then we would need to keep them in sync with the original and that sounds very fragile to me. Anyway I'll give this some thought today and see if I can come up with something to cover this functionality.

Comment 12

[Julien Wajsberg [:julienw]]

Maybe it's possible to just fill the scrollable div with some garbage to check that the function works ? (like it is with the current test already)

Comment 13

[Julien Wajsberg [:julienw]]

Comment on attachment 787592 [details] [diff] [review]
[PATCH v2] Prevent synchronous reflows when scrolling to the bottom of the message list

so, waiting for a testcase :)

Comment 14

[Gabriele Svelto [:gsvelto]]

(In reply to Julien Wajsberg [:julienw] from comment #12)
> Maybe it's possible to just fill the scrollable div with some garbage to
> check that the function works ? (like it is with the current test already)

I'll try but it won't be very robust. Still it's going to be better than nothing.

Comment 15

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH v3] Prevent synchronous reflows when scrolling to the bottom of the message list

This is an updated patch with an additional unit-test used for detecting correct scroll-to-bottom functionality as well as a previously existing unit-test refreshed to make use of the new functionality so that it reflects what the application is doing.

To work around the missing CSS issue I've manually added 16 pixels of padding to the last element in the unit-test harness. This is not particularly robust but it's better than not having a unit-test at all.

I have updated the PR with this patch (and a rebase on top of today's gaia master tree).

Comment 16

[Gabriele Svelto [:gsvelto]]

Awww, I take back everything I said in the comments before, there's still something amiss with the tests :(

Comment 17

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH v4] Prevent synchronous reflows when scrolling to the bottom of the message list

Now this is better and Travis is happy :)

Comment 18

[Julien Wajsberg [:julienw]]

Comment on attachment 789623 [details] [diff] [review]
[PATCH v4] Prevent synchronous reflows when scrolling to the bottom of the message list

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

r=me with the nits

::: apps/sms/js/thread_ui.js
@@ +534,4 @@
>    },
>  
>    scrollViewToBottom: function thui_scrollViewToBottom() {
> +    if (!this.isScrolledManually && this.container.firstElementChild) {

nit: reading this again, I now think this should be lastElementChild instead of firstElementChild here. Makes the code clearer to read, and it's basically as fast. Sorry about that as it was my first advice..

::: apps/sms/test/unit/thread_ui_test.js
@@ +195,5 @@
> +                  container.scrollHeight);
> +        done();
> +      });
> +      ThreadUI.isScrolledManually = false;
> +      ThreadUI.scrollViewToBottom();

nit: I'd put those 2 tests inside the same suite; the setup() would be:

setup(function(done) {
  container.addEventListener('scroll', function onscroll() {
    container.removeEventListener('scroll', onscroll);
    done();
  });
  ThreadUI.isScrolledManually = false;
  ThreadUI.scrollViewToBottom();
});

And then your tests could just be the asserts.

Comment 19

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH v5] Prevent synchronous reflows when scrolling to the bottom of the message list

Thanks for the review! Refreshed patch with nits from comment 18 addressed carrying over the r+ flag with r=julienw. I've updated the PR and will be waiting for Travis before merging.

Comment 20

[Gabriele Svelto [:gsvelto]]

Merged because the Travis run had failed but because of a clock app test, my stuff is clean :)

master: 7ca2728938944150cbf30ef1c1cba6e20772a720