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).
This commit is contained in:
David Taylor 2021-11-26 20:22:50 +00:00 committed by GitHub
parent 5d59b7e733
commit 1b184cefd0
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
5 changed files with 30 additions and 33 deletions

View File

@ -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

View File

@ -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");

View File

@ -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() {

View File

@ -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");

View File

@ -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);
},
});