From 1b184cefd0270171b516ff0c0a5ecc11fb4a2790 Mon Sep 17 00:00:00 2001 From: David Taylor Date: Fri, 26 Nov 2021 20:22:50 +0000 Subject: [PATCH] PERF: Update scrolling mixin implementation (#15109) This mixin calls the "scrolled" method of some object with no parameters, so there is no way that consumers would ever call `event.preventDefault()`. Therefore we can make the listeners passive, and improve scrolling performance on mobile. This commit also updates the mixin to remove JQuery usage. The API is slightly modified to remove the need for an event 'name' for binding/unbinding. The calls to `.bindScrolling` and `.unbindScrolling` in user-stream.js are removed because they are already called by the LoadMore mixin which is applied to the component. The `bindScrolling` method claimed to offer debouncing-by-default. However, a bug in the `opts` parsing meant that debouncing was skipped if a 'name' was passed in. Therefore the only consumer actually being debounced was the LoadMore mixin. This commit fixes the opts parsing, so all consumers get the same behavior. However, when scrolling, debounce is rarely what we want. The documentation of `bindScrolling` says "called every 100ms". In fact, debounce means that the functions were only called 'after the user **stops scrolling** for 100ms'. If you're scrolling very slowly (e.g. when using momentum-based scrolling on mobile), then this can be quite frustrating. This is why "Load more" is only triggered on topics/topic-lists when you completely stop scrolling. Therefore, this commit also replaces the default 'debounce' with a 'throttle'. The 'throttle' is configured with `immediate = false`, so that it fires on the trailing edge, and therefore the final call will always be **after** we finish scrolling. (the default `immediate: true` would fire on the leading edge, and so the last call could be up to 100ms **before** we finish scrolling). --- .../app/components/discourse-topic.js | 4 +- .../discourse/app/components/footer-nav.js | 4 +- .../app/components/scroll-tracker.js | 4 +- .../discourse/app/components/user-stream.js | 3 -- .../discourse/app/mixins/scrolling.js | 48 +++++++++---------- 5 files changed, 30 insertions(+), 33 deletions(-) diff --git a/app/assets/javascripts/discourse/app/components/discourse-topic.js b/app/assets/javascripts/discourse/app/components/discourse-topic.js index c1b3d53d8f4..d5daaca8397 100644 --- a/app/assets/javascripts/discourse/app/components/discourse-topic.js +++ b/app/assets/javascripts/discourse/app/components/discourse-topic.js @@ -95,7 +95,7 @@ export default Component.extend( didInsertElement() { this._super(...arguments); - this.bindScrolling({ name: "topic-view" }); + this.bindScrolling(); window.addEventListener("resize", this.scrolled); $(this.element).on( "click.discourse-redirect", @@ -110,7 +110,7 @@ export default Component.extend( willDestroyElement() { this._super(...arguments); - this.unbindScrolling("topic-view"); + this.unbindScrolling(); window.removeEventListener("resize", this.scrolled); // Unbind link tracking diff --git a/app/assets/javascripts/discourse/app/components/footer-nav.js b/app/assets/javascripts/discourse/app/components/footer-nav.js index 449ab625e4d..5df81b7bc73 100644 --- a/app/assets/javascripts/discourse/app/components/footer-nav.js +++ b/app/assets/javascripts/discourse/app/components/footer-nav.js @@ -40,7 +40,7 @@ const FooterNavComponent = MountWidget.extend( if (this.capabilities.isIpadOS) { document.body.classList.add("footer-nav-ipad"); } else { - this.bindScrolling({ name: "footer-nav" }); + this.bindScrolling(); window.addEventListener("resize", this.scrolled, false); this.appEvents.on("composer:opened", this, "_composerOpened"); this.appEvents.on("composer:closed", this, "_composerClosed"); @@ -60,7 +60,7 @@ const FooterNavComponent = MountWidget.extend( if (this.capabilities.isIpadOS) { document.body.classList.remove("footer-nav-ipad"); } else { - this.unbindScrolling("footer-nav"); + this.unbindScrolling(); window.removeEventListener("resize", this.scrolled); this.appEvents.off("composer:opened", this, "_composerOpened"); this.appEvents.off("composer:closed", this, "_composerClosed"); diff --git a/app/assets/javascripts/discourse/app/components/scroll-tracker.js b/app/assets/javascripts/discourse/app/components/scroll-tracker.js index 228a50d1a25..9e961b28924 100644 --- a/app/assets/javascripts/discourse/app/components/scroll-tracker.js +++ b/app/assets/javascripts/discourse/app/components/scroll-tracker.js @@ -12,7 +12,7 @@ export default Component.extend(Scrolling, { didInsertElement() { this._super(...arguments); - this.bindScrolling({ name: this.name }); + this.bindScrolling(); }, didRender() { @@ -27,7 +27,7 @@ export default Component.extend(Scrolling, { willDestroyElement() { this._super(...arguments); - this.unbindScrolling(this.name); + this.unbindScrolling(); }, scrolled() { diff --git a/app/assets/javascripts/discourse/app/components/user-stream.js b/app/assets/javascripts/discourse/app/components/user-stream.js index 71cc51166ec..6e45c375c76 100644 --- a/app/assets/javascripts/discourse/app/components/user-stream.js +++ b/app/assets/javascripts/discourse/app/components/user-stream.js @@ -37,8 +37,6 @@ export default Component.extend(LoadMore, { }, _inserted: on("didInsertElement", function () { - this.bindScrolling({ name: "user-stream-view" }); - $(window).on("resize.discourse-on-scroll", () => this.scrolled()); $(this.element).on( @@ -54,7 +52,6 @@ export default Component.extend(LoadMore, { // This view is being removed. Shut down operations _destroyed: on("willDestroyElement", function () { - this.unbindScrolling("user-stream-view"); $(window).unbind("resize.discourse-on-scroll"); $(this.element).off("click.details-disabled", "details.disabled"); diff --git a/app/assets/javascripts/discourse/app/mixins/scrolling.js b/app/assets/javascripts/discourse/app/mixins/scrolling.js index 2f0a588e21b..ced98a406f9 100644 --- a/app/assets/javascripts/discourse/app/mixins/scrolling.js +++ b/app/assets/javascripts/discourse/app/mixins/scrolling.js @@ -1,6 +1,5 @@ import Mixin from "@ember/object/mixin"; -import discourseDebounce from "discourse-common/lib/debounce"; -import { scheduleOnce } from "@ember/runloop"; +import { scheduleOnce, throttle } from "@ember/runloop"; import { inject as service } from "@ember/service"; /** @@ -9,20 +8,18 @@ import { inject as service } from "@ember/service"; easier. **/ const ScrollingDOMMethods = { - bindOnScroll(onScrollMethod, name) { - name = name || "default"; - $(document).bind(`touchmove.discourse-${name}`, onScrollMethod); - $(window).bind(`scroll.discourse-${name}`, onScrollMethod); + bindOnScroll(onScrollMethod) { + document.addEventListener("touchmove", onScrollMethod, { passive: true }); + window.addEventListener("scroll", onScrollMethod, { passive: true }); }, - unbindOnScroll(name) { - name = name || "default"; - $(window).unbind(`scroll.discourse-${name}`); - $(document).unbind(`touchmove.discourse-${name}`); + unbindOnScroll(onScrollMethod) { + document.removeEventListener("touchmove", onScrollMethod); + window.removeEventListener("scroll", onScrollMethod); }, screenNotFull() { - return $(window).height() > $("#main").height(); + return window.height > document.querySelector("#main").offsetHeight; }, }; @@ -30,14 +27,15 @@ const Scrolling = Mixin.create({ router: service(), // Begin watching for scroll events. By default they will be called at max every 100ms. - // call with {debounce: N} for a diff time - bindScrolling(opts) { - opts = opts || { debounce: 100 }; - + // call with {throttle: N} to change the throttle spacing + bindScrolling(opts = {}) { + if (!opts.throttle) { + opts.throttle = 100; + } // So we can not call the scrolled event while transitioning. There is no public API for this :'( const microLib = this.router._router._routerMicrolib; - let onScrollMethod = () => { + let scheduleScrolled = () => { if (microLib.activeTransition) { return; } @@ -45,20 +43,22 @@ const Scrolling = Mixin.create({ return scheduleOnce("afterRender", this, "scrolled"); }; - if (opts.debounce) { - let debouncedScrollMethod = () => { - discourseDebounce(this, onScrollMethod, opts.debounce); - }; - ScrollingDOMMethods.bindOnScroll(debouncedScrollMethod, opts.name); + let onScrollMethod; + if (opts.throttle) { + onScrollMethod = () => + throttle(this, scheduleScrolled, opts.throttle, false); } else { - ScrollingDOMMethods.bindOnScroll(onScrollMethod, opts.name); + onScrollMethod = scheduleScrolled; } + + this._scrollingMixinOnScrollMethod = onScrollMethod; + ScrollingDOMMethods.bindOnScroll(onScrollMethod); }, screenNotFull: () => ScrollingDOMMethods.screenNotFull(), - unbindScrolling(name) { - ScrollingDOMMethods.unbindOnScroll(name); + unbindScrolling() { + ScrollingDOMMethods.unbindOnScroll(this._scrollingMixinOnScrollMethod); }, });