From ef461ffd60b2cb2ee44a602b29a2c01e2ded4bf1 Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Thu, 13 Aug 2020 14:56:13 +1000 Subject: [PATCH] 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 --- .../controllers/user-activity-bookmarks.js | 36 ++++- .../discourse/app/lib/click-track.js | 62 ++++--- .../app/templates/user/bookmarks.hbs | 152 +++++++++--------- test/javascripts/lib/click-track-test.js | 6 +- 4 files changed, 151 insertions(+), 105 deletions(-) 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 9a87fafd3dc..577f148f507 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 c408da105b1..93543b847e3 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 07effe370c9..47537f8a3c3 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}}
{{noResultsHelp}}
{{else}} - {{#conditional-loading-spinner condition=loading}} - {{#load-more selector=".bookmark-list tr" action=(action "loadMore")}} - - - {{#if site.mobileView}} - - - - {{else}} - - - - - - {{/if}} - - - {{#each content as |bookmark|}} - - {{#if site.mobileView}} - - {{/if}} - + + {{raw "list/activity-column" topic=bookmark class="num post-metadata" tagName="td"}} + {{/unless}} + + + {{/each}} + +
 {{i18n "topic.title"}} {{i18n "topic.title"}} {{i18n "post.bookmarks.updated"}}{{i18n "activity"}} 
- {{#if bookmark.post_user_avatar_template}} - - {{avatar bookmark.postUser avatarTemplatePath="avatar_template" usernamePath="username" namePath="name" imageSize="small"}} - - {{/if}} - - -
- {{#if bookmark.name}} - - {{d-icon "info-circle"}}{{bookmark.name}} - +
+ {{#conditional-loading-spinner condition=loading}} + {{#load-more selector=".bookmark-list .bookmark-list-item" action=(action "loadMore")}} + + + {{#if site.mobileView}} + + + + {{else}} + + + + + + {{/if}} + + + {{#each content as |bookmark|}} + + {{#if site.mobileView}} + {{/if}} -
- {{category-link bookmark.category}} - {{discourse-tags bookmark mode="list" tagsForUser=tagsForUser}} -
- - {{#unless site.mobileView}} - - - {{raw "list/activity-column" topic=bookmark class="num post-metadata" tagName="td"}} - {{/unless}} - - - {{/each}} - -
 {{i18n "topic.title"}} {{i18n "topic.title"}} {{i18n "post.bookmarks.updated"}}{{i18n "activity"}} 
+ {{#if bookmark.post_user_avatar_template}} + + {{avatar bookmark.postUser avatarTemplatePath="avatar_template" usernamePath="username" namePath="name" imageSize="small"}} + {{/if}} - {{#if bookmark.reminder_at}} - - {{d-icon "far-clock"}}{{bookmark.formattedReminder}} - - {{/if}} - - - {{topic-status topic=bookmark}} - {{topic-link bookmark}} - - {{#if bookmark.excerpt}} -

{{html-safe bookmark.excerpt}}

+ 		- {{#if bookmark.post_user_avatar_template}} - - {{avatar bookmark.postUser avatarTemplatePath="avatar_template" usernamePath="username" namePath="name" imageSize="small"}} - + + +
+ {{#if bookmark.name}} + + {{d-icon "info-circle"}}{{bookmark.name}} + + {{/if}} + {{#if bookmark.reminder_at}} + + {{d-icon "far-clock"}}{{bookmark.formattedReminder}} + + {{/if}} +
+ + {{topic-status topic=bookmark}} + {{topic-link bookmark}} + + {{#if bookmark.excerpt}} +

{{html-safe bookmark.excerpt}}

{{/if}} +
+ {{category-link bookmark.category}} + {{discourse-tags bookmark mode="list" tagsForUser=tagsForUser}} +
{{format-date bookmark.updated_at format="tiny"}} - {{bookmark-actions-dropdown - bookmark=bookmark - removeBookmark=(action "removeBookmark") - editBookmark=(action "editBookmark") - }} -
- {{conditional-loading-spinner condition=loadingMore}} - {{/load-more}} - {{/conditional-loading-spinner}} + {{#unless site.mobileView}} +
+ {{#if bookmark.post_user_avatar_template}} + + {{avatar bookmark.postUser avatarTemplatePath="avatar_template" usernamePath="username" namePath="name" imageSize="small"}} + + {{/if}} + {{format-date bookmark.updated_at format="tiny"}} + {{bookmark-actions-dropdown + bookmark=bookmark + removeBookmark=(action "removeBookmark") + editBookmark=(action "editBookmark") + }} +
+ {{conditional-loading-spinner condition=loadingMore}} + {{/load-more}} + {{/conditional-loading-spinner}} + {{/if}} diff --git a/test/javascripts/lib/click-track-test.js b/test/javascripts/lib/click-track-test.js index 3364e5b14bc..15693bc3b64 100644 --- a/test/javascripts/lib/click-track-test.js +++ b/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 => {