From 71f808dea9c66cb7f6e6fe7905642a825bd2fa2a Mon Sep 17 00:00:00 2001
From: Keegan George <kgeorge13@gmail.com>
Date: Fri, 1 Nov 2024 04:44:08 +0900
Subject: [PATCH] DEV: Improve method of presenting link clicks (#29453)

Currently the tracking for clicked links are injected into the HTML in a span tag. This leads to the link counter value being highlighted when copying and pasting. Additionally, any means for using CSS to hide link counters result in a gap due to it occupying a specific width.

With this change, we make link counters appear in a data attribute on the link element and visually shown with CSS `::after` element.
---
 .../components/topic-map/topic-map-link.gjs   |  3 +++
 .../topic-map/topic-map-summary.gjs           |  8 +------
 .../discourse/app/templates/user/summary.hbs  | 10 ++------
 .../discourse/app/widgets/post-cooked.js      | 19 +++++++--------
 .../discourse/app/widgets/post-links.js       | 10 ++++----
 .../tests/acceptance/composer-test.js         |  2 +-
 .../discourse/tests/acceptance/topic-test.js  |  6 ++++-
 .../components/widgets/post-test.js           | 23 +++++++++++++++----
 app/assets/stylesheets/common/base/topic.scss |  8 +++----
 .../stylesheets/common/components/badges.scss | 11 ---------
 .../common/components/topic-map.scss          | 15 ++++++++----
 .../stylesheets/common/foundation/mixins.scss | 18 +++++++++++++++
 .../stylesheets/common/printer-friendly.scss  |  2 +-
 app/assets/stylesheets/wcag.scss              |  3 ++-
 config/locales/client.en.yml                  |  3 +++
 15 files changed, 82 insertions(+), 59 deletions(-)

diff --git a/app/assets/javascripts/discourse/app/components/topic-map/topic-map-link.gjs b/app/assets/javascripts/discourse/app/components/topic-map/topic-map-link.gjs
index 347f6665d2e..d8f64ec4672 100644
--- a/app/assets/javascripts/discourse/app/components/topic-map/topic-map-link.gjs
+++ b/app/assets/javascripts/discourse/app/components/topic-map/topic-map-link.gjs
@@ -1,5 +1,6 @@
 import Component from "@glimmer/component";
 import replaceEmoji from "discourse/helpers/replace-emoji";
+import i18n from "discourse-common/helpers/i18n";
 import and from "truth-helpers/helpers/and";
 
 const TRUNCATE_LENGTH_LIMIT = 85;
@@ -27,6 +28,8 @@ export default class TopicMapLink extends Component {
       data-ignore-post-id="true"
       target="_blank"
       rel="nofollow ugc noopener noreferrer"
+      data-clicks={{@clickCount}}
+      aria-label={{i18n "topic_map.clicks" count=@clickCount}}
     >
       {{#if @title}}
         {{replaceEmoji this.truncatedContent}}
diff --git a/app/assets/javascripts/discourse/app/components/topic-map/topic-map-summary.gjs b/app/assets/javascripts/discourse/app/components/topic-map/topic-map-summary.gjs
index 3d379249ffe..14a39278c10 100644
--- a/app/assets/javascripts/discourse/app/components/topic-map/topic-map-summary.gjs
+++ b/app/assets/javascripts/discourse/app/components/topic-map/topic-map-summary.gjs
@@ -344,19 +344,13 @@ export default class TopicMapSummary extends Component {
             <ul class="topic-links">
               {{#each this.linksToShow as |link|}}
                 <li>
-                  <span
-                    class="badge badge-notification clicks"
-                    title={{i18n "topic_map.clicks" count=link.clicks}}
-                  >
-                    {{link.clicks}}
-                  </span>
-
                   <TopicMapLink
                     @attachment={{link.attachment}}
                     @title={{link.title}}
                     @rootDomain={{link.root_domain}}
                     @url={{link.url}}
                     @userId={{link.user_id}}
+                    @clickCount={{link.clicks}}
                   />
                 </li>
               {{/each}}
diff --git a/app/assets/javascripts/discourse/app/templates/user/summary.hbs b/app/assets/javascripts/discourse/app/templates/user/summary.hbs
index 5a00fbd1eeb..58acce735e4 100644
--- a/app/assets/javascripts/discourse/app/templates/user/summary.hbs
+++ b/app/assets/javascripts/discourse/app/templates/user/summary.hbs
@@ -194,18 +194,12 @@
                   'nofollow ugc'
                 }}"
                 target="_blank"
+                data-clicks={{link.clicks}}
+                aria-label={{i18n "topic_map.clicks" count=link.clicks}}
               >
                 {{shorten-url link.url}}
               </a>
               {{! template-lint-enable link-rel-noopener }}
-
-              <span
-                class="badge badge-notification clicks"
-                title={{i18n "topic_map.clicks" count=link.clicks}}
-              >
-                {{number link.clicks}}
-              </span>
-
               <br />
 
               <a href={{link.post_url}}>
diff --git a/app/assets/javascripts/discourse/app/widgets/post-cooked.js b/app/assets/javascripts/discourse/app/widgets/post-cooked.js
index d1e968d212d..fde295c8347 100644
--- a/app/assets/javascripts/discourse/app/widgets/post-cooked.js
+++ b/app/assets/javascripts/discourse/app/widgets/post-cooked.js
@@ -9,7 +9,6 @@ import {
   destroyUserStatusOnMentions,
   updateUserStatusOnMention,
 } from "discourse/lib/update-user-status-on-mention";
-import domFromString from "discourse-common/lib/dom-from-string";
 import escape from "discourse-common/lib/escape";
 import { getOwnerWithFallback } from "discourse-common/lib/get-owner";
 import getURL from "discourse-common/lib/get-url";
@@ -162,16 +161,14 @@ export default class PostCooked {
             !bestElements.has(onebox) ||
             bestElements.get(onebox) === link
           ) {
-            const title = I18n.t("topic_map.clicks", { count: lc.clicks });
-
-            link.appendChild(document.createTextNode(" "));
-            link.appendChild(
-              domFromString(
-                `<span class='badge badge-notification clicks' title='${title}'>${number(
-                  lc.clicks
-                )}</span>`
-              )[0]
-            );
+            link.setAttribute("data-clicks", number(lc.clicks));
+            const ariaLabel = `${link.textContent.trim()} ${I18n.t(
+              "post.link_clicked",
+              {
+                count: lc.clicks,
+              }
+            )}`;
+            link.setAttribute("aria-label", ariaLabel);
           }
         }
       });
diff --git a/app/assets/javascripts/discourse/app/widgets/post-links.js b/app/assets/javascripts/discourse/app/widgets/post-links.js
index f1ae950a20b..e9a3c474cc1 100644
--- a/app/assets/javascripts/discourse/app/widgets/post-links.js
+++ b/app/assets/javascripts/discourse/app/widgets/post-links.js
@@ -13,10 +13,12 @@ export default createWidget("post-links", {
 
   linkHtml(link) {
     const linkBody = replaceEmoji(link.title);
+    const attributes = {
+      href: link.url,
+    };
+
     if (link.clicks) {
-      linkBody.push(
-        h("span.badge.badge-notification.clicks", link.clicks.toString())
-      );
+      attributes["data-clicks"] = link.clicks.toString();
     }
 
     return h(
@@ -25,7 +27,7 @@ export default createWidget("post-links", {
         "a.track-link",
         {
           className: "inbound",
-          attributes: { href: link.url },
+          attributes,
         },
         [iconNode("link"), linkBody]
       )
diff --git a/app/assets/javascripts/discourse/tests/acceptance/composer-test.js b/app/assets/javascripts/discourse/tests/acceptance/composer-test.js
index d1eddfa2e30..9deec7c969b 100644
--- a/app/assets/javascripts/discourse/tests/acceptance/composer-test.js
+++ b/app/assets/javascripts/discourse/tests/acceptance/composer-test.js
@@ -408,7 +408,7 @@ acceptance("Composer", function (needs) {
     assert
       .dom(".topic-post:last-of-type .cooked p")
       .hasText(
-        "If you use gettext format you could leverage Launchpad 13 translations and the community behind it."
+        "If you use gettext format you could leverage Launchpad translations and the community behind it."
       );
   });
 
diff --git a/app/assets/javascripts/discourse/tests/acceptance/topic-test.js b/app/assets/javascripts/discourse/tests/acceptance/topic-test.js
index 396f76163b2..2170d271344 100644
--- a/app/assets/javascripts/discourse/tests/acceptance/topic-test.js
+++ b/app/assets/javascripts/discourse/tests/acceptance/topic-test.js
@@ -271,6 +271,11 @@ acceptance("Topic featured links", function (needs) {
     display_name_on_posts: false,
     prioritize_username_in_ux: true,
   });
+  needs.pretender((server, helper) => {
+    server.get("/inline-onebox", () => {
+      return helper.response({ "inline-oneboxes": [] });
+    });
+  });
 
   test("remove nofollow attribute", async function (assert) {
     await visit("/t/-/299/1");
@@ -433,7 +438,6 @@ acceptance("Topic featured links", function (needs) {
     await visit("/t/internationalization-localization/280");
     await selectText("#post_5 .cooked");
     await click(".quote-button .insert-quote");
-
     assert.ok(
       query(".d-editor-input").value.includes(
         'quote="pekka, post:5, topic:280, full:true"'
diff --git a/app/assets/javascripts/discourse/tests/integration/components/widgets/post-test.js b/app/assets/javascripts/discourse/tests/integration/components/widgets/post-test.js
index b630a607a6f..0a5f028a596 100644
--- a/app/assets/javascripts/discourse/tests/integration/components/widgets/post-test.js
+++ b/app/assets/javascripts/discourse/tests/integration/components/widgets/post-test.js
@@ -44,8 +44,14 @@ module("Integration | Component | Widget | post", function (hooks) {
       hbs`<MountWidget @widget="post-contents" @args={{this.args}} />`
     );
 
-    assert.strictEqual(queryAll(".badge.clicks")[0].innerText, "1");
-    assert.strictEqual(queryAll(".badge.clicks")[1].innerText, "2");
+    assert.strictEqual(
+      queryAll("a[data-clicks='1']")[0].getAttribute("data-clicks"),
+      "1"
+    );
+    assert.strictEqual(
+      queryAll("a[data-clicks='2']")[0].getAttribute("data-clicks"),
+      "2"
+    );
   });
 
   test("post - onebox links", async function (assert) {
@@ -72,9 +78,16 @@ module("Integration | Component | Widget | post", function (hooks) {
       hbs`<MountWidget @widget="post-contents" @args={{this.args}} />`
     );
 
-    assert.strictEqual(count(".badge.clicks"), 2);
-    assert.strictEqual(queryAll(".badge.clicks")[0].innerText, "1");
-    assert.strictEqual(queryAll(".badge.clicks")[1].innerText, "2");
+    assert.strictEqual(
+      queryAll("a[data-clicks='1']")[0].getAttribute("data-clicks"),
+      "1",
+      "First link has correct data attribute and content"
+    );
+    assert.strictEqual(
+      queryAll("a[data-clicks='2']")[0].getAttribute("data-clicks"),
+      "2",
+      "Second link has correct data attribute and content"
+    );
   });
 
   test("wiki", async function (assert) {
diff --git a/app/assets/stylesheets/common/base/topic.scss b/app/assets/stylesheets/common/base/topic.scss
index 79d6d53c92f..c732a74036e 100644
--- a/app/assets/stylesheets/common/base/topic.scss
+++ b/app/assets/stylesheets/common/base/topic.scss
@@ -388,10 +388,6 @@
       a[href] {
         color: var(--primary-med-or-secondary-med);
       }
-      .clicks {
-        margin-left: 0.5em;
-        flex: 0 0 auto;
-      }
       .d-icon {
         font-size: var(--font-down-2);
         margin: 0 0.5em 0 0;
@@ -458,3 +454,7 @@ a.topic-featured-link {
     text-transform: lowercase;
   }
 }
+
+a[data-clicks]::after {
+  @include click-counter-badge;
+}
diff --git a/app/assets/stylesheets/common/components/badges.scss b/app/assets/stylesheets/common/components/badges.scss
index b099a897c06..d7d2a4a6956 100644
--- a/app/assets/stylesheets/common/components/badges.scss
+++ b/app/assets/stylesheets/common/components/badges.scss
@@ -112,17 +112,6 @@
     width: 8px;
     border-radius: 50%;
   }
-
-  // Click count
-
-  &.clicks {
-    font-weight: normal;
-    background-color: var(--primary-low);
-    top: -1px;
-    color: var(--primary-medium);
-    position: relative;
-    border: none;
-  }
 }
 
 // Posts badge
diff --git a/app/assets/stylesheets/common/components/topic-map.scss b/app/assets/stylesheets/common/components/topic-map.scss
index eb91da999f6..46ce1f90b0f 100644
--- a/app/assets/stylesheets/common/components/topic-map.scss
+++ b/app/assets/stylesheets/common/components/topic-map.scss
@@ -393,14 +393,19 @@ body:not(.archetype-private_message) {
         }
       }
     }
-    .badge {
-      grid-area: counter;
-      align-self: start;
-      top: 0.2em;
-    }
+
     .topic-link {
       grid-area: link;
     }
+
+    .topic-link[data-clicks]::before {
+      @include click-counter-badge;
+    }
+
+    .topic-link[data-clicks]::after {
+      display: none;
+    }
+
     .domain {
       grid-area: domain;
       font-size: var(--font-down-2);
diff --git a/app/assets/stylesheets/common/foundation/mixins.scss b/app/assets/stylesheets/common/foundation/mixins.scss
index 9651fc0de6c..2a4a5744bac 100644
--- a/app/assets/stylesheets/common/foundation/mixins.scss
+++ b/app/assets/stylesheets/common/foundation/mixins.scss
@@ -292,3 +292,21 @@ $hpad: 0.65em;
     background: var(--d-nav-color--active);
   }
 }
+
+@mixin click-counter-badge {
+  content: attr(data-clicks);
+  font-weight: normal;
+  background-color: var(--primary-low);
+  color: var(--primary-medium);
+  position: relative;
+  top: -1px;
+  padding: 0.21em 0.42em;
+  min-width: 0.5em;
+  line-height: var(--line-height-small);
+  font-size: var(--font-down-2);
+  text-align: center;
+  border-radius: 10px;
+  white-space: nowrap;
+  display: inline-block;
+  margin: 0.15em;
+}
diff --git a/app/assets/stylesheets/common/printer-friendly.scss b/app/assets/stylesheets/common/printer-friendly.scss
index 86e95b259ce..e00be1854e8 100644
--- a/app/assets/stylesheets/common/printer-friendly.scss
+++ b/app/assets/stylesheets/common/printer-friendly.scss
@@ -32,7 +32,7 @@
   #footer,
   .alert-info,
   .badge-category,
-  .badge-notification.clicks,
+  a[data-clicks]::after,
   .crawler-nav,
   .powered-by-link,
   .timeline-container,
diff --git a/app/assets/stylesheets/wcag.scss b/app/assets/stylesheets/wcag.scss
index 94913518736..dfffb13c0ff 100644
--- a/app/assets/stylesheets/wcag.scss
+++ b/app/assets/stylesheets/wcag.scss
@@ -195,7 +195,8 @@ html {
     color: var(--primary-medium);
   }
 
-  .badge-notification.clicks {
+  a[data-clicks]::before,
+  a[data-clicks]::after {
     color: var(--primary-high);
   }
 
diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml
index cbfd66a7213..c3db626bbfb 100644
--- a/config/locales/client.en.yml
+++ b/config/locales/client.en.yml
@@ -3766,6 +3766,9 @@ en:
         one: "view %{count} hidden reply"
         other: "view %{count} hidden replies"
       sr_reply_to: "Reply to post #%{post_number} by @%{username}"
+      link_clicked:
+        one: "link clicked %{count} time"
+        other: "link clicked %{count} times"
 
       notice:
         new_user: "This is the first time %{user} has posted — let’s welcome them to our community!"