Attempt to fix Failed to execute 'removeChild' on 'Node' (#9196)

* Switch tooltips to use React Portals * Remove redundant React key to simplify reconciliation * Fix cleanup and it.each test * Update snapshots due to style order difference
2022-08-17 14:35:33 +01:00 · 2022-08-17 14:35:33 +01:00 · 27a7263965
commit 27a7263965
parent 3461573df9
4 changed files with 66 additions and 65 deletions
--- a/src/HtmlUtils.tsx
+++ b/src/HtmlUtils.tsx
@ -632,13 +632,13 @@ export function topicToHtml(
        emojiBodyElements = formatEmojis(topic, false);
    }

-    return isFormattedTopic ?
-        <span
-            key="body"
+    return isFormattedTopic
+        ? <span
            ref={ref}
            dangerouslySetInnerHTML={{ __html: safeTopic }}
            dir="auto"
-        /> : <span key="body" ref={ref} dir="auto">
+        />
+        : <span ref={ref} dir="auto">
            { emojiBodyElements || topic }
        </span>;
 }
--- a/src/components/views/elements/Tooltip.tsx
+++ b/src/components/views/elements/Tooltip.tsx
@ -52,8 +52,10 @@ export interface ITooltipProps {
        maxParentWidth?: number;
 }

-export default class Tooltip extends React.Component<ITooltipProps> {
-    private tooltipContainer: HTMLElement;
+type State = Partial<Pick<CSSProperties, "display" | "right" | "top" | "transform" | "left">>;
+
+export default class Tooltip extends React.PureComponent<ITooltipProps, State> {
+    private static container: HTMLElement;
    private parent: Element;

    // XXX: This is because some components (Field) are unable to `import` the Tooltip class,
@ -65,37 +67,47 @@ export default class Tooltip extends React.Component<ITooltipProps> {
        alignment: Alignment.Natural,
    };

-    // Create a wrapper for the tooltip outside the parent and attach it to the body element
+    constructor(props) {
+        super(props);
+
+        this.state = {};
+
+        // Create a wrapper for the tooltips and attach it to the body element
+        if (!Tooltip.container) {
+            Tooltip.container = document.createElement("div");
+            Tooltip.container.className = "mx_Tooltip_wrapper";
+            document.body.appendChild(Tooltip.container);
+        }
+    }
+
    public componentDidMount() {
-        this.tooltipContainer = document.createElement("div");
-        this.tooltipContainer.className = "mx_Tooltip_wrapper";
-        document.body.appendChild(this.tooltipContainer);
-        window.addEventListener('scroll', this.renderTooltip, {
+        window.addEventListener('scroll', this.updatePosition, {
            passive: true,
            capture: true,
        });

        this.parent = ReactDOM.findDOMNode(this).parentNode as Element;

-        this.renderTooltip();
+        this.updatePosition();
    }

    public componentDidUpdate() {
-        this.renderTooltip();
+        this.updatePosition();
    }

    // Remove the wrapper element, as the tooltip has finished using it
    public componentWillUnmount() {
-        ReactDOM.unmountComponentAtNode(this.tooltipContainer);
-        document.body.removeChild(this.tooltipContainer);
-        window.removeEventListener('scroll', this.renderTooltip, {
+        window.removeEventListener('scroll', this.updatePosition, {
            capture: true,
        });
    }

    // Add the parent's position to the tooltips, so it's correctly
    // positioned, also taking into account any window zoom
-    private updatePosition(style: CSSProperties) {
+    private updatePosition = (): void => {
+        // When the tooltip is hidden, no need to thrash the DOM with `style` attribute updates (performance)
+        if (!this.props.visible) return;
+
        const parentBox = this.parent.getBoundingClientRect();
        const width = UIStore.instance.windowWidth;
        const spacing = 6;
@ -112,6 +124,7 @@ export default class Tooltip extends React.Component<ITooltipProps> {
            parentBox.left - window.scrollX + (parentWidth / 2)
        );

+        const style: State = {};
        switch (this.props.alignment) {
            case Alignment.Natural:
                if (parentBox.right > width / 2) {
@ -153,25 +166,20 @@ export default class Tooltip extends React.Component<ITooltipProps> {
                break;
        }

-        return style;
-    }
-
-    private renderTooltip = () => {
-        let style: CSSProperties = {};
-        // When the tooltip is hidden, no need to thrash the DOM with `style`
-        // attribute updates (performance)
-        if (this.props.visible) {
-            style = this.updatePosition({});
-        }
-        // Hide the entire container when not visible. This prevents flashing of the tooltip
-        // if it is not meant to be visible on first mount.
-        style.display = this.props.visible ? "block" : "none";
+        this.setState(style);
+    };

+    public render() {
        const tooltipClasses = classNames("mx_Tooltip", this.props.tooltipClassName, {
            "mx_Tooltip_visible": this.props.visible,
            "mx_Tooltip_invisible": !this.props.visible,
        });

+        const style = { ...this.state };
+        // Hide the entire container when not visible.
+        // This prevents flashing of the tooltip if it is not meant to be visible on first mount.
+        style.display = this.props.visible ? "block" : "none";
+
        const tooltip = (
            <div className={tooltipClasses} style={style}>
                <div className="mx_Tooltip_chevron" />
@ -179,14 +187,10 @@ export default class Tooltip extends React.Component<ITooltipProps> {
            </div>
        );

-        // Render the tooltip manually, as we wish it not to be rendered within the parent
-        ReactDOM.render<Element>(tooltip, this.tooltipContainer);
-    };
-
-    public render() {
-        // Render a placeholder
        return (
-            <div className={this.props.className} />
+            <div className={this.props.className}>
+                { ReactDOM.createPortal(tooltip, Tooltip.container) }
+            </div>
        );
    }
 }
--- a/test/components/views/elements/TooltipTarget-test.tsx
+++ b/test/components/views/elements/TooltipTarget-test.tsx
@ -35,6 +35,14 @@ describe('<TooltipTarget />', () => {
        'data-test-id': 'test',
    };

+    afterEach(() => {
+        // clean up renderer tooltips
+        const wrapper = document.querySelector('.mx_Tooltip_wrapper');
+        while (wrapper?.firstChild) {
+            wrapper.removeChild(wrapper.lastChild);
+        }
+    });
+
    const getComponent = (props = {}) => {
        const wrapper = renderIntoDocument<HTMLSpanElement>(
        // wrap in element so renderIntoDocument can render functional component
@ -49,31 +57,20 @@ describe('<TooltipTarget />', () => {

    const getVisibleTooltip = () => document.querySelector('.mx_Tooltip.mx_Tooltip_visible');

-    afterEach(() => {
-        // clean up visible tooltips
-        const tooltipWrapper = document.querySelector('.mx_Tooltip_wrapper');
-        if (tooltipWrapper) {
-            document.body.removeChild(tooltipWrapper);
-        }
-    });
-
    it('renders container', () => {
        const component = getComponent();
        expect(component).toMatchSnapshot();
        expect(getVisibleTooltip()).toBeFalsy();
    });

-    for (const alignment in Alignment) {
-        if (isNaN(Number(alignment))) {
-            it(`displays ${alignment} aligned tooltip on mouseover`, () => {
-                const wrapper = getComponent({ alignment: Alignment[alignment] });
-                act(() => {
-                    Simulate.mouseOver(wrapper);
-                });
-                expect(getVisibleTooltip()).toMatchSnapshot();
-            });
-        }
-    }
+    const alignmentKeys = Object.keys(Alignment).filter((o: any) => isNaN(o));
+    it.each(alignmentKeys)("displays %s aligned tooltip on mouseover", async (alignment) => {
+        const wrapper = getComponent({ alignment: Alignment[alignment] });
+        act(() => {
+            Simulate.mouseOver(wrapper);
+        });
+        expect(getVisibleTooltip()).toMatchSnapshot();
+    });

    it('hides tooltip on mouseleave', () => {
        const wrapper = getComponent();
@ -101,8 +98,8 @@ describe('<TooltipTarget />', () => {
            Simulate.focus(wrapper);
        });
        expect(getVisibleTooltip()).toBeTruthy();
-        await act(async () => {
-            await Simulate.blur(wrapper);
+        act(() => {
+            Simulate.blur(wrapper);
        });
        expect(getVisibleTooltip()).toBeFalsy();
    });
--- a/test/components/views/elements/snapshots/TooltipTarget-test.tsx.snap
+++ b/test/components/views/elements/snapshots/TooltipTarget-test.tsx.snap
@ -3,7 +3,7 @@
 exports[`<TooltipTarget /> displays Bottom aligned tooltip on mouseover 1`] = `
 <div
  class="mx_Tooltip test tooltipClassName mx_Tooltip_visible"
-  style="top: 6px; left: 0px; transform: translate(-50%); display: block;"
+  style="display: block; top: 6px; left: 0px; transform: translate(-50%);"
 >
  <div
    class="mx_Tooltip_chevron"
@ -15,7 +15,7 @@ exports[`<TooltipTarget /> displays Bottom aligned tooltip on mouseover 1`] = `
 exports[`<TooltipTarget /> displays InnerBottom aligned tooltip on mouseover 1`] = `
 <div
  class="mx_Tooltip test tooltipClassName mx_Tooltip_visible"
-  style="top: -50px; left: 0px; transform: translate(-50%); display: block;"
+  style="display: block; top: -50px; left: 0px; transform: translate(-50%);"
 >
  <div
    class="mx_Tooltip_chevron"
@ -27,7 +27,7 @@ exports[`<TooltipTarget /> displays InnerBottom aligned tooltip on mouseover 1`]
 exports[`<TooltipTarget /> displays Left aligned tooltip on mouseover 1`] = `
 <div
  class="mx_Tooltip test tooltipClassName mx_Tooltip_visible"
-  style="right: 1030px; top: 0px; transform: translateY(-50%); display: block;"
+  style="display: block; right: 1030px; top: 0px; transform: translateY(-50%);"
 >
  <div
    class="mx_Tooltip_chevron"
@ -39,7 +39,7 @@ exports[`<TooltipTarget /> displays Left aligned tooltip on mouseover 1`] = `
 exports[`<TooltipTarget /> displays Natural aligned tooltip on mouseover 1`] = `
 <div
  class="mx_Tooltip test tooltipClassName mx_Tooltip_visible"
-  style="left: 6px; top: 0px; transform: translateY(-50%); display: block;"
+  style="display: block; left: 6px; top: 0px; transform: translateY(-50%);"
 >
  <div
    class="mx_Tooltip_chevron"
@ -51,7 +51,7 @@ exports[`<TooltipTarget /> displays Natural aligned tooltip on mouseover 1`] = `
 exports[`<TooltipTarget /> displays Right aligned tooltip on mouseover 1`] = `
 <div
  class="mx_Tooltip test tooltipClassName mx_Tooltip_visible"
-  style="left: 6px; top: 0px; transform: translateY(-50%); display: block;"
+  style="display: block; left: 6px; top: 0px; transform: translateY(-50%);"
 >
  <div
    class="mx_Tooltip_chevron"
@ -63,7 +63,7 @@ exports[`<TooltipTarget /> displays Right aligned tooltip on mouseover 1`] = `
 exports[`<TooltipTarget /> displays Top aligned tooltip on mouseover 1`] = `
 <div
  class="mx_Tooltip test tooltipClassName mx_Tooltip_visible"
-  style="top: -6px; left: 0px; transform: translate(-50%, -100%); display: block;"
+  style="display: block; top: -6px; left: 0px; transform: translate(-50%, -100%);"
 >
  <div
    class="mx_Tooltip_chevron"
@ -75,7 +75,7 @@ exports[`<TooltipTarget /> displays Top aligned tooltip on mouseover 1`] = `
 exports[`<TooltipTarget /> displays TopRight aligned tooltip on mouseover 1`] = `
 <div
  class="mx_Tooltip test tooltipClassName mx_Tooltip_visible"
-  style="top: -6px; right: 1024px; transform: translateY(-100%); display: block;"
+  style="display: block; top: -6px; right: 1024px; transform: translateY(-100%);"
 >
  <div
    class="mx_Tooltip_chevron"