Closed Bug 902500 Opened 10 years ago Closed 9 years ago

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

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: julienw, Assigned: gsvelto)

References

Details

Attachments

(2 files, 4 obsolete files)

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.
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™.
Attachment #787415 - Flags: review?(felash)
Attachment #787416 - Attachment description: Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/11424 → [PULL REQUEST] Prevent synchronous reflows when scrolling to the bottom of the message list
The patch looks good to me—I wish there was a way to test it, but I don't think there is
(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.
I meant unit or integration tests that can be added to the app's existing tests to prevent regressions
(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.
A unit test would basically be: call scrollViewToBottom, check that it's at the bottom.

Not very useful imho.
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).
Depends on: 875338
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 :(
Attachment #787415 - Attachment is obsolete: true
Attachment #787415 - Flags: review?(felash)
Attachment #787592 - Flags: review?(felash)
(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 ;)
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.
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 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 :)
Attachment #787592 - Flags: review?(felash)
(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.
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).
Attachment #789565 - Flags: review?(felash)
Awww, I take back everything I said in the comments before, there's still something amiss with the tests :(
Now this is better and Travis is happy :)
Attachment #787592 - Attachment is obsolete: true
Attachment #789565 - Attachment is obsolete: true
Attachment #789565 - Flags: review?(felash)
Attachment #789623 - Flags: review?(felash)
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.
Attachment #789623 - Flags: review?(felash) → review+
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.
Attachment #789623 - Attachment is obsolete: true
Attachment #790670 - Flags: review+
Merged because the Travis run had failed but because of a clock app test, my stuff is clean :)

master: 7ca2728938944150cbf30ef1c1cba6e20772a720
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.