DEV: Move all scroll position reset/remember logic to a shared service (#22552)

Previously we were implementing scroll reset/memorization on a per-page basis. Many of these approaches relied on the `didInsertElement` hook, which is no longer appropriate since Discourse changed to use the 'loading slider' strategy for page transitions.

This commit rips out all of our custom scroll resetting/memorizing, and implements those things in a generic service. There are two features:

1. After every route transition, scroll to the top of the page
2. When using browser back/forward buttons, restore the last known scroll position for those routes

To opt-out of the behaviour, individual routes can add a scrollOnTransition boolean to their RouteInfo metadata using Ember's `buildRouteInfoMetadata` hook.
This commit is contained in:
David Taylor 2023-07-13 13:40:08 +01:00 committed by GitHub
parent 80578e75f0
commit dfe94ba118
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
29 changed files with 179 additions and 190 deletions

View File

@ -10,8 +10,6 @@
@canBulkSelect={{this.canBulkSelect}}
@selected={{this.selected}}
@tagsForUser={{this.tagsForUser}}
@onScroll={{this.onScroll}}
@scrollOnLoad={{this.scrollOnLoad}}
@toggleBulkSelect={{this.toggleBulkSelect}}
@updateAutoAddTopicsToBulkSelect={{this.updateAutoAddTopicsToBulkSelect}}
/>

View File

@ -1,48 +1,19 @@
import Component from "@ember/component";
import { action } from "@ember/object";
import { next, schedule } from "@ember/runloop";
import { openBookmarkModal } from "discourse/controllers/bookmark";
import { ajax } from "discourse/lib/ajax";
import {
openLinkInNewTab,
shouldOpenInNewTab,
} from "discourse/lib/click-track";
import Scrolling from "discourse/mixins/scrolling";
import I18n from "I18n";
import { Promise } from "rsvp";
import { inject as service } from "@ember/service";
export default Component.extend(Scrolling, {
export default Component.extend({
dialog: service(),
classNames: ["bookmark-list-wrapper"],
didInsertElement() {
this._super(...arguments);
this.bindScrolling();
this.scrollToLastPosition();
},
willDestroyElement() {
this._super(...arguments);
this.unbindScrolling();
},
scrollToLastPosition() {
const scrollTo = this.session.bookmarkListScrollPosition;
if (scrollTo >= 0) {
schedule("afterRender", () => {
if (this.element && !this.isDestroying && !this.isDestroyed) {
next(() => window.scrollTo(0, scrollTo));
}
});
}
},
scrolled() {
this._super(...arguments);
this.session.set("bookmarkListScrollPosition", window.scrollY);
},
@action
removeBookmark(bookmark) {
return new Promise((resolve, reject) => {

View File

@ -1,36 +1,13 @@
import deprecated from "discourse-common/lib/deprecated";
import Component from "@ember/component";
import { scrollTop } from "discourse/mixins/scroll-top";
import { scheduleOnce } from "@ember/runloop";
// Can add a body class from within a component, also will scroll to the top automatically.
// Can add a body class from within a component
export default class extends Component {
tagName = null;
pageClass = null;
bodyClass = null;
scrollTop = true;
currentClasses = new Set();
didInsertElement() {
this._super(...arguments);
if (this.scrollTop === "false") {
deprecated("Uses boolean instead of string for scrollTop.", {
since: "2.8.0.beta9",
dropFrom: "2.9.0.beta1",
id: "discourse.d-section.scroll-top-boolean",
});
return;
}
if (!this.scrollTop) {
return;
}
scrollTop();
}
didReceiveAttrs() {
this._super(...arguments);
scheduleOnce("afterRender", this, this._updateClasses);

View File

@ -1 +0,0 @@
{{yield (hash saveScrollPosition=this.saveScrollPosition)}}

View File

@ -1,5 +1,4 @@
import { observes, on } from "discourse-common/utils/decorators";
import { next, schedule, scheduleOnce } from "@ember/runloop";
import Component from "@ember/component";
import LoadMore from "discourse/mixins/load-more";
import UrlRefresh from "discourse/mixins/url-refresh";
@ -10,21 +9,6 @@ export default Component.extend(UrlRefresh, LoadMore, {
eyelineSelector: ".topic-list-item",
documentTitle: service(),
@on("didInsertElement")
@observes("model")
_readjustScrollPosition() {
const scrollTo = this.session.topicListScrollPosition;
if (scrollTo >= 0) {
schedule("afterRender", () => {
if (this.element && !this.isDestroying && !this.isDestroyed) {
next(() => window.scrollTo(0, scrollTo));
}
});
} else {
scheduleOnce("afterRender", this, this.loadMoreUnlessFull);
}
},
@on("didInsertElement")
_monitorTrackingState() {
this.stateChangeCallbackId = this.topicTrackingState.onStateChange(() =>
@ -48,10 +32,6 @@ export default Component.extend(UrlRefresh, LoadMore, {
this.documentTitle.updateContextCount(this.incomingCount);
},
saveScrollPosition() {
this.session.set("topicListScrollPosition", $(window).scrollTop());
},
actions: {
loadMore() {
this.documentTitle.updateContextCount(0);
@ -64,7 +44,6 @@ export default Component.extend(UrlRefresh, LoadMore, {
) {
this.addTopicsToBulkSelect(newTopics);
}
schedule("afterRender", () => this.saveScrollPosition());
if (moreTopicsUrl && $(window).height() >= $(document).height()) {
this.send("loadMore");
}

View File

@ -1,9 +1,4 @@
<DSection
@pageClass="has-sidebar"
@id="d-sidebar"
@class="sidebar-container"
@scrollTop={{false}}
>
<DSection @pageClass="has-sidebar" @id="d-sidebar" @class="sidebar-container">
<Sidebar::Sections
@currentUser={{this.currentUser}}
@collapsableSections={{true}}

View File

@ -3,7 +3,6 @@ import discourseComputed, { observes } from "discourse-common/utils/decorators";
import Component from "@ember/component";
import LoadMore from "discourse/mixins/load-more";
import { on } from "@ember/object/evented";
import { next, schedule } from "@ember/runloop";
import showModal from "discourse/lib/show-modal";
export default Component.extend(LoadMore, {
@ -67,26 +66,6 @@ export default Component.extend(LoadMore, {
onScroll.call(this);
},
scrollToLastPosition() {
if (!this.scrollOnLoad) {
return;
}
const scrollTo = this.session.topicListScrollPosition;
if (scrollTo >= 0) {
schedule("afterRender", () => {
if (this.element && !this.isDestroying && !this.isDestroyed) {
next(() => window.scrollTo(0, scrollTo));
}
});
}
},
didInsertElement() {
this._super(...arguments);
this.scrollToLastPosition();
},
_updateLastVisitedTopic(topics, order, ascending, top) {
this.set("lastVisitedTopic", null);

View File

@ -6,10 +6,8 @@ import I18n from "I18n";
import LoadMore from "discourse/mixins/load-more";
import Post from "discourse/models/post";
import { NEW_TOPIC_KEY } from "discourse/models/composer";
import { observes } from "discourse-common/utils/decorators";
import { on } from "@ember/object/evented";
import { popupAjaxError } from "discourse/lib/ajax-error";
import { next, schedule } from "@ember/runloop";
import { inject as service } from "@ember/service";
export default Component.extend(LoadMore, {
@ -32,14 +30,7 @@ export default Component.extend(LoadMore, {
eyelineSelector: ".user-stream .item",
classNames: ["user-stream"],
@observes("stream.user.id")
_scrollTopOnModelChange() {
schedule("afterRender", () => $(document).scrollTop(0));
},
_inserted: on("didInsertElement", function () {
$(window).on("resize.discourse-on-scroll", () => this.scrolled());
$(this.element).on(
"click.details-disabled",
"details.disabled",
@ -49,12 +40,10 @@ export default Component.extend(LoadMore, {
return ClickTrack.trackClick(e, this.siteSettings);
});
this._updateLastDecoratedElement();
this._scrollToLastPosition();
}),
// This view is being removed. Shut down operations
_destroyed: on("willDestroyElement", function () {
$(window).unbind("resize.discourse-on-scroll");
$(this.element).off("click.details-disabled", "details.disabled");
// Unbind link tracking
@ -73,22 +62,6 @@ export default Component.extend(LoadMore, {
this._lastDecoratedElement = lastElement;
},
_scrollToLastPosition() {
const scrollTo = this.session.userStreamScrollPosition;
if (scrollTo >= 0) {
schedule("afterRender", () => {
if (this.element && !this.isDestroying && !this.isDestroyed) {
next(() => window.scrollTo(0, scrollTo));
}
});
}
},
scrolled() {
this._super(...arguments);
this.session.set("userStreamScrollPosition", window.scrollY);
},
actions: {
removeBookmark(userAction) {
const stream = this.stream;

View File

@ -25,10 +25,6 @@ export default Controller.extend(BulkTopicSelection, {
return topicsLength === 0 && incomingCount === 0;
},
saveScrollPosition() {
this.session.set("topicListScrollPosition", $(window).scrollTop());
},
@observes("model.canLoadMore")
_showFooter() {
this.set("application.showFooter", !this.get("model.canLoadMore"));

View File

@ -0,0 +1,5 @@
export default {
initialize(owner) {
owner.lookup("service:route-scroll-manager");
},
};

View File

@ -9,7 +9,6 @@ export function getCachedTopicList(session) {
export function resetCachedTopicList(session) {
session.setProperties({
topicList: null,
topicListScrollPosition: null,
});
}

View File

@ -6,6 +6,17 @@ let popstateFired = false;
const supportsHistoryState = window.history && "state" in window.history;
const popstateCallbacks = [];
function _uuid() {
return "xxxxxxxx-xxxx-4xxx-yxxx-xxxxxxxxxxxx".replace(/[xy]/g, function (c) {
let r, v;
/* eslint-disable no-bitwise */
r = (Math.random() * 16) | 0;
v = c === "x" ? r : (r & 3) | 8;
/* eslint-enable no-bitwise */
return v.toString(16);
});
}
/**
`Ember.DiscourseLocation` implements the location API using the browser's
`history.pushState` API.
@ -130,7 +141,7 @@ const DiscourseLocation = EmberObject.extend({
@param path {String}
*/
pushState(path) {
const state = { path };
const state = { path, uuid: _uuid() };
// store state if browser doesn't support `history.state`
if (!supportsHistoryState) {
@ -152,7 +163,7 @@ const DiscourseLocation = EmberObject.extend({
@param path {String}
*/
replaceState(path) {
const state = { path };
const state = { path, uuid: _uuid() };
// store state if browser doesn't support `history.state`
if (!supportsHistoryState) {

View File

@ -56,7 +56,7 @@ async function findTopicList(
session.set("topicList", null);
} else {
// Clear the cache
session.setProperties({ topicList: null, topicListScrollPosition: null });
session.setProperties({ topicList: null });
}
if (!list) {

View File

@ -5,9 +5,9 @@
import DiscourseRoute from "discourse/routes/discourse";
import OpenComposer from "discourse/mixins/open-composer";
import User from "discourse/models/user";
import { scrollTop } from "discourse/mixins/scroll-top";
import { setTopicList } from "discourse/lib/topic-list-tracker";
import { action } from "@ember/object";
import { resetCachedTopicList } from "discourse/lib/cached-topic-list";
export default DiscourseRoute.extend(OpenComposer, {
queryParams: {
@ -56,9 +56,6 @@ export default DiscourseRoute.extend(OpenComposer, {
@action
loadingComplete() {
this.controllerFor("discovery").loadingComplete();
if (!this.session.get("topicListScrollPosition")) {
scrollTop();
}
},
@action
@ -99,6 +96,11 @@ export default DiscourseRoute.extend(OpenComposer, {
});
},
refresh() {
resetCachedTopicList(this.session);
this._super();
},
@action
triggerRefresh() {
this.refresh();

View File

@ -21,6 +21,12 @@ const TopicRoute = DiscourseRoute.extend({
lastScrollPos: null,
isTransitioning: false,
buildRouteInfoMetadata() {
return {
scrollOnTransition: false,
};
},
redirect() {
return this.redirectIfLoginRequired();
},

View File

@ -25,7 +25,6 @@ export default DiscourseRoute.extend({
this.session.setProperties({
bookmarksModel: null,
bookmarkListScrollPosition: null,
});
controller.set("loading", true);

View File

@ -24,12 +24,6 @@ export default UserTopicListRoute.extend({
});
},
afterModel(model, transition) {
if (!this.isPoppedState(transition)) {
this.session.set("topicListScrollPosition", null);
}
},
emptyState() {
const title = I18n.t("user_activity.no_read_topics_title");
const body = htmlSafe(

View File

@ -21,10 +21,6 @@ export default DiscourseRoute.extend(ViewingActionType, {
},
afterModel(model, transition) {
if (!this.isPoppedState(transition)) {
this.session.set("userStreamScrollPosition", null);
}
return model.stream.filterBy({
filter: this.userActionType,
actingUsername: transition.to.queryParams.acting_username,

View File

@ -24,12 +24,6 @@ export default UserTopicListRoute.extend({
});
},
afterModel(model, transition) {
if (!this.isPoppedState(transition)) {
this.session.set("topicListScrollPosition", null);
}
},
emptyState() {
const user = this.modelFor("user");
let title, body;

View File

@ -11,12 +11,6 @@ export default DiscourseRoute.extend({
return user;
},
afterModel(_model, transition) {
if (!this.isPoppedState(transition)) {
this.session.set("userStreamScrollPosition", null);
}
},
setupController(controller, user) {
this.controllerFor("user-activity").set("model", user);
},

View File

@ -0,0 +1,92 @@
import Service, { inject as service } from "@ember/service";
import { bind } from "discourse-common/utils/decorators";
import { schedule } from "@ember/runloop";
import { isTesting } from "discourse-common/config/environment";
const MAX_SCROLL_LOCATIONS = 100;
/**
* This service is responsible for managing scroll position when transitioning.
* When visiting a new route, this service will scroll to the top of the page.
* When returning to a previously-visited route via the browser back button,
* this service will scroll to the previous scroll position.
*
* To opt-out of the behaviour, individual routes can add a scrollOnTransition
* boolean to their RouteInfo metadata using Ember's `buildRouteInfoMetadata` hook.
*/
export default class RouteScrollManager extends Service {
@service router;
scrollLocationHistory = new Map();
uuid;
scrollElement = isTesting()
? document.getElementById("ember-testing-container")
: document.scrollingElement;
@bind
routeWillChange() {
if (!this.uuid) {
return;
}
this.scrollLocationHistory.set(this.uuid, [
this.scrollElement.scrollLeft,
this.scrollElement.scrollTop,
]);
this.#pruneOldScrollLocations();
}
@bind
routeDidChange(transition) {
const newUuid = this.router.location.getState?.().uuid;
if (newUuid === this.uuid) {
// routeDidChange fired without the history state actually changing. Most likely a refresh.
// Forget the previously-stored scroll location so that we scroll to the top
this.scrollLocationHistory.delete(this.uuid);
}
this.uuid = newUuid;
if (!this.#shouldScroll(transition.to)) {
return;
}
const scrollLocation = this.scrollLocationHistory.get(this.uuid) || [0, 0];
schedule("afterRender", () => {
this.scrollElement.scrollTo(...scrollLocation);
});
}
#pruneOldScrollLocations() {
while (this.scrollLocationHistory.size > MAX_SCROLL_LOCATIONS) {
// JS Map guarantees keys will be returned in insertion order
const oldestUUID = this.scrollLocationHistory.keys().next().value;
this.scrollLocationHistory.delete(oldestUUID);
}
}
#shouldScroll(routeInfo) {
// Leafmost route has priority
for (let route = routeInfo; route; route = route.parent) {
const scrollOnTransition = route.metadata?.scrollOnTransition;
if (typeof scrollOnTransition === "boolean") {
return scrollOnTransition;
}
}
// No overrides - default to true
return true;
}
init() {
super.init(...arguments);
this.router.on("routeDidChange", this.routeDidChange);
this.router.on("routeWillChange", this.routeWillChange);
}
willDestroy() {
this.router.off("routeDidChange", this.routeDidChange);
this.router.off("routeWillChange", this.routeWillChange);
}
}

View File

@ -31,7 +31,6 @@
@autoAddTopicsToBulkSelect={{this.autoAddTopicsToBulkSelect}}
@bulkSelectEnabled={{this.bulkSelectEnabled}}
@addTopicsToBulkSelect={{action "addTopicsToBulkSelect"}}
as |discoveryTopicList|
>
{{#if this.top}}
<div class="top-lists">
@ -90,8 +89,6 @@
@category={{this.category}}
@topics={{this.model.topics}}
@discoveryList={{true}}
@scrollOnLoad={{true}}
@onScroll={{discoveryTopicList.saveScrollPosition}}
@focusLastVisitedTopic={{true}}
/>
{{/if}}

View File

@ -11,7 +11,6 @@
@model={{this.model}}
@refresh={{action "refresh"}}
@incomingCount={{this.topicTrackingState.incomingCount}}
as |discoveryTopicList|
>
{{#if this.top}}
<div class="top-lists">
@ -56,8 +55,6 @@
@expandAllPinned={{this.expandAllPinned}}
@category={{this.category}}
@topics={{this.model.topics}}
@scrollOnLoad={{true}}
@onScroll={{discoveryTopicList.saveScrollPosition}}
/>
{{/if}}
</DiscoveryTopicsList>

View File

@ -1,8 +1,4 @@
<DSection
@bodyClass="navigation-topics"
@class="navigation-container"
@scrollTop={{false}}
>
<DSection @bodyClass="navigation-topics" @class="navigation-container">
<DNavigation
@filterMode={{this.filterMode}}
@canCreateTopic={{this.canCreateTopic}}

View File

@ -1,8 +1,4 @@
<DSection
@bodyClass="navigation-filter"
@class="navigation-container"
@scrollTop={{false}}
>
<DSection @bodyClass="navigation-filter" @class="navigation-container">
<div class="topic-query-filter">
<div class="topic-query-filter__input">
{{d-icon "filter" class="topic-query-filter__icon"}}

View File

@ -95,7 +95,6 @@
@autoAddTopicsToBulkSelect={{this.autoAddTopicsToBulkSelect}}
@bulkSelectEnabled={{this.bulkSelectEnabled}}
@addTopicsToBulkSelect={{action "addTopicsToBulkSelect"}}
as |discoveryTopicList|
>
{{#if this.top}}
<div class="top-lists">
@ -140,8 +139,6 @@
@order={{this.order}}
@ascending={{this.ascending}}
@changeSort={{action "changeSort"}}
@onScroll={{discoveryTopicList.saveScrollPosition}}
@scrollOnLoad={{true}}
@focusLastVisitedTopic={{true}}
/>
{{/if}}

View File

@ -43,9 +43,7 @@
@bulkSelectAction={{action "refresh"}}
@selected={{this.selected}}
@tagsForUser={{this.tagsForUser}}
@onScroll={{this.saveScrollPosition}}
@canBulkSelect={{this.canBulkSelect}}
@scrollOnLoad={{true}}
@toggleBulkSelect={{action "toggleBulkSelect"}}
@updateAutoAddTopicsToBulkSelect={{action
"updateAutoAddTopicsToBulkSelect"

View File

@ -10,8 +10,12 @@ module PageObjects
TOPIC_LIST_BODY_SELECTOR
end
def has_topics?(count:)
page.has_css?(TOPIC_LIST_ITEM_SELECTOR, count: count)
def has_topics?(count: nil)
if count.nil?
page.has_css?(TOPIC_LIST_ITEM_SELECTOR)
else
page.has_css?(TOPIC_LIST_ITEM_SELECTOR, count: count)
end
end
def has_no_topics?

View File

@ -0,0 +1,45 @@
# frozen_string_literal: true
describe "Ember route-scroll-manager service", type: :system do
before do
Fabricate(:admin)
Fabricate.times(50, :post)
end
let(:discovery) { PageObjects::Pages::Discovery.new }
let(:topic) { PageObjects::Pages::Topic.new }
def current_scroll_y
page.evaluate_script("window.scrollY")
end
it "scrolls to top when navigating to new routes, and remembers scroll position when going back" do
visit("/")
expect(page).to have_css("body.navigation-topics")
expect(discovery.topic_list).to have_topics
page.execute_script <<~JS
document.querySelectorAll('.topic-list-item')[10].scrollIntoView(true);
JS
topic_list_scroll_y = current_scroll_y
try_until_success { expect(topic_list_scroll_y).to be > 0 }
find(".sidebar-section-link[data-link-name='all-categories']").click
expect(page).to have_css("body.navigation-categories")
try_until_success { expect(current_scroll_y).to eq(0) }
page.go_back
expect(page).to have_css("body.navigation-topics")
expect(discovery.topic_list).to have_topics
try_until_success { expect(current_scroll_y).to eq(topic_list_scroll_y) }
# Clicking site logo triggers refresh and scrolls to top
find("#site-logo").click
try_until_success { expect(current_scroll_y).to eq(0) }
end
end