From e63072e21fa7001aa9d8ba2162ce7c51967b791c Mon Sep 17 00:00:00 2001 From: Michael Telatynski <7t3chguy@gmail.com> Date: Tue, 9 Aug 2022 15:37:55 +0100 Subject: [PATCH] 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 --- .../pills-click-in-app.spec.ts | 5 +++-- src/components/views/messages/TextualBody.tsx | 16 ++++++++++----- src/utils/tooltipify.tsx | 20 ++++++++----------- test/utils/tooltipify-test.tsx | 15 ++++++++++++++ 4 files changed, 37 insertions(+), 19 deletions(-) diff --git a/cypress/e2e/regression-tests/pills-click-in-app.spec.ts b/cypress/e2e/regression-tests/pills-click-in-app.spec.ts index 33c86cbf3a..cebbc86ed8 100644 --- a/cypress/e2e/regression-tests/pills-click-in-app.spec.ts +++ b/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); }); }); }); diff --git a/src/components/views/messages/TextualBody.tsx b/src/components/views/messages/TextualBody.tsx index 459f384558..23ba901acd 100644 --- a/src/components/views/messages/TextualBody.tsx +++ b/src/components/views/messages/TextualBody.tsx @@ -432,11 +432,17 @@ export default class TextualBody extends React.Component { * 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 `` may contain children, e.g. an anchor wrapping an inline code section + target = target.closest("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; diff --git a/src/utils/tooltipify.tsx b/src/utils/tooltipify.tsx index 3f7042e157..afdcf29609 100644 --- a/src/utils/tooltipify.tsx +++ b/src/utils/tooltipify.tsx @@ -39,9 +39,7 @@ export function tooltipifyLinks(rootNodes: ArrayLike, 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; } @@ -49,20 +47,18 @@ export function tooltipifyLinks(rootNodes: ArrayLike, ignoredNodes: Ele 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 = - { node.innerHTML } + ; - 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, ignoredNodes, containers); } diff --git a/test/utils/tooltipify-test.tsx b/test/utils/tooltipify-test.tsx index 8e3784e7fd..1cad2a0ea2 100644 --- a/test/utils/tooltipify-test.tsx +++ b/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(
click
); + 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(); + }); });