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

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

diff --git a/app/assets/javascripts/discourse/app/controllers/user-activity-bookmarks.js b/app/assets/javascripts/discourse/app/controllers/user-activity-bookmarks.js
index 9a87faf..577f148 100644
--- a/app/assets/javascripts/discourse/app/controllers/user-activity-bookmarks.js
+++ b/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")
diff --git a/app/assets/javascripts/discourse/app/lib/click-track.js b/app/assets/javascripts/discourse/app/lib/click-track.js
index c408da1..93543b8 100644
--- a/app/assets/javascripts/discourse/app/lib/click-track.js
+++ b/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);
diff --git a/app/assets/javascripts/discourse/app/templates/user/bookmarks.hbs b/app/assets/javascripts/discourse/app/templates/user/bookmarks.hbs
index 07effe3..47537f8 100644
--- a/app/assets/javascripts/discourse/app/templates/user/bookmarks.hbs
+++ b/app/assets/javascripts/discourse/app/templates/user/bookmarks.hbs
@@ -13,83 +13,85 @@
 {{#if noContent}}
   <div class="alert alert-info">{{noResultsHelp}}</div>
 {{else}}
-  {{#conditional-loading-spinner condition=loading}}
-    {{#load-more selector=".bookmark-list tr" action=(action "loadMore")}}
-      <table class="topic-list bookmark-list">
-        <thead>
-          {{#if site.mobileView}}
-            <th>&nbsp;</th>
-            <th>{{i18n "topic.title"}}</th>
-            <th>&nbsp;</th>
-          {{else}}
-            <th>{{i18n "topic.title"}}</th>
-            <th>&nbsp;</th>
-            <th class="post-metadata">{{i18n "post.bookmarks.updated"}}</th>
-            <th class="post-metadata">{{i18n "activity"}}</th>
-            <th>&nbsp;</th>
-          {{/if}}
-        </thead>
-        <tbody>
-          {{#each content as |bookmark|}}
-            <tr class="topic-list-item bookmark-list-item">
-              {{#if site.mobileView}}
-                <td>
-                  {{#if bookmark.post_user_avatar_template}}
-                    <a href={{bookmark.postUser.path}} data-user-card={{bookmark.post_user_username}}>
-                      {{avatar bookmark.postUser avatarTemplatePath="avatar_template" usernamePath="username" namePath="name" imageSize="small"}}
-                    </a>
+  <div class="bookmark-list-wrapper">
+    {{#conditional-loading-spinner condition=loading}}
+      {{#load-more selector=".bookmark-list .bookmark-list-item" action=(action "loadMore")}}
+        <table class="topic-list bookmark-list">
+          <thead>
+            {{#if site.mobileView}}
+              <th>&nbsp;</th>
+              <th>{{i18n "topic.title"}}</th>
+              <th>&nbsp;</th>
+            {{else}}
+              <th>{{i18n "topic.title"}}</th>
+              <th>&nbsp;</th>
+              <th class="post-metadata">{{i18n "post.bookmarks.updated"}}</th>
+              <th class="post-metadata">{{i18n "activity"}}</th>
+              <th>&nbsp;</th>
+            {{/if}}
+          </thead>
+          <tbody>
+            {{#each content as |bookmark|}}
+              <tr class="topic-list-item bookmark-list-item">
+                {{#if site.mobileView}}
+                  <td>
+                    {{#if bookmark.post_user_avatar_template}}
+                      <a href={{bookmark.postUser.path}} data-user-card={{bookmark.post_user_username}}>
+                        {{avatar bookmark.postUser avatarTemplatePath="avatar_template" usernamePath="username" namePath="name" imageSize="small"}}
+                      </a>
+                    {{/if}}
+                  </td>
+                {{/if}}

[... diff too long, it was truncated ...]

GitHub sha: ef461ffd

This commit appears in #10409 which was merged by martin.