Recursively check document height before scrollTo #217

snewcomer · 2019-09-10T04:08:56Z

For heavy content pages, the window height might not be filled in all the way by the time you call scrollTo. As a result, clicking the back/forward button will just end up at the top of the page. We can start calling recursively the same scrollTo until we have a height that is >= than the previous y.

For scrolling to last remembered position, we need it to call scrollTo(x,y) at the exact right moment and not a minute early or late. This solution uses recursive rAF callbacks to check and execute.

ember-app-scheduler is a brilliant little library. However, whenRouteIdle and whenRoutePainted are too late (you will see the top of the page before scrolling to last remembered position). We ideally need to avoid these artifacts in the out of the box implementation (although you might want ember-app-scheduler's afterPaint or afterIdle scrollTo though

snewcomer · 2019-09-10T16:06:52Z

addon/index.js

+
+  if (
+      (documentWidth + scrollBarWidth - window.innerWidth >= scrollHash.x
+      && documentHeight + scrollBarWidth - window.innerHeight >= scrollHash.y)


Lots of sync forced layout/reflow issues when requested :(

snewcomer · 2019-09-11T03:39:31Z

I don't think this is a good idea. rAF runs every 16ms so I'm wondering if we can do this in a rAF pool where we recursively check in a requestAnimationFrame if these conditions are true.

snewcomer · 2019-09-11T04:52:49Z

addon/index.js

+      if (targetElement || 'window' === scrollElement) {
+        if (recursiveCheck) {
+          // our own implementation
+          tryScrollRecursively(window.scrollTo, scrollPosition);


We also want to do this if the page is optimistically rendered.

snewcomer · 2019-09-11T05:08:13Z

addon/index.js

@@ -103,12 +145,20 @@ let RouterScrollMixin = Mixin.create({
    }

    const delayScrollTop = get(this, 'service.delayScrollTop');
+    const afterPaint = get(this, 'service.afterPaint');
+    const afterIdle = get(this, 'service.afterIdle');


Both of these aren't strictly necessary for this PR. Likely users won't want either (b/c it leaves artifacts for most route transition). However, I added and documented just to test/make sure this PR does what is says it does and I can test which implementation works best for the apps I work on

jleja

Very nice! Mainly comments around wording/consistency.

jleja · 2019-09-11T19:04:52Z

README.md

-the First Meaningful Paint using `delayScrollTop: true` in your config.  `delayScrollTop` defaults to `false`.
+#### Scroll Timing
+
+You may want the default out of the box behaviour.  We schedule scroll after Ember's `render`.  This occurs on the tightest schedule between route transition start and end


s/out of the box/"out of the box"

jleja · 2019-09-11T19:06:22Z

addon/index.js

+
+    if (documentWidth + scrollBarWidth - window.innerWidth >= scrollHash.x
+        && documentHeight + scrollBarWidth - window.innerHeight >= scrollHash.y
+        || ATTEMPTS >= 60) {


If 60 is a magic number, maybe set it as a const

jleja · 2019-09-11T19:07:33Z

addon/index.js

+    if (requestId) {
+      window.cancelAnimationFrame(requestId);
+    }
+
    this._super(...arguments);
  },

  /**
   * Updates the scroll position
   * @param {transition|transition[]} transition If before Ember 3.6, this will be an array of transitions, otherwise


should this @param be above the @method?

jleja · 2019-09-11T19:09:17Z

addon/index.js

    this._super(...arguments);
  },

  /**
   * Updates the scroll position
   * @param {transition|transition[]} transition If before Ember 3.6, this will be an array of transitions, otherwise
   * it will be a single transition
+   * @method updateScrollPosition
+   * @param {Object} transition
+   * @param {Boolean} recursiveCheck - if check until document height is same or lger than than  y


Description of this param could read a little better. if "true", check until document height is >= y. Maybe replace y with what it actually is.

jleja · 2019-09-11T19:14:45Z

addon/index.js

@@ -103,12 +150,20 @@ let RouterScrollMixin = Mixin.create({
    }

    const delayScrollTop = get(this, 'service.delayScrollTop');
+    const whenPainted = get(this, 'service.whenPainted');


At the expense of being wordier, I'm wondering if these new properties could be more explicit. Like, scrollWhenPainted and scrollWhenIdle or something like that? Also -- would settled be a more accurate description than idle?

great idea! changed

jleja · 2019-09-11T19:16:43Z

addon/services/router-scroll.js

@@ -47,7 +50,10 @@ const RouterScroll = Service.extend({

        // if we are looking to where to transition to next, we need to set the default to the position
        // of the targetElement on screen
-        set(scrollMap, 'default', { x, y });
+        set(scrollMap, 'default', {
+          x,


Inconsistent formatting between this and line 26.

jleja · 2019-09-11T19:16:50Z

addon/services/router-scroll.js

@@ -63,7 +69,10 @@ const RouterScroll = Service.extend({

    // only a `key` present after first load
    if (key && 'number' === typeOf(x) && 'number' === typeOf(y)) {
-      set(scrollMap, key, { x, y });
+      set(scrollMap, key, {


jleja · 2019-09-11T19:18:38Z

addon/services/router-scroll.js

@@ -84,10 +93,16 @@ const RouterScroll = Service.extend({
        set(this, 'targetElement', targetElement);
      }

-      const delayScrollTop = config.routerScroll.delayScrollTop;
+      const { whenPainted, whenIdle, delayScrollTop } = config.routerScroll;
      if (delayScrollTop === true) {


If these properties default to false and only get set to true when set by the user, it might be simpler to use something like:

setProperties(this, { delayScrollTop, whenPainted, whenIdle });

snewcomer · 2019-09-11T20:32:30Z

@jleja Really appreciate the thoroughness. All your considerations were implemented here!

jleja

👏

Recursively check document height before scrollTo

400a42f

snewcomer self-assigned this Sep 10, 2019

snewcomer added the enhancement label Sep 10, 2019

snewcomer added 6 commits September 9, 2019 21:12

default case

19daa54

further improve tests

63c3a89

fix tests with mock date util

6029053

need run

8592b66

put back rules

563f041

updatemsg

35c9aeb

snewcomer commented Sep 10, 2019

View reviewed changes

update lastTry before updating scroll position

c6d1a03

snewcomer added 7 commits September 10, 2019 21:01

Use rAF instead of setTimeout for better read/write

3a8b4d9

Lets try with rAF with afterPaint config as well

e3fcd90

add more notes

8c7af08

add another readme note

867a8d5

grammar

43448be

remove mock-date

511dde6

nit

f51dbcc

snewcomer commented Sep 11, 2019

View reviewed changes

snewcomer added 2 commits September 10, 2019 21:57

update comments

394c811

add moar comments

06af4f0

snewcomer requested a review from jleja September 11, 2019 05:00

snewcomer added 3 commits September 10, 2019 22:02

go back to tests the way they were before

4fffc4a

add doc block

c0b0c77

spellings

7aa6f0a

snewcomer commented Sep 11, 2019

View reviewed changes

snewcomer added 2 commits September 11, 2019 09:08

change config names

3772500

cancel animationFrame is need be

8119f12

jleja reviewed Sep 11, 2019

View reviewed changes

snewcomer added 2 commits September 11, 2019 13:17

update based on feedback

19442a2

use default false

5f152a3

jleja approved these changes Sep 11, 2019

View reviewed changes

snewcomer merged commit 5b73f0a into master Sep 11, 2019

snewcomer deleted the sn/recursive-scroll-check branch September 11, 2019 20:45

jordanbyron mentioned this pull request Sep 12, 2019

1.3.0, 1.3.1 releases are incompatible with Ember Fastboot #220

Closed

snewcomer mentioned this pull request Feb 21, 2020

Remove recursive scroll check #237

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Recursively check document height before scrollTo #217

Recursively check document height before scrollTo #217

snewcomer commented Sep 10, 2019 •

edited

Loading

snewcomer Sep 10, 2019

snewcomer commented Sep 11, 2019

snewcomer Sep 11, 2019

snewcomer Sep 11, 2019 •

edited

Loading

jleja left a comment

jleja Sep 11, 2019

jleja Sep 11, 2019

jleja Sep 11, 2019

jleja Sep 11, 2019

jleja Sep 11, 2019

snewcomer Sep 11, 2019

jleja Sep 11, 2019

jleja Sep 11, 2019

jleja Sep 11, 2019

snewcomer commented Sep 11, 2019

jleja left a comment

Recursively check document height before scrollTo #217

Recursively check document height before scrollTo #217

Conversation

snewcomer commented Sep 10, 2019 • edited Loading

Choose a reason for hiding this comment

snewcomer commented Sep 11, 2019

Choose a reason for hiding this comment

snewcomer Sep 11, 2019 • edited Loading

Choose a reason for hiding this comment

jleja left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snewcomer commented Sep 11, 2019

jleja left a comment

Choose a reason for hiding this comment

snewcomer commented Sep 10, 2019 •

edited

Loading

snewcomer Sep 11, 2019 •

edited

Loading