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

Commit

Permalink
Attempt to fix Failed to execute 'removeChild' on 'Node' (#9196)
Browse files Browse the repository at this point in the history 
* 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
  • Loading branch information
@t3chguy
t3chguy committed Aug 17, 2022
1 parent 3461573 commit 27a7263
Show file tree
Hide file tree
Showing 4 changed files with 66 additions and 65 deletions.
8 changes: 4 additions & 4 deletions src/HtmlUtils.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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>;
}
Expand Down
70 changes: 37 additions & 33 deletions src/components/views/elements/Tooltip.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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;
Expand All @@ -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) {
Expand Down Expand Up @@ -153,40 +166,31 @@ 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" />
{ this.props.label }
</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>
);
}
}
39 changes: 18 additions & 21 deletions test/components/views/elements/TooltipTarget-test.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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();
Expand Down Expand Up @@ -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();
});
Expand Down
14 changes: 7 additions & 7 deletions test/components/views/elements/__snapshots__/TooltipTarget-test.tsx.snap
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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"
Expand All @@ -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"
Expand All @@ -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"
Expand All @@ -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"
Expand All @@ -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"
Expand All @@ -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"
Expand Down

0 comments on commit 27a7263

Please sign in to comment.