From 1aedfb126737d9cd7b8088fdcb3c572042c2ba7b Mon Sep 17 00:00:00 2001 From: Ruslan Hrabovyi Date: Wed, 1 Mar 2023 15:00:46 +0100 Subject: [PATCH 1/3] add failing test for single scrollable with multiple handlers --- .../data-view/utils/scroll-handler-test.js | 60 +++++++++++++++++++ 1 file changed, 60 insertions(+) diff --git a/tests/unit/-private/data-view/utils/scroll-handler-test.js b/tests/unit/-private/data-view/utils/scroll-handler-test.js index 12ba157b..874f9bef 100644 --- a/tests/unit/-private/data-view/utils/scroll-handler-test.js +++ b/tests/unit/-private/data-view/utils/scroll-handler-test.js @@ -200,3 +200,63 @@ test('Multiple elements with handlers works as expected', (assert) => { }); }); }); + +test('multiple handlers with same scrollable', (assert) => { + let scrollHandlers = new ScrollHandler(); + let done = assert.async(3); + let scrollable = createScrollable(); + let handler1 = () => { + assert.ok('handler1 was triggered'); + done(); + }; + let handler2 = () => { + assert.ok('handler2 was triggered'); + done(); + }; + + // test adding the handlers + assert.strictEqual(scrollHandlers.length, 0, `We initially have no elements to watch.`); + scrollHandlers.addScrollHandler(scrollable, handler1); + scrollHandlers.addScrollHandler(scrollable, handler2); + + assert.strictEqual(scrollHandlers.length, 1, `We have one element to watch.`); + + let scrollable1Index = scrollHandlers.elements.indexOf(scrollable); + + assert.strictEqual(scrollable1Index, 0, `The scrollable was added to the watched elements list.`); + + let cache1 = scrollHandlers.handlers[0]; + + assert.true(cache1.handlers.length === 2, `We added the handler`); + + // test triggering that handler + assert.strictEqual(scrollable.scrollTop, 0, `The scrollable is initially unscrolled`); + + assert.false(scrollHandlers.isPolling, 'polling is inactive'); + + afterNextScrollUpdate(() => { + scrollable.scrollTop = 10; + assert.equal(scrollable.scrollTop, 10, `We updated the scrollable's scroll position`); + + afterNextScrollUpdate(() => { + // test removing that handler + scrollHandlers.removeScrollHandler(scrollable, handler1); + let newScrollableIndex = scrollHandlers.elements.indexOf(scrollable); + assert.strictEqual(newScrollableIndex, 0, `The element remains in the watched elements list.`); + assert.strictEqual(scrollHandlers.length, 1, `We have an element to watch.`); + assert.strictEqual(cache1.handlers.length, 1, `The first handler was removed from the listener cache.`); + assert.strictEqual(scrollHandlers.handlers.indexOf(cache1), 0, `The cache still exists.`); + + scrollHandlers.removeScrollHandler(scrollable, handler2); + newScrollableIndex = scrollHandlers.elements.indexOf(scrollable); + assert.strictEqual(newScrollableIndex, -1, `Removing the last handler removed the element from the watched elements list.`); + assert.strictEqual(scrollHandlers.length, 0, `We have no more elements to watch.`); + assert.strictEqual(cache1.handlers.length, 0, `The last handler was removed from the listener cache.`); + + assert.false(scrollHandlers.isPolling, `We are no longer polling the elements.`); + + destroyScrollable(scrollable); + done(); + }); + }); +}); From 589c2f604e8f0fc88e3eba5c7fda4cef652dfaea Mon Sep 17 00:00:00 2001 From: Ruslan Hrabovyi Date: Wed, 1 Mar 2023 15:11:11 +0100 Subject: [PATCH 2/3] avoid concurrent passiveHandler and poll --- addon/-private/data-view/utils/scroll-handler.js | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/addon/-private/data-view/utils/scroll-handler.js b/addon/-private/data-view/utils/scroll-handler.js index e793b59d..eaae71c7 100644 --- a/addon/-private/data-view/utils/scroll-handler.js +++ b/addon/-private/data-view/utils/scroll-handler.js @@ -40,23 +40,21 @@ export class ScrollHandler { cache.passiveHandler = function() { ScrollHandler.triggerElementHandlers(element, cache); }; + + element.addEventListener('scroll', cache.passiveHandler, { capture: true, passive: true }); + // TODO add explicit test } else { cache.passiveHandler = UNDEFINED_VALUE; + + if (!this.isPolling) { + this.poll(); + } } } else { cache = this.handlers[index]; handlers = cache.handlers; handlers.push(handler); } - - // TODO add explicit test - if (this.isUsingPassive && handlers.length === 1) { - element.addEventListener('scroll', cache.passiveHandler, { capture: true, passive: true }); - - // TODO add explicit test - } else if (!this.isPolling) { - this.poll(); - } } removeScrollHandler(element, handler) { From 5e8b9d1ccf3625f9390c0811a6f1ddb7288ee28c Mon Sep 17 00:00:00 2001 From: Ruslan Hrabovyi Date: Wed, 1 Mar 2023 15:37:26 +0100 Subject: [PATCH 3/3] explicitly test passive VS polling modes --- .../data-view/utils/scroll-handler.js | 5 +- .../data-view/utils/scroll-handler-test.js | 76 +++++++++++++++---- 2 files changed, 64 insertions(+), 17 deletions(-) diff --git a/addon/-private/data-view/utils/scroll-handler.js b/addon/-private/data-view/utils/scroll-handler.js index eaae71c7..453ccb21 100644 --- a/addon/-private/data-view/utils/scroll-handler.js +++ b/addon/-private/data-view/utils/scroll-handler.js @@ -35,14 +35,13 @@ export class ScrollHandler { left: element.scrollLeft, handlers }; - // TODO add explicit test - if (SUPPORTS_PASSIVE) { + + if (this.isUsingPassive) { cache.passiveHandler = function() { ScrollHandler.triggerElementHandlers(element, cache); }; element.addEventListener('scroll', cache.passiveHandler, { capture: true, passive: true }); - // TODO add explicit test } else { cache.passiveHandler = UNDEFINED_VALUE; diff --git a/tests/unit/-private/data-view/utils/scroll-handler-test.js b/tests/unit/-private/data-view/utils/scroll-handler-test.js index 874f9bef..8e87ab23 100644 --- a/tests/unit/-private/data-view/utils/scroll-handler-test.js +++ b/tests/unit/-private/data-view/utils/scroll-handler-test.js @@ -32,6 +32,8 @@ module('Unit | Radar Utils | Scroll Handler'); test('We can add, trigger, and remove a scroll handler', (assert) => { let scrollHandlers = new ScrollHandler(); + scrollHandlers.isUsingPassive = true; + let done = assert.async(2); let scrollable = createScrollable(); let handler = () => { @@ -44,31 +46,81 @@ test('We can add, trigger, and remove a scroll handler', (assert) => { // test adding a single handler scrollHandlers.addScrollHandler(scrollable, handler); - assert.equal(scrollHandlers.length, 1, `We have one element to watch.`); + assert.strictEqual(scrollHandlers.length, 1, `We have one element to watch.`); + assert.false(scrollHandlers.isPolling, 'polling is inactive, using a passive handler'); let scrollableIndex = scrollHandlers.elements.indexOf(scrollable); - assert.ok(scrollableIndex !== -1, `The scrollable was added to the watched elements list.`); + assert.true(scrollableIndex !== -1, `The scrollable was added to the watched elements list.`); let cache = scrollHandlers.handlers[scrollableIndex]; - assert.ok(cache.handlers.length === 1); + assert.strictEqual(cache.handlers.length, 1); // test triggering that handler - assert.equal(scrollable.scrollTop, 0, `The scrollable is initially unscrolled`); + assert.strictEqual(scrollable.scrollTop, 0, `The scrollable is initially unscrolled`); afterNextScrollUpdate(() => { scrollable.scrollTop = 10; - assert.equal(scrollable.scrollTop, 10, `We updated the scrollable's scroll position`); + assert.strictEqual(scrollable.scrollTop, 10, `We updated the scrollable's scroll position`); afterNextScrollUpdate(() => { // test removing that handler scrollHandlers.removeScrollHandler(scrollable, handler); let newScrollableIndex = scrollHandlers.elements.indexOf(scrollable); - assert.ok(cache.handlers.length === 0, `The handler was removed from the listener cache.`); - assert.ok(newScrollableIndex === -1, `Removing the last handler removed the element from the watched elements list.`); - assert.ok(scrollHandlers.handlers.indexOf(cache) === -1, `Removing the last handler removed the cache.`); + assert.strictEqual(cache.handlers.length, 0, `The handler was removed from the listener cache.`); + assert.strictEqual(newScrollableIndex, -1, `Removing the last handler removed the element from the watched elements list.`); + assert.strictEqual(scrollHandlers.handlers.indexOf(cache), -1, `Removing the last handler removed the cache.`); - assert.equal(scrollHandlers.length, 0, `We have no more elements to watch.`); - assert.equal(scrollHandlers.isPolling, false, `We are no longer polling the elements.`); + assert.strictEqual(scrollHandlers.length, 0, `We have no more elements to watch.`); + assert.false(scrollHandlers.isPolling, `polling is still inactive`); + + destroyScrollable(scrollable); + done(); + }); + }); +}); + +test('Polling', (assert) => { + let scrollHandlers = new ScrollHandler(); + scrollHandlers.isUsingPassive = false; + + let done = assert.async(2); + let scrollable = createScrollable(); + let handler = () => { + assert.ok('handler was triggered'); + done(); + }; + + assert.strictEqual(scrollHandlers.length, 0, `We initially have no elements to watch.`); + + // test adding a single handler + scrollHandlers.addScrollHandler(scrollable, handler); + + assert.strictEqual(scrollHandlers.length, 1, `We have one element to watch.`); + assert.true(scrollHandlers.isPolling, 'polling is active'); + + let scrollableIndex = scrollHandlers.elements.indexOf(scrollable); + assert.true(scrollableIndex !== -1, `The scrollable was added to the watched elements list.`); + let cache = scrollHandlers.handlers[scrollableIndex]; + assert.strictEqual(cache.handlers.length, 1); + + // test triggering that handler + assert.strictEqual(scrollable.scrollTop, 0, `The scrollable is initially unscrolled`); + + afterNextScrollUpdate(() => { + scrollable.scrollTop = 10; + assert.strictEqual(scrollable.scrollTop, 10, `We updated the scrollable's scroll position`); + + afterNextScrollUpdate(() => { + // test removing that handler + scrollHandlers.removeScrollHandler(scrollable, handler); + let newScrollableIndex = scrollHandlers.elements.indexOf(scrollable); + + assert.strictEqual(cache.handlers.length, 0, `The handler was removed from the listener cache.`); + assert.strictEqual(newScrollableIndex, -1, `Removing the last handler removed the element from the watched elements list.`); + assert.strictEqual(scrollHandlers.handlers.indexOf(cache), -1, `Removing the last handler removed the cache.`); + + assert.strictEqual(scrollHandlers.length, 0, `We have no more elements to watch.`); + assert.false(scrollHandlers.isPolling, `We are no longer polling the elements.`); destroyScrollable(scrollable); done(); @@ -125,7 +177,6 @@ test('Adding/removing multiple handlers to an element works as expected', (asser assert.ok(scrollHandlers.handlers.indexOf(cache) === -1, `Removing the last handler removed the cache.`); assert.equal(scrollHandlers.length, 0, `We have no more elements to watch.`); - assert.equal(scrollHandlers.isPolling, false, `We are no longer polling the elements.`); destroyScrollable(scrollable); done(); @@ -192,7 +243,6 @@ test('Multiple elements with handlers works as expected', (assert) => { assert.ok(scrollHandlers.handlers.indexOf(cache2) === -1, `Removing the last handler removed the cache.`); assert.equal(scrollHandlers.length, 0, `We have no more elements to watch.`); - assert.equal(scrollHandlers.isPolling, false, `We are no longer polling the elements.`); destroyScrollable(scrollable1); destroyScrollable(scrollable2); @@ -253,8 +303,6 @@ test('multiple handlers with same scrollable', (assert) => { assert.strictEqual(scrollHandlers.length, 0, `We have no more elements to watch.`); assert.strictEqual(cache1.handlers.length, 0, `The last handler was removed from the listener cache.`); - assert.false(scrollHandlers.isPolling, `We are no longer polling the elements.`); - destroyScrollable(scrollable); done(); });