FIX: Only use lastViewedTopic when going 'back' to a topic list (#22594)

Using the lastViewedTopicId indiscriminately can cause strange scrolling behavior when navigating to a **different** topic list after viewing a topic. We only want to refocus the topic when going 'back' to the same topic list which originally triggered the navigation.

app/assets/javascripts/discourse/app/components/topic-list-item.js

@@ -13,6 +13,7 @@ import { schedule } from "@ember/runloop";
 import { topicTitleDecorators } from "discourse/components/topic-title";
 import { wantsNewWindow } from "discourse/lib/intercept-click";
 import { htmlSafe } from "@ember/template";
+import { inject as service } from "@ember/service";
 
 export function showEntrance(e) {
   let target = $(e.target);
@@ -39,11 +40,18 @@ export function navigateToTopic(topic, href) {
     // so skip setting it early.
     this.appEvents.trigger("header:update-topic", topic);
   }
+
+  this.session.set("lastTopicIdViewed", {
+    topicId: topic.id,
+    historyUuid: this.router.location.getState?.().uuid,
+  });
+
   DiscourseURL.routeTo(href || topic.get("url"));
   return false;
 }
 
 export default Component.extend({
+  router: service(),
   tagName: "tr",
   classNameBindings: [":topic-list-item", "unboundClassNames", "topic.visited"],
   attributeBindings: ["data-topic-id", "role", "ariaLevel:aria-level"],
@@ -328,7 +336,14 @@ export default Component.extend({
 
   _highlightIfNeeded: on("didInsertElement", function () {
     // highlight the last topic viewed
-    if (this.session.get("lastTopicIdViewed") === this.get("topic.id")) {
+    const lastViewedTopicInfo = this.session.get("lastTopicIdViewed");
+
+    const isLastViewedTopic =
+      lastViewedTopicInfo?.topicId === this.topic.id &&
+      lastViewedTopicInfo?.historyUuid ===
+        this.router.location.getState?.().uuid;
+
+    if (isLastViewedTopic) {
       this.session.set("lastTopicIdViewed", null);
       this.highlight({ isLastViewedTopic: true });
     } else if (this.get("topic.highlight")) {

app/assets/javascripts/discourse/app/routes/topic.js

@@ -323,9 +323,6 @@ const TopicRoute = DiscourseRoute.extend({
   activate() {
     this._super(...arguments);
     this.set("isTransitioning", false);
-
-    const topic = this.modelFor("topic");
-    this.session.set("lastTopicIdViewed", parseInt(topic.get("id"), 10));
   },
 
   deactivate() {

app/assets/javascripts/discourse/tests/acceptance/last-visited-topic-focus-test.js

@@ -1,26 +0,0 @@
-import { acceptance, query } from "discourse/tests/helpers/qunit-helpers";
-import { test } from "qunit";
-import { visit } from "@ember/test-helpers";
-import { cloneJSON } from "discourse-common/lib/object";
-import topicFixtures from "discourse/tests/fixtures/topic";
-
-acceptance("Last Visited Topic Focus", function (needs) {
-  needs.pretender((server, helper) => {
-    const fixture = cloneJSON(topicFixtures["/t/54077.json"]);
-    fixture.id = 11996;
-    fixture.slug =
-      "its-really-hard-to-navigate-the-create-topic-reply-pane-with-the-keyboard";
-    server.get("/t/11996.json", () => helper.response(fixture));
-  });
-  test("last visited topic receives focus when you return back to the topic list", async function (assert) {
-    await visit("/");
-    await visit(
-      "/t/its-really-hard-to-navigate-the-create-topic-reply-pane-with-the-keyboard/11996"
-    );
-    await visit("/");
-    const visitedTopicTitle = query(
-      '.topic-list-body tr[data-topic-id="11996"] .main-link'
-    );
-    assert.ok(visitedTopicTitle.classList.contains("focused"));
-  });
-});

spec/system/page_objects/components/topic_list.rb

@@ -34,6 +34,10 @@ module PageObjects
         find("#{TOPIC_LIST_BODY_SELECTOR} a", text: title).click
       end
 
+      def visit_topic(topic)
+        find("#{topic_list_item_class(topic)} a.raw-topic-link").click
+      end
+
       private
 
       def topic_list_item_class(topic)

spec/system/topic_list_focus_spec.rb

@@ -0,0 +1,40 @@
+# frozen_string_literal: true
+
+describe "Topic list focus", type: :system do
+  let!(:topics) { Fabricate.times(10, :post).map(&:topic) }
+
+  before { Fabricate(:admin) }
+
+  let(:discovery) { PageObjects::Pages::Discovery.new }
+  let(:topic) { PageObjects::Pages::Topic.new }
+
+  def focussed_topic_id
+    page.evaluate_script(
+      "document.activeElement.closest('.topic-list-item')?.dataset.topicId",
+    )&.to_i
+  end
+
+  it "refocusses last clicked topic when going back to topic list" do
+    visit("/")
+    expect(page).to have_css("body.navigation-topics")
+    expect(discovery.topic_list).to have_topics
+
+    # Click a topic
+    discovery.topic_list.visit_topic(topics[5])
+    expect(topic).to have_topic_title(topics[5].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(topics[5].id)
+
+    # Click topic again
+    discovery.topic_list.visit_topic(topics[5])
+    expect(topic).to have_topic_title(topics[5].title)
+
+    # Visiting a topic list another way should not focus
+    find(".sidebar-section-link[data-link-name='everything']").click
+    expect(page).to have_css("body.navigation-topics")
+    expect(focussed_topic_id).to eq(nil)
+  end
+end