FIX: Make sure user preference to open external links in new tab works for bookmark list excerpts (#10409)

Meta post: https://meta.discourse.org/t/bookmark-page-does-not-respect-open-all-external-links-in-new-tab-user-preference/160118

app/assets/javascripts/discourse/app/controllers/user-activity-bookmarks.js

@@ -1,4 +1,5 @@
 import I18n from "I18n";
+import { schedule } from "@ember/runloop";
 import Controller from "@ember/controller";
 import showModal from "discourse/lib/show-modal";
 import { Promise } from "rsvp";
@@ -6,6 +7,10 @@ import { inject } from "@ember/controller";
 import { action } from "@ember/object";
 import discourseComputed from "discourse-common/utils/decorators";
 import Bookmark from "discourse/models/bookmark";
+import {
+  shouldOpenInNewTab,
+  openLinkInNewTab
+} from "discourse/lib/click-track";
 
 export default Controller.extend({
   application: inject(),
@@ -19,6 +24,11 @@ export default Controller.extend({
 
   queryParams: ["q"],
 
+  init() {
+    this._super(...arguments);
+    this._boundClick = false;
+  },
+
   loadItems() {
     this.setProperties({
       content: [],
@@ -30,16 +40,36 @@ export default Controller.extend({
       this.set("searchTerm", this.q);
     }
 
+    if (!this._boundClick) {
+      schedule("afterRender", () => {
+        // TODO(martin): This should be pulled out into a bookmark-list component,
+        // the controller is not the best place for this.
+        let wrapper = document.querySelector(".bookmark-list-wrapper");
+        if (!wrapper) {
+          return;
+        }
+        wrapper.addEventListener("click", function(e) {
+          if (e.target && e.target.tagName === "A") {
+            let link = e.target;
+            if (shouldOpenInNewTab(link.href)) {
+              openLinkInNewTab(link);
+            }
+          }
+        });
+        this._boundClick = true;
+      });
+    }
+
     return this.model
       .loadItems({ q: this.searchTerm })
       .then(response => this._processLoadResponse(response))
       .catch(() => this._bookmarksListDenied())
-      .finally(() =>
+      .finally(() => {
         this.setProperties({
           loaded: true,
           loading: false
-        })
-      );
+        });
+      });
   },
 
   @discourseComputed("loaded", "content.length", "noResultsHelp")

app/assets/javascripts/discourse/app/lib/click-track.js

@@ -34,6 +34,41 @@ export function isValidLink($link) {
   );
 }
 
+export function shouldOpenInNewTab(href) {
+  const isInternal = DiscourseURL.isInternal(href);
+  const openExternalInNewTab = User.currentProp("external_links_in_new_tab");
+  return !isInternal && openExternalInNewTab;
+}
+
+export function openLinkInNewTab(link) {
+  let href = (link.href || link.dataset.href || "").trim();
+  if (href === "") {
+    return;
+  }
+
+  const newWindow = window.open(href, "_blank");
+  newWindow.opener = null;
+  newWindow.focus();
+
+  // Hack to prevent changing current window.location.
+  // e.preventDefault() does not work.
+  if (!link.dataset.href) {
+    link.classList.add("no-href");
+    link.dataset.href = link.href;
+    link.dataset.autoRoute = true;
+    link.removeAttribute("href");
+
+    later(() => {
+      if (link) {
+        link.classList.remove("no-href");
+        link.setAttribute("href", link.dataset.href);
+        delete link.dataset.href;
+        delete link.dataset.autoRoute;
+      }
+    }, 50);
+  }
+}
+
 export default {
   trackClick(e, siteSettings) {
     // right clicks are not tracked
@@ -121,33 +156,12 @@ export default {
       }
     }
 
-    const isInternal = DiscourseURL.isInternal(href);
-    const openExternalInNewTab = User.currentProp("external_links_in_new_tab");
-
     if (!wantsNewWindow(e)) {
-      if (!isInternal && openExternalInNewTab) {
-        const newWindow = window.open(href, "_blank");
-        newWindow.opener = null;
-        newWindow.focus();
-
-        // Hack to prevent changing current window.location.
-        // e.preventDefault() does not work.
-        if (!$link.data("href")) {
-          $link.addClass("no-href");
-          $link.data("href", $link.attr("href"));
-          $link.attr("href", null);
-          $link.data("auto-route", true);
-
-          later(() => {
-            $link.removeClass("no-href");
-            $link.attr("href", $link.data("href"));
-            $link.data("href", null);
-            $link.data("auto-route", null);
-          }, 50);
-        }
+      if (shouldOpenInNewTab(href)) {
+        openLinkInNewTab($link[0]);
       } else {
         trackPromise.finally(() => {
-          if (isInternal) {
+          if (DiscourseURL.isInternal(href)) {
             DiscourseURL.routeTo(href);
           } else {
             DiscourseURL.redirectTo(href);

app/assets/javascripts/discourse/app/templates/user/bookmarks.hbs

@@ -13,8 +13,9 @@
 {{#if noContent}}
   <div class="alert alert-info">{{noResultsHelp}}</div>
 {{else}}
+  <div class="bookmark-list-wrapper">
     {{#conditional-loading-spinner condition=loading}}
-    {{#load-more selector=".bookmark-list tr" action=(action "loadMore")}}
+      {{#load-more selector=".bookmark-list .bookmark-list-item" action=(action "loadMore")}}
         <table class="topic-list bookmark-list">
           <thead>
             {{#if site.mobileView}}
@@ -92,4 +93,5 @@
         {{conditional-loading-spinner condition=loadingMore}}
       {{/load-more}}
     {{/conditional-loading-spinner}}
+  </div>
 {{/if}}

test/javascripts/lib/click-track-test.js

@@ -140,7 +140,7 @@ QUnit.test("does not track right clicks inside quotes", async assert => {
 QUnit.test("does not track clicks links in quotes", async assert => {
   User.currentProp("external_links_in_new_tab", true);
   assert.notOk(track(generateClickEventOn(".quote a:last-child")));
-  assert.ok(window.open.calledWith("https://google.com", "_blank"));
+  assert.ok(window.open.calledWith("https://google.com/", "_blank"));
 });
 
 QUnit.test("does not track clicks on category badges", async assert => {
@@ -158,10 +158,10 @@ QUnit.test("removes the href and put it as a data attribute", async assert => {
 
   var $link = fixture("a").first();
   assert.ok($link.hasClass("no-href"));
-  assert.equal($link.data("href"), "http://www.google.com");
+  assert.equal($link.data("href"), "http://www.google.com/");
   assert.blank($link.attr("href"));
   assert.ok($link.data("auto-route"));
-  assert.ok(window.open.calledWith("http://www.google.com", "_blank"));
+  assert.ok(window.open.calledWith("http://www.google.com/", "_blank"));
 });
 
 QUnit.test("restores the href after a while", async assert => {