Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Commit

Permalink
Switch to linkify-react for element Linkification as it handles React…
Browse files Browse the repository at this point in the history
… subtrees without exploding (#10060

* Switch to linkify-react instead of our faulty implementation

Fixes a series of soft crashes where errors include "The node to be removed is not a child of this node."

* Improve types

* Fix types

* Update snapshots

* Add test

* Fix test
  • Loading branch information
t3chguy authored Feb 3, 2023
1 parent 0895570 commit 2bde31d
Show file tree
Hide file tree
Showing 15 changed files with 101 additions and 193 deletions.
29 changes: 28 additions & 1 deletion cypress/e2e/spaces/spaces.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ limitations under the License.
/// <reference types="cypress" />

import type { MatrixClient } from "matrix-js-sdk/src/client";
import type { Preset } from "matrix-js-sdk/src/@types/partials";
import type { ICreateRoomOpts } from "matrix-js-sdk/src/@types/requests";
import { HomeserverInstance } from "../../plugins/utils/homeserver";
import Chainable = Cypress.Chainable;
Expand All @@ -32,7 +33,7 @@ function openSpaceContextMenu(spaceName: string): Chainable<JQuery> {
return cy.get(".mx_SpacePanel_contextMenu");
}

function spaceCreateOptions(spaceName: string): ICreateRoomOpts {
function spaceCreateOptions(spaceName: string, roomIds: string[] = []): ICreateRoomOpts {
return {
creation_content: {
type: "m.space",
Expand All @@ -44,6 +45,7 @@ function spaceCreateOptions(spaceName: string): ICreateRoomOpts {
name: spaceName,
},
},
...roomIds.map(spaceChildInitialState),
],
};
}
Expand Down Expand Up @@ -283,4 +285,29 @@ describe("Spaces", () => {
cy.checkA11y(undefined, axeOptions);
cy.get(".mx_SpacePanel").percySnapshotElement("Space panel expanded", { widths: [258] });
});

it("should not soft crash when joining a room from space hierarchy which has a link in its topic", () => {
cy.getBot(homeserver, { displayName: "BotBob" }).then({ timeout: 10000 }, async (bot) => {
const { room_id: roomId } = await bot.createRoom({
preset: "public_chat" as Preset,
name: "Test Room",
topic: "This is a topic https://github.com/matrix-org/matrix-react-sdk/pull/10060 with a link",
});
const { room_id: spaceId } = await bot.createRoom(spaceCreateOptions("Test Space", [roomId]));
await bot.invite(spaceId, user.userId);
});

cy.getSpacePanelButton("Test Space").should("exist");
cy.wait(500); // without this we can end up clicking too quickly and it ends up having no effect
cy.viewSpaceByName("Test Space");
cy.contains(".mx_AccessibleButton", "Accept").click();

cy.contains(".mx_SpaceHierarchy_roomTile.mx_AccessibleButton", "Test Room").within(() => {
cy.contains("Join").should("exist").realHover().click();
cy.contains("View", { timeout: 5000 }).should("exist").click();
});

// Assert we get shown the new room intro, and thus not the soft crash screen
cy.get(".mx_NewRoomIntro").should("exist");
});
});
1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,7 @@
"jszip": "^3.7.0",
"katex": "^0.16.0",
"linkify-element": "4.0.0-beta.4",
"linkify-react": "4.0.0-beta.4",
"linkify-string": "4.0.0-beta.4",
"linkifyjs": "4.0.0-beta.4",
"lodash": "^4.17.20",
Expand Down
14 changes: 12 additions & 2 deletions src/HtmlUtils.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -17,16 +17,17 @@ See the License for the specific language governing permissions and
limitations under the License.
*/

import React, { ReactNode } from "react";
import React, { ReactElement, ReactNode } from "react";
import sanitizeHtml from "sanitize-html";
import cheerio from "cheerio";
import classNames from "classnames";
import EMOJIBASE_REGEX from "emojibase-regex";
import { split } from "lodash";
import { merge, split } from "lodash";
import katex from "katex";
import { decode } from "html-entities";
import { IContent } from "matrix-js-sdk/src/models/event";
import { Optional } from "matrix-events-sdk";
import _Linkify from "linkify-react";

import {
_linkifyElement,
Expand Down Expand Up @@ -682,6 +683,15 @@ export function topicToHtml(
);
}

/* Wrapper around linkify-react merging in our default linkify options */
export function Linkify({ as, options, children }: React.ComponentProps<typeof _Linkify>): ReactElement {
return (
<_Linkify as={as} options={merge({}, linkifyMatrixOptions, options)}>
{children}
</_Linkify>
);
}

/**
* Linkifies the given string. This is a wrapper around 'linkifyjs/string'.
*
Expand Down
9 changes: 3 additions & 6 deletions src/SlashCommands.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ import dis from "./dispatcher/dispatcher";
import { _t, _td, ITranslatableError, newTranslatableError } from "./languageHandler";
import Modal from "./Modal";
import MultiInviter from "./utils/MultiInviter";
import { linkifyElement, topicToHtml } from "./HtmlUtils";
import { Linkify, topicToHtml } from "./HtmlUtils";
import QuestionDialog from "./components/views/dialogs/QuestionDialog";
import WidgetUtils from "./utils/WidgetUtils";
import { textToHtmlRainbow } from "./utils/colour";
Expand Down Expand Up @@ -501,14 +501,11 @@ export const Commands = [
? ContentHelpers.parseTopicContent(content)
: { text: _t("This room has no topic.") };

const ref = (e): void => {
if (e) linkifyElement(e);
};
const body = topicToHtml(topic.text, topic.html, ref, true);
const body = topicToHtml(topic.text, topic.html, undefined, true);

Modal.createDialog(InfoDialog, {
title: room.name,
description: <div ref={ref}>{body}</div>,
description: <Linkify>{body}</Linkify>,
hasCloseButton: true,
className: "markdown-body",
});
Expand Down
2 changes: 1 addition & 1 deletion src/components/structures/MatrixChat.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -136,9 +136,9 @@ import { SdkContextClass, SDKContext } from "../../contexts/SDKContext";
import { viewUserDeviceSettings } from "../../actions/handlers/viewUserDeviceSettings";
import { cleanUpBroadcasts, VoiceBroadcastResumer } from "../../voice-broadcast";
import GenericToast from "../views/toasts/GenericToast";
import { Linkify } from "../views/elements/Linkify";
import RovingSpotlightDialog, { Filter } from "../views/dialogs/spotlight/SpotlightDialog";
import { findDMForUser } from "../../utils/dm/findDMForUser";
import { Linkify } from "../../HtmlUtils";

// legacy export
export { default as Views } from "../../Views";
Expand Down
35 changes: 22 additions & 13 deletions src/components/structures/SpaceHierarchy.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ import TextWithTooltip from "../views/elements/TextWithTooltip";
import { useStateToggle } from "../../hooks/useStateToggle";
import { getChildOrder } from "../../stores/spaces/SpaceStore";
import AccessibleTooltipButton from "../views/elements/AccessibleTooltipButton";
import { linkifyElement, topicToHtml } from "../../HtmlUtils";
import { Linkify, topicToHtml } from "../../HtmlUtils";
import { useDispatcher } from "../../hooks/useDispatcher";
import { Action } from "../../dispatcher/actions";
import { IState, RovingTabIndexProvider, useRovingTabIndex } from "../../accessibility/RovingTabIndex";
Expand Down Expand Up @@ -210,6 +210,25 @@ const Tile: React.FC<ITileProps> = ({
topic = room.topic;
}

let topicSection: ReactNode | undefined;
if (topic) {
topicSection = (
<Linkify
options={{
attributes: {
onClick(ev: MouseEvent) {
// prevent clicks on links from bubbling up to the room tile
ev.stopPropagation();
},
},
}}
>
{" · "}
{topic}
</Linkify>
);
}

let joinedSection: ReactElement | undefined;
if (joinedRoom) {
joinedSection = <div className="mx_SpaceHierarchy_roomTile_joined">{_t("Joined")}</div>;
Expand All @@ -231,19 +250,9 @@ const Tile: React.FC<ITileProps> = ({
{joinedSection}
{suggestedSection}
</div>
<div
className="mx_SpaceHierarchy_roomTile_info"
ref={(e) => e && linkifyElement(e)}
onClick={(ev) => {
// prevent clicks on links from bubbling up to the room tile
if ((ev.target as HTMLElement).tagName === "A") {
ev.stopPropagation();
}
}}
>
<div className="mx_SpaceHierarchy_roomTile_info">
{description}
{topic && " · "}
{topic}
{topicSection}
</div>
</div>
<div className="mx_SpaceHierarchy_actions">
Expand Down
39 changes: 0 additions & 39 deletions src/components/views/elements/Linkify.tsx

This file was deleted.

15 changes: 8 additions & 7 deletions src/components/views/elements/RoomTopic.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -29,9 +29,8 @@ import InfoDialog from "../dialogs/InfoDialog";
import { useDispatcher } from "../../../hooks/useDispatcher";
import MatrixClientContext from "../../../contexts/MatrixClientContext";
import AccessibleButton from "./AccessibleButton";
import { Linkify } from "./Linkify";
import TooltipTarget from "./TooltipTarget";
import { topicToHtml } from "../../../HtmlUtils";
import { Linkify, topicToHtml } from "../../../HtmlUtils";

interface IProps extends React.HTMLProps<HTMLDivElement> {
room?: Room;
Expand Down Expand Up @@ -71,12 +70,14 @@ export default function RoomTopic({ room, ...props }: IProps): JSX.Element {
description: (
<div>
<Linkify
as="p"
onClick={(ev: MouseEvent) => {
if ((ev.target as HTMLElement).tagName.toUpperCase() === "A") {
modal.close();
}
options={{
attributes: {
onClick() {
modal.close();
},
},
}}
as="p"
>
{body}
</Linkify>
Expand Down
2 changes: 1 addition & 1 deletion src/components/views/messages/TextualBody.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -436,7 +436,7 @@ export default class TextualBody extends React.Component<IBodyProps, IState> {
private onBodyLinkClick = (e: MouseEvent): void => {
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.classList.contains(linkifyOpts.className as string)) 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");
Expand Down
19 changes: 3 additions & 16 deletions src/components/views/rooms/LinkPreviewWidget.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ import { decode } from "html-entities";
import { MatrixEvent } from "matrix-js-sdk/src/models/event";
import { IPreviewUrlResponse } from "matrix-js-sdk/src/client";

import { linkifyElement } from "../../../HtmlUtils";
import { Linkify } from "../../../HtmlUtils";
import SettingsStore from "../../../settings/SettingsStore";
import Modal from "../../../Modal";
import * as ImageUtils from "../../../ImageUtils";
Expand All @@ -35,21 +35,8 @@ interface IProps {
}

export default class LinkPreviewWidget extends React.Component<IProps> {
private readonly description = createRef<HTMLDivElement>();
private image = createRef<HTMLImageElement>();

public componentDidMount(): void {
if (this.description.current) {
linkifyElement(this.description.current);
}
}

public componentDidUpdate(): void {
if (this.description.current) {
linkifyElement(this.description.current);
}
}

private onImageClick = (ev): void => {
const p = this.props.preview;
if (ev.button != 0 || ev.metaKey) return;
Expand Down Expand Up @@ -155,8 +142,8 @@ export default class LinkPreviewWidget extends React.Component<IProps> {
<span className="mx_LinkPreviewWidget_siteName">{" - " + p["og:site_name"]}</span>
)}
</div>
<div className="mx_LinkPreviewWidget_description" ref={this.description}>
{description}
<div className="mx_LinkPreviewWidget_description">
<Linkify>{description}</Linkify>
</div>
</div>
</div>
Expand Down
2 changes: 1 addition & 1 deletion src/components/views/rooms/RoomInfoLine.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ const RoomInfoLine: FC<IProps> = ({ room }) => {
const summary = useAsyncMemo(async (): Promise<Awaited<ReturnType<MatrixClient["getRoomSummary"]>> | null> => {
if (room.getMyMembership() !== "invite") return null;
try {
return room.client.getRoomSummary(room.roomId);
return await room.client.getRoomSummary(room.roomId);
} catch (e) {
return null;
}
Expand Down
8 changes: 4 additions & 4 deletions src/linkify-matrix.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ limitations under the License.
*/

import * as linkifyjs from "linkifyjs";
import { registerCustomProtocol, registerPlugin } from "linkifyjs";
import { Opts, registerCustomProtocol, registerPlugin } from "linkifyjs";
import linkifyElement from "linkify-element";
import linkifyString from "linkify-string";
import { RoomMember } from "matrix-js-sdk/src/models/room-member";
Expand Down Expand Up @@ -139,9 +139,9 @@ export const ELEMENT_URL_PATTERN =
"(?:app|beta|staging|develop)\\.element\\.io/" +
")(#.*)";

export const options = {
events: function (href: string, type: Type | string): Partial<GlobalEventHandlers> {
switch (type) {
export const options: Opts = {
events: function (href: string, type: string): Partial<GlobalEventHandlers> {
switch (type as Type) {
case Type.URL: {
// intercept local permalinks to users and show them like userids (in userinfo of current room)
try {
Expand Down
Loading

0 comments on commit 2bde31d

Please sign in to comment.