Fixes around URL tooltips and in-app matrix.to link handling (#9139)

* Add regression test for tooltipify exposing raw HTML

* Handle m.to links involving children better

* Comments

* Fix mistaken assertion

Author: t3chguy

cypress/e2e/regression-tests/pills-click-in-app.spec.ts

@@ -59,8 +59,9 @@ describe("Pills", () => {
             // find the pill in the timeline and click it
             cy.get(".mx_EventTile_body .mx_Pill").click();
 
+            const localUrl = `/#/room/#${targetLocalpart}:`;
             // verify we landed at a sane place
-            cy.url().should("contain", `/#/room/#${targetLocalpart}:`);
+            cy.url().should("contain", localUrl);
 
             cy.wait(250); // let the room list settle
 
@@ -69,7 +70,7 @@ describe("Pills", () => {
             cy.get(".mx_EventTile_body .mx_Pill .mx_Pill_linkText")
                 .should("have.css", "pointer-events", "none")
                 .click({ force: true }); // force is to ensure we bypass pointer-events
-            cy.url().should("contain", `https://matrix.to/#/#${targetLocalpart}:`);
+            cy.url().should("contain", localUrl);
         });
     });
 });

src/components/views/messages/TextualBody.tsx

@@ -432,11 +432,17 @@ export default class TextualBody extends React.Component<IBodyProps, IState> {
      * to start with (e.g. pills, links in the content).
      */
     private onBodyLinkClick = (e: MouseEvent): void => {
-        const target = e.target as Element;
-        if (target.nodeName !== "A" || target.classList.contains(linkifyOpts.className)) return;
-        const { href } = target as HTMLLinkElement;
-        const localHref = tryTransformPermalinkToLocalHref(href);
-        if (localHref !== href) {
+        let target = e.target as HTMLLinkElement;
+        // links processed by linkifyjs have their own handler so don't handle those here
+        if (target.classList.contains(linkifyOpts.className)) return;
+        if (target.nodeName !== "A") {
+            // Jump to parent as the `<a>` may contain children, e.g. an anchor wrapping an inline code section
+            target = target.closest<HTMLLinkElement>("a");
+        }
+        if (!target) return;
+
+        const localHref = tryTransformPermalinkToLocalHref(target.href);
+        if (localHref !== target.href) {
             // it could be converted to a localHref -> therefore handle locally
             e.preventDefault();
             window.location.hash = localHref;

src/utils/tooltipify.tsx

@@ -39,30 +39,26 @@ export function tooltipifyLinks(rootNodes: ArrayLike<Element>, ignoredNodes: Ele
     let node = rootNodes[0];
 
     while (node) {
-        let tooltipified = false;
-
-        if (ignoredNodes.indexOf(node) >= 0) {
+        if (ignoredNodes.includes(node) || containers.includes(node)) {
             node = node.nextSibling as Element;
             continue;
         }
 
         if (node.tagName === "A" && node.getAttribute("href")
             && node.getAttribute("href") !== node.textContent.trim()
         ) {
-            const container = document.createElement("span");
             const href = node.getAttribute("href");
 
+            // The node's innerHTML was already sanitized before being rendered in the first place, here we are just
+            // wrapping the link with the LinkWithTooltip component, keeping the same children. Ideally we'd do this
+            // without the superfluous span but this is not something React trivially supports at this time.
             const tooltip = <LinkWithTooltip tooltip={new URL(href, window.location.href).toString()}>
-                { node.innerHTML }
+                <span dangerouslySetInnerHTML={{ __html: node.innerHTML }} />
             </LinkWithTooltip>;
 
-            ReactDOM.render(tooltip, container);
-            node.replaceChildren(container);
-            containers.push(container);
-            tooltipified = true;
-        }
-
-        if (node.childNodes?.length && !tooltipified) {
+            ReactDOM.render(tooltip, node);
+            containers.push(node);
+        } else if (node.childNodes?.length) {
             tooltipifyLinks(node.childNodes as NodeListOf<Element>, ignoredNodes, containers);
         }
 

test/utils/tooltipify-test.tsx

@@ -57,4 +57,19 @@ describe('tooltipify', () => {
         expect(containers).toHaveLength(0);
         expect(root.outerHTML).toEqual(originalHtml);
     });
+
+    it("does not re-wrap if called multiple times", () => {
+        const component = mount(<div><a href="/foo">click</a></div>);
+        const root = component.getDOMNode();
+        const containers: Element[] = [];
+        tooltipifyLinks([root], [], containers);
+        tooltipifyLinks([root], [], containers);
+        tooltipifyLinks([root], [], containers);
+        tooltipifyLinks([root], [], containers);
+        expect(containers).toHaveLength(1);
+        const anchor = root.querySelector("a");
+        expect(anchor?.getAttribute("href")).toEqual("/foo");
+        const tooltip = anchor.querySelector(".mx_TextWithTooltip_target");
+        expect(tooltip).toBeDefined();
+    });
 });