diff --git a/app/assets/javascripts/discourse/app/components/topic-entrance.js b/app/assets/javascripts/discourse/app/components/topic-entrance.js index 493d426c727..e43f4762145 100644 --- a/app/assets/javascripts/discourse/app/components/topic-entrance.js +++ b/app/assets/javascripts/discourse/app/components/topic-entrance.js @@ -33,6 +33,7 @@ function entranceDate(dt, showTime) { export default Component.extend(CleansUp, { router: service(), session: service(), + historyStore: service(), elementId: "topic-entrance", classNameBindings: ["visible::hidden"], topic: null, @@ -166,10 +167,7 @@ export default Component.extend(CleansUp, { }, _jumpTo(destination) { - this.session.set("lastTopicIdViewed", { - topicId: this.topic.id, - historyUuid: this.router.location.getState?.().uuid, - }); + this.historyStore.set("lastTopicIdViewed", this.topic.id); this.cleanUp(); DiscourseURL.routeTo(destination); diff --git a/app/assets/javascripts/discourse/app/components/topic-list-item.js b/app/assets/javascripts/discourse/app/components/topic-list-item.js index d67528ebf88..590dc594fcb 100644 --- a/app/assets/javascripts/discourse/app/components/topic-list-item.js +++ b/app/assets/javascripts/discourse/app/components/topic-list-item.js @@ -37,14 +37,8 @@ export function showEntrance(e) { } export function navigateToTopic(topic, href) { - const owner = getOwner(this); - const router = owner.lookup("service:router"); - const session = owner.lookup("service:session"); - - session.set("lastTopicIdViewed", { - topicId: topic.id, - historyUuid: router.location.getState?.().uuid, - }); + const historyStore = getOwner(this).lookup("service:history-store"); + historyStore.set("lastTopicIdViewed", topic.id); DiscourseURL.routeTo(href || topic.get("url")); return false; @@ -52,6 +46,7 @@ export function navigateToTopic(topic, href) { export default Component.extend({ router: service(), + historyStore: service(), tagName: "tr", classNameBindings: [":topic-list-item", "unboundClassNames", "topic.visited"], attributeBindings: ["data-topic-id", "role", "ariaLevel:aria-level"], @@ -346,15 +341,11 @@ export default Component.extend({ _highlightIfNeeded: on("didInsertElement", function () { // highlight the last topic viewed - const lastViewedTopicInfo = this.session.get("lastTopicIdViewed"); - - const isLastViewedTopic = - lastViewedTopicInfo?.topicId === this.topic.id && - lastViewedTopicInfo?.historyUuid === - this.router.location.getState?.().uuid; + const lastViewedTopicId = this.historyStore.get("lastTopicIdViewed"); + const isLastViewedTopic = lastViewedTopicId === this.topic.id; if (isLastViewedTopic) { - this.session.set("lastTopicIdViewed", null); + this.historyStore.delete("lastTopicIdViewed"); this.highlight({ isLastViewedTopic: true }); } else if (this.get("topic.highlight")) { // highlight new topics that have been loaded from the server or the one we just created diff --git a/app/assets/javascripts/discourse/app/routes/application.js b/app/assets/javascripts/discourse/app/routes/application.js index 6d5611e44b0..3f5bec02b13 100644 --- a/app/assets/javascripts/discourse/app/routes/application.js +++ b/app/assets/javascripts/discourse/app/routes/application.js @@ -42,6 +42,7 @@ const ApplicationRoute = DiscourseRoute.extend({ siteSettings: service(), clientErrorHandler: service(), login: service(), + historyStore: service(), get isOnlyOneExternalLoginMethod() { return ( @@ -63,6 +64,12 @@ const ApplicationRoute = DiscourseRoute.extend({ return false; }, + @action + willResolveModel(transition) { + this.historyStore.willResolveModel(transition); + return true; + }, + actions: { toggleMobileView() { mobile.toggleMobileView(); diff --git a/app/assets/javascripts/discourse/app/routes/build-category-route.js b/app/assets/javascripts/discourse/app/routes/build-category-route.js index fb402aebea5..eb1b7f93b43 100644 --- a/app/assets/javascripts/discourse/app/routes/build-category-route.js +++ b/app/assets/javascripts/discourse/app/routes/build-category-route.js @@ -21,6 +21,7 @@ class AbstractCategoryRoute extends DiscourseRoute { @service store; @service topicTrackingState; @service("search") searchService; + @service historyStore; queryParams = queryParams; @@ -86,7 +87,7 @@ class AbstractCategoryRoute extends DiscourseRoute { async _retrieveTopicList(category, transition, modelParams) { const findOpts = filterQueryParams(modelParams, this.routeConfig); - const extras = { cached: this.isPoppedState(transition) }; + const extras = { cached: this.historyStore.isPoppedState }; let listFilter = `c/${Category.slugFor(category)}/${category.id}`; if (findOpts.no_subcategories) { 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 275cacf44a8..d2fdbaac1bb 100644 --- a/app/assets/javascripts/discourse/app/routes/build-topic-route.js +++ b/app/assets/javascripts/discourse/app/routes/build-topic-route.js @@ -98,17 +98,18 @@ class AbstractTopicRoute extends DiscourseRoute { @service store; @service topicTrackingState; @service currentUser; + @service historyStore; queryParams = queryParams; templateName = "discovery/list"; controllerName = "discovery/list"; - async model(data, transition) { + async model(data) { // attempt to stop early cause we need this to be called before .sync this.screenTrack.stop(); const findOpts = filterQueryParams(data), - findExtras = { cached: this.isPoppedState(transition) }; + findExtras = { cached: this.historyStore.isPoppedState }; const topicListPromise = findTopicList( this.store, diff --git a/app/assets/javascripts/discourse/app/routes/discourse.js b/app/assets/javascripts/discourse/app/routes/discourse.js index 7a545bbd474..a4ba7bd378f 100644 --- a/app/assets/javascripts/discourse/app/routes/discourse.js +++ b/app/assets/javascripts/discourse/app/routes/discourse.js @@ -65,13 +65,6 @@ const DiscourseRoute = Route.extend({ return user.id === this.currentUser.id; }, - - isPoppedState(transition) { - return ( - !transition._discourse_intercepted && - (!!transition.intent.url || !!transition.queryParamsOnly) - ); - }, }); export default DiscourseRoute; diff --git a/app/assets/javascripts/discourse/app/routes/tag-show.js b/app/assets/javascripts/discourse/app/routes/tag-show.js index 75f41e89040..19926a8ffd5 100644 --- a/app/assets/javascripts/discourse/app/routes/tag-show.js +++ b/app/assets/javascripts/discourse/app/routes/tag-show.js @@ -26,6 +26,7 @@ export default class TagShowRoute extends DiscourseRoute { @service store; @service topicTrackingState; @service("search") searchService; + @service historyStore; queryParams = queryParams; controllerName = "discovery/list"; @@ -119,7 +120,7 @@ export default class TagShowRoute extends DiscourseRoute { filter, filteredQueryParams, { - cached: this.isPoppedState(transition), + cached: this.historyStore.isPoppedState, } ); 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 b3e115bf702..ecc7a7fbece 100644 --- a/app/assets/javascripts/discourse/app/routes/user-activity-bookmarks.js +++ b/app/assets/javascripts/discourse/app/routes/user-activity-bookmarks.js @@ -1,4 +1,5 @@ import { action } from "@ember/object"; +import { inject as service } from "@ember/service"; import $ from "jquery"; import { Promise } from "rsvp"; import { ajax } from "discourse/lib/ajax"; @@ -6,6 +7,7 @@ import DiscourseRoute from "discourse/routes/discourse"; import I18n from "discourse-i18n"; export default DiscourseRoute.extend({ + historyStore: service(), templateName: "user/bookmarks", queryParams: { @@ -13,11 +15,11 @@ export default DiscourseRoute.extend({ q: { refreshModel: true }, }, - model(params, transition) { + model(params) { const controller = this.controllerFor("user-activity-bookmarks"); if ( - this.isPoppedState(transition) && + this.historyStore.isPoppedState && this.session.bookmarksModel && this.session.bookmarksModel.searchTerm === params.q ) { diff --git a/app/assets/javascripts/discourse/app/services/history-store.js b/app/assets/javascripts/discourse/app/services/history-store.js new file mode 100644 index 00000000000..c4ccf644ea6 --- /dev/null +++ b/app/assets/javascripts/discourse/app/services/history-store.js @@ -0,0 +1,133 @@ +import { DEBUG } from "@glimmer/env"; +import Service, { inject as service } from "@ember/service"; +import { TrackedMap } from "@ember-compat/tracked-built-ins"; +import { disableImplicitInjections } from "discourse/lib/implicit-injections"; +import { isTesting } from "discourse-common/config/environment"; +import { bind } from "discourse-common/utils/decorators"; + +const HISTORY_SIZE = 100; +const HISTORIC_KEY = Symbol("historic"); +const HANDLED_TRANSITIONS = new WeakSet(); + +/** + * This service provides a key-value store which can store per-route information. + * When navigating 'back' via browser controls, the service will restore the data + * for the appropriate route. + */ +@disableImplicitInjections +export default class HistoryStore extends Service { + @service router; + + #routeData = new Map(); + #uuid; + #pendingStore; + + get #currentStore() { + if (this.#pendingStore) { + return this.#pendingStore; + } + + return this.#dataFor(this.#uuid); + } + + /** + * Identify if the current route was accessed via the browser back/forward buttons + * @returns {boolean} + */ + get isPoppedState() { + return !!this.get(HISTORIC_KEY); + } + + /** + * Fetch a value from the current route's key/value store + */ + get(key) { + return this.#currentStore.get(key); + } + + /** + * Set a value in the current route's key/value store. Will persist for the lifetime + * of the route, and will be restored if the user navigates 'back' to the route. + */ + set(key, value) { + return this.#currentStore.set(key, value); + } + + /** + * Delete a value from the current route's key/value store + */ + delete(key) { + return this.#currentStore.delete(key); + } + + #pruneOldData() { + while (this.#routeData.size > HISTORY_SIZE) { + // JS Map guarantees keys will be returned in insertion order + const oldestUUID = this.#routeData.keys().next().value; + this.#routeData.delete(oldestUUID); + } + } + + #dataFor(uuid) { + let data = this.#routeData.get(uuid); + if (data) { + return data; + } + + data = new TrackedMap(); + this.#routeData.set(uuid, data); + this.#pruneOldData(); + + return data; + } + + /** + * Called by the Application route when its willResolveModel hook + * is triggered by the ember router. Unfortunately this hook is + * not available as an event on the router service. + */ + @bind + willResolveModel(transition) { + if (HANDLED_TRANSITIONS.has(transition)) { + return; + } + HANDLED_TRANSITIONS.add(transition); + + if (DEBUG && isTesting()) { + // Can't use window.history in tests + this.#pendingStore = new TrackedMap(); + return; + } + + this.set(HISTORIC_KEY, true); + + let pendingStoreForThisTransition; + + if (this.#uuid === window.history.state?.uuid) { + // A normal ember transition. The history uuid will only change **after** models are resolved. + // To allow routes to store data for the upcoming uuid, we set up a temporary data store + // and then persist it if/when the transition succeeds. + pendingStoreForThisTransition = new TrackedMap(); + } else { + // A transition initiated by the browser back/forward buttons. We might already have some stored + // data for this route. If so, take a copy of it and use that as the pending store. As with normal transitions, + // it'll be persisted if/when the transition succeeds. + pendingStoreForThisTransition = new TrackedMap( + this.#dataFor(window.history.state?.uuid)?.entries() + ); + } + + this.#pendingStore = pendingStoreForThisTransition; + transition + .then(() => { + this.#uuid = window.history.state?.uuid; + this.#routeData.set(this.#uuid, this.#pendingStore); + this.#pruneOldData(); + }) + .finally(() => { + if (pendingStoreForThisTransition === this.#pendingStore) { + this.#pendingStore = null; + } + }); + } +} diff --git a/app/assets/javascripts/discourse/app/services/route-scroll-manager.js b/app/assets/javascripts/discourse/app/services/route-scroll-manager.js index 36330d6aa6d..51f20818534 100644 --- a/app/assets/javascripts/discourse/app/services/route-scroll-manager.js +++ b/app/assets/javascripts/discourse/app/services/route-scroll-manager.js @@ -4,7 +4,7 @@ import { disableImplicitInjections } from "discourse/lib/implicit-injections"; import { isTesting } from "discourse-common/config/environment"; import { bind } from "discourse-common/utils/decorators"; -const MAX_SCROLL_LOCATIONS = 100; +const STORE_KEY = Symbol("scroll-location"); /** * This service is responsible for managing scroll position when transitioning. @@ -18,9 +18,7 @@ const MAX_SCROLL_LOCATIONS = 100; @disableImplicitInjections export default class RouteScrollManager extends Service { @service router; - - scrollLocationHistory = new Map(); - uuid; + @service historyStore; scrollElement = isTesting() ? document.getElementById("ember-testing-container") @@ -28,14 +26,10 @@ export default class RouteScrollManager extends Service { @bind routeWillChange() { - if (!this.uuid) { - return; - } - this.scrollLocationHistory.set(this.uuid, [ + this.historyStore.set(STORE_KEY, [ this.scrollElement.scrollLeft, this.scrollElement.scrollTop, ]); - this.#pruneOldScrollLocations(); } @bind @@ -44,34 +38,16 @@ export default class RouteScrollManager extends Service { return; } - 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]; + const scrollLocation = this.historyStore.get(STORE_KEY) || [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) { diff --git a/spec/system/topic_list_focus_spec.rb b/spec/system/topic_list_focus_spec.rb index 4c6a2008f7a..75b93c75e35 100644 --- a/spec/system/topic_list_focus_spec.rb +++ b/spec/system/topic_list_focus_spec.rb @@ -81,4 +81,26 @@ describe "Topic list focus", type: :system do expect(page).to have_css("body.navigation-topics") expect(focussed_topic_id).to eq(nil) end + + it "refocusses properly when there are multiple pages of topics" do + extra_topics = Fabricate.times(25, :post).map(&:topic) + oldest_topic = Fabricate(:post).topic + oldest_topic.update(bumped_at: 1.day.ago) + + visit("/latest") + + # Scroll to bottom for infinite load + page.execute_script <<~JS + document.querySelectorAll('.topic-list-item')[24].scrollIntoView(true); + JS + + # Click a topic + discovery.topic_list.visit_topic(oldest_topic) + expect(topic).to have_topic_title(oldest_topic.title) + + # Going back to the topic-list should re-focus + page.go_back + expect(page).to have_css("body.navigation-topics") + expect(focussed_topic_id).to eq(oldest_topic.id) + end end