From dfe94ba11887527550c9060b47497aeb9103cded Mon Sep 17 00:00:00 2001 From: David Taylor Date: Thu, 13 Jul 2023 13:40:08 +0100 Subject: [PATCH] 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. --- .../app/components/basic-topic-list.hbs | 2 - .../discourse/app/components/bookmark-list.js | 31 +------ .../discourse/app/components/d-section.js | 25 +---- .../app/components/discovery-topics-list.hbs | 1 - .../app/components/discovery-topics-list.js | 21 ----- .../discourse/app/components/sidebar.hbs | 7 +- .../discourse/app/components/topic-list.js | 21 ----- .../discourse/app/components/user-stream.js | 27 ------ .../app/controllers/user-topics-list.js | 4 - .../init-route-scroll-manager.js | 5 + .../discourse/app/lib/cached-topic-list.js | 1 - .../discourse/app/lib/discourse-location.js | 15 ++- .../discourse/app/routes/build-topic-route.js | 2 +- .../discourse/app/routes/discovery.js | 10 +- .../javascripts/discourse/app/routes/topic.js | 6 ++ .../app/routes/user-activity-bookmarks.js | 1 - .../app/routes/user-activity-read.js | 6 -- .../app/routes/user-activity-stream.js | 4 - .../app/routes/user-activity-topics.js | 6 -- .../discourse/app/routes/user-activity.js | 6 -- .../app/services/route-scroll-manager.js | 92 +++++++++++++++++++ .../app/templates/discovery/topics.hbs | 3 - .../app/templates/mobile/discovery/topics.hbs | 3 - .../app/templates/navigation/default.hbs | 6 +- .../app/templates/navigation/filter.hbs | 6 +- .../discourse/app/templates/tag/show.hbs | 3 - .../app/templates/user-topics-list.hbs | 2 - .../page_objects/components/topic_list.rb | 8 +- spec/system/scroll_manager_service_spec.rb | 45 +++++++++ 29 files changed, 179 insertions(+), 190 deletions(-) delete mode 100644 app/assets/javascripts/discourse/app/components/discovery-topics-list.hbs create mode 100644 app/assets/javascripts/discourse/app/instance-initializers/init-route-scroll-manager.js create mode 100644 app/assets/javascripts/discourse/app/services/route-scroll-manager.js create mode 100644 spec/system/scroll_manager_service_spec.rb diff --git a/app/assets/javascripts/discourse/app/components/basic-topic-list.hbs b/app/assets/javascripts/discourse/app/components/basic-topic-list.hbs index 596973ac661..478050fce40 100644 --- a/app/assets/javascripts/discourse/app/components/basic-topic-list.hbs +++ b/app/assets/javascripts/discourse/app/components/basic-topic-list.hbs @@ -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}} /> diff --git a/app/assets/javascripts/discourse/app/components/bookmark-list.js b/app/assets/javascripts/discourse/app/components/bookmark-list.js index 15dd1f42cab..f13a4282aa5 100644 --- a/app/assets/javascripts/discourse/app/components/bookmark-list.js +++ b/app/assets/javascripts/discourse/app/components/bookmark-list.js @@ -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) => { diff --git a/app/assets/javascripts/discourse/app/components/d-section.js b/app/assets/javascripts/discourse/app/components/d-section.js index 515f4b5ed75..3746a3855d6 100644 --- a/app/assets/javascripts/discourse/app/components/d-section.js +++ b/app/assets/javascripts/discourse/app/components/d-section.js @@ -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); diff --git a/app/assets/javascripts/discourse/app/components/discovery-topics-list.hbs b/app/assets/javascripts/discourse/app/components/discovery-topics-list.hbs deleted file mode 100644 index 5bd46512095..00000000000 --- a/app/assets/javascripts/discourse/app/components/discovery-topics-list.hbs +++ /dev/null @@ -1 +0,0 @@ -{{yield (hash saveScrollPosition=this.saveScrollPosition)}} \ No newline at end of file diff --git a/app/assets/javascripts/discourse/app/components/discovery-topics-list.js b/app/assets/javascripts/discourse/app/components/discovery-topics-list.js index 2de7ed73ef7..9a1e6038c07 100644 --- a/app/assets/javascripts/discourse/app/components/discovery-topics-list.js +++ b/app/assets/javascripts/discourse/app/components/discovery-topics-list.js @@ -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"); } diff --git a/app/assets/javascripts/discourse/app/components/sidebar.hbs b/app/assets/javascripts/discourse/app/components/sidebar.hbs index 8ed675f33b6..516b3d76941 100644 --- a/app/assets/javascripts/discourse/app/components/sidebar.hbs +++ b/app/assets/javascripts/discourse/app/components/sidebar.hbs @@ -1,9 +1,4 @@ - + = 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); diff --git a/app/assets/javascripts/discourse/app/components/user-stream.js b/app/assets/javascripts/discourse/app/components/user-stream.js index b6aa0dd839c..e170d91e4f7 100644 --- a/app/assets/javascripts/discourse/app/components/user-stream.js +++ b/app/assets/javascripts/discourse/app/components/user-stream.js @@ -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; diff --git a/app/assets/javascripts/discourse/app/controllers/user-topics-list.js b/app/assets/javascripts/discourse/app/controllers/user-topics-list.js index a40ab97f4e9..ecee3f764b9 100644 --- a/app/assets/javascripts/discourse/app/controllers/user-topics-list.js +++ b/app/assets/javascripts/discourse/app/controllers/user-topics-list.js @@ -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")); diff --git a/app/assets/javascripts/discourse/app/instance-initializers/init-route-scroll-manager.js b/app/assets/javascripts/discourse/app/instance-initializers/init-route-scroll-manager.js new file mode 100644 index 00000000000..8e7a64c8672 --- /dev/null +++ b/app/assets/javascripts/discourse/app/instance-initializers/init-route-scroll-manager.js @@ -0,0 +1,5 @@ +export default { + initialize(owner) { + owner.lookup("service:route-scroll-manager"); + }, +}; diff --git a/app/assets/javascripts/discourse/app/lib/cached-topic-list.js b/app/assets/javascripts/discourse/app/lib/cached-topic-list.js index 1cf2000a7f9..a32d75990f9 100644 --- a/app/assets/javascripts/discourse/app/lib/cached-topic-list.js +++ b/app/assets/javascripts/discourse/app/lib/cached-topic-list.js @@ -9,7 +9,6 @@ export function getCachedTopicList(session) { export function resetCachedTopicList(session) { session.setProperties({ topicList: null, - topicListScrollPosition: null, }); } diff --git a/app/assets/javascripts/discourse/app/lib/discourse-location.js b/app/assets/javascripts/discourse/app/lib/discourse-location.js index d672978f269..39a18a7fbd3 100644 --- a/app/assets/javascripts/discourse/app/lib/discourse-location.js +++ b/app/assets/javascripts/discourse/app/lib/discourse-location.js @@ -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) { diff --git a/app/assets/javascripts/discourse/app/routes/build-topic-route.js b/app/assets/javascripts/discourse/app/routes/build-topic-route.js index feea4a2a098..ae18ee2b867 100644 --- a/app/assets/javascripts/discourse/app/routes/build-topic-route.js +++ b/app/assets/javascripts/discourse/app/routes/build-topic-route.js @@ -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) { diff --git a/app/assets/javascripts/discourse/app/routes/discovery.js b/app/assets/javascripts/discourse/app/routes/discovery.js index f3047a999c9..0434c228178 100644 --- a/app/assets/javascripts/discourse/app/routes/discovery.js +++ b/app/assets/javascripts/discourse/app/routes/discovery.js @@ -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(); diff --git a/app/assets/javascripts/discourse/app/routes/topic.js b/app/assets/javascripts/discourse/app/routes/topic.js index 1cb1ba31ac1..520c3f3896f 100644 --- a/app/assets/javascripts/discourse/app/routes/topic.js +++ b/app/assets/javascripts/discourse/app/routes/topic.js @@ -21,6 +21,12 @@ const TopicRoute = DiscourseRoute.extend({ lastScrollPos: null, isTransitioning: false, + buildRouteInfoMetadata() { + return { + scrollOnTransition: false, + }; + }, + redirect() { return this.redirectIfLoginRequired(); }, diff --git a/app/assets/javascripts/discourse/app/routes/user-activity-bookmarks.js b/app/assets/javascripts/discourse/app/routes/user-activity-bookmarks.js index fcbea962c38..5e6857816ea 100644 --- a/app/assets/javascripts/discourse/app/routes/user-activity-bookmarks.js +++ b/app/assets/javascripts/discourse/app/routes/user-activity-bookmarks.js @@ -25,7 +25,6 @@ export default DiscourseRoute.extend({ this.session.setProperties({ bookmarksModel: null, - bookmarkListScrollPosition: null, }); controller.set("loading", true); diff --git a/app/assets/javascripts/discourse/app/routes/user-activity-read.js b/app/assets/javascripts/discourse/app/routes/user-activity-read.js index c988fad68e5..8a0fbbe2eb0 100644 --- a/app/assets/javascripts/discourse/app/routes/user-activity-read.js +++ b/app/assets/javascripts/discourse/app/routes/user-activity-read.js @@ -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( diff --git a/app/assets/javascripts/discourse/app/routes/user-activity-stream.js b/app/assets/javascripts/discourse/app/routes/user-activity-stream.js index 87776a2d21f..05d81f4c46c 100644 --- a/app/assets/javascripts/discourse/app/routes/user-activity-stream.js +++ b/app/assets/javascripts/discourse/app/routes/user-activity-stream.js @@ -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, diff --git a/app/assets/javascripts/discourse/app/routes/user-activity-topics.js b/app/assets/javascripts/discourse/app/routes/user-activity-topics.js index 6e020374477..d1064f0fa0e 100644 --- a/app/assets/javascripts/discourse/app/routes/user-activity-topics.js +++ b/app/assets/javascripts/discourse/app/routes/user-activity-topics.js @@ -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; diff --git a/app/assets/javascripts/discourse/app/routes/user-activity.js b/app/assets/javascripts/discourse/app/routes/user-activity.js index 2638e1d7afa..f6bd36c49c3 100644 --- a/app/assets/javascripts/discourse/app/routes/user-activity.js +++ b/app/assets/javascripts/discourse/app/routes/user-activity.js @@ -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); }, diff --git a/app/assets/javascripts/discourse/app/services/route-scroll-manager.js b/app/assets/javascripts/discourse/app/services/route-scroll-manager.js new file mode 100644 index 00000000000..0f9e63158de --- /dev/null +++ b/app/assets/javascripts/discourse/app/services/route-scroll-manager.js @@ -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); + } +} diff --git a/app/assets/javascripts/discourse/app/templates/discovery/topics.hbs b/app/assets/javascripts/discourse/app/templates/discovery/topics.hbs index ae2c06b85d7..c4c1129178a 100644 --- a/app/assets/javascripts/discourse/app/templates/discovery/topics.hbs +++ b/app/assets/javascripts/discourse/app/templates/discovery/topics.hbs @@ -31,7 +31,6 @@ @autoAddTopicsToBulkSelect={{this.autoAddTopicsToBulkSelect}} @bulkSelectEnabled={{this.bulkSelectEnabled}} @addTopicsToBulkSelect={{action "addTopicsToBulkSelect"}} - as |discoveryTopicList| > {{#if this.top}}
@@ -90,8 +89,6 @@ @category={{this.category}} @topics={{this.model.topics}} @discoveryList={{true}} - @scrollOnLoad={{true}} - @onScroll={{discoveryTopicList.saveScrollPosition}} @focusLastVisitedTopic={{true}} /> {{/if}} diff --git a/app/assets/javascripts/discourse/app/templates/mobile/discovery/topics.hbs b/app/assets/javascripts/discourse/app/templates/mobile/discovery/topics.hbs index 5dbe608e694..869f996736a 100644 --- a/app/assets/javascripts/discourse/app/templates/mobile/discovery/topics.hbs +++ b/app/assets/javascripts/discourse/app/templates/mobile/discovery/topics.hbs @@ -11,7 +11,6 @@ @model={{this.model}} @refresh={{action "refresh"}} @incomingCount={{this.topicTrackingState.incomingCount}} - as |discoveryTopicList| > {{#if this.top}}
@@ -56,8 +55,6 @@ @expandAllPinned={{this.expandAllPinned}} @category={{this.category}} @topics={{this.model.topics}} - @scrollOnLoad={{true}} - @onScroll={{discoveryTopicList.saveScrollPosition}} /> {{/if}} diff --git a/app/assets/javascripts/discourse/app/templates/navigation/default.hbs b/app/assets/javascripts/discourse/app/templates/navigation/default.hbs index 9848aadf5fd..c075d552437 100644 --- a/app/assets/javascripts/discourse/app/templates/navigation/default.hbs +++ b/app/assets/javascripts/discourse/app/templates/navigation/default.hbs @@ -1,8 +1,4 @@ - + +
{{d-icon "filter" class="topic-query-filter__icon"}} diff --git a/app/assets/javascripts/discourse/app/templates/tag/show.hbs b/app/assets/javascripts/discourse/app/templates/tag/show.hbs index 13e5dedb80a..53289e6c310 100644 --- a/app/assets/javascripts/discourse/app/templates/tag/show.hbs +++ b/app/assets/javascripts/discourse/app/templates/tag/show.hbs @@ -95,7 +95,6 @@ @autoAddTopicsToBulkSelect={{this.autoAddTopicsToBulkSelect}} @bulkSelectEnabled={{this.bulkSelectEnabled}} @addTopicsToBulkSelect={{action "addTopicsToBulkSelect"}} - as |discoveryTopicList| > {{#if this.top}}
@@ -140,8 +139,6 @@ @order={{this.order}} @ascending={{this.ascending}} @changeSort={{action "changeSort"}} - @onScroll={{discoveryTopicList.saveScrollPosition}} - @scrollOnLoad={{true}} @focusLastVisitedTopic={{true}} /> {{/if}} diff --git a/app/assets/javascripts/discourse/app/templates/user-topics-list.hbs b/app/assets/javascripts/discourse/app/templates/user-topics-list.hbs index cdc84cb42e0..e1a46665d13 100644 --- a/app/assets/javascripts/discourse/app/templates/user-topics-list.hbs +++ b/app/assets/javascripts/discourse/app/templates/user-topics-list.hbs @@ -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" diff --git a/spec/system/page_objects/components/topic_list.rb b/spec/system/page_objects/components/topic_list.rb index 69f6d775032..ab7d0a6a960 100644 --- a/spec/system/page_objects/components/topic_list.rb +++ b/spec/system/page_objects/components/topic_list.rb @@ -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? diff --git a/spec/system/scroll_manager_service_spec.rb b/spec/system/scroll_manager_service_spec.rb new file mode 100644 index 00000000000..eb8a3d57dba --- /dev/null +++ b/spec/system/scroll_manager_service_spec.rb @@ -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