Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Post Editor: Update EmbedView to use createRef instead of string refs #39316

Closed
wants to merge 2 commits into from
Closed

Post Editor: Update EmbedView to use createRef instead of string refs #39316

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e7359dc
TinyMCE: Remove unnecessary wrapper handling in EmbedView
tyxla Feb 7, 2020
f755608
TinyMCE: Use createRef in EmbedView
tyxla Feb 7, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 13 additions & 29 deletions client/components/tinymce/plugins/wpcom-view/views/embed/view.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,9 @@
/* eslint-disable react/no-string-refs */

/**
* External dependencies
*/

import ReactDom from 'react-dom';
import PropTypes from 'prop-types';
import React, { Component } from 'react';
import React, { Component, createRef } from 'react';
import { pick } from 'lodash';
import { connect } from 'react-redux';

Expand All @@ -19,25 +16,12 @@ import QueryEmbed from 'components/data/query-embed';
import ResizableIframe from 'components/resizable-iframe';

class EmbedView extends Component {
state = {
wrapper: null,
};
view = createRef();

iframe = createRef();

componentDidMount() {
// Rendering the frame follows a specific set of steps, whereby an
// initial rendering pass is made, at which time the frame is rendered
// in a second pass, before finally setting the frame markup.
//
// TODO: Investigate and evaluate whether we need to avoid rendering
// the iframe on the initial render pass
// eslint-disable-next-line react/no-did-mount-set-state
this.setState(
{
// eslint-disable-line react/no-did-mount-set-state
wrapper: this.refs.view,
},
this.setHtml
);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm afraid this setState() call, which forces a two pass render, might be necessary. It first renders the container <div> and then, in second pass, creates the <iframe>. Instead of rendering <div><iframe></div> in one pass.

When iframe is being created in DOM, it might go through some weird intermediate state where iframe.contentDocument or iframe.contentWindow are null. And they are initialized only a moment later, when some async process inside the browser finishes.

If iframe.contentDocument is null, our code can either crash, or fail to set the content HTML.

I've been debugging this a few months ago inside the components/web-preview component and still don't understand it.

Here's the relevant definition from the spec: https://html.spec.whatwg.org/multipage/iframe-embed-object.html#dom-iframe-contentwindow

I don't know what "nested browsing context" means and when it exists or not.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure either 🤷 I prefer closing this one to avoid introducing subtle bugs. Let's revisit in the future 👍

this.setHtml();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

componentDidMount should also call constrainEmbedDimensions() here.

Previously, there was a setState( { wrapper } ) call that immediately triggered a second render. On that second render, componentDidUpdate was called with in turn called constrainEmbedDimensions.

But now, when this setState() is gone, there can be just one initial render with no further updates.

}

componentDidUpdate( prevProps ) {
Expand All @@ -49,12 +33,12 @@ class EmbedView extends Component {
}

constrainEmbedDimensions() {
if ( ! this.refs.iframe ) {
if ( ! this.iframe ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this.iframe is always an object, for example, { current: null }. What you really want to check for is ! this.iframe.current.

return;
}

const view = ReactDom.findDOMNode( this.refs.view );
const iframe = ReactDom.findDOMNode( this.refs.iframe );
const view = this.view.current;
const iframe = ReactDom.findDOMNode( this.iframe.current );
if ( ! iframe.contentDocument ) {
return;
}
Expand All @@ -78,11 +62,11 @@ class EmbedView extends Component {
}

setHtml() {
if ( ! this.props.embed?.body || ! this.refs.iframe ) {
if ( ! this.props.embed?.body || ! this.iframe ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check for this.iframe.current instead.

return;
}

const iframe = ReactDom.findDOMNode( this.refs.iframe );
const iframe = ReactDom.findDOMNode( this.iframe.current );
if ( ! iframe.contentDocument ) {
return;
}
Expand All @@ -98,13 +82,13 @@ class EmbedView extends Component {
}

renderFrame() {
if ( ! this.state.wrapper || ! this.props.embed ) {
if ( ! this.props.embed ) {
return;
}

return (
<ResizableIframe
ref="iframe"
ref={ this.iframe }
onResize={ this.props.onResize }
frameBorder="0"
seamless
Expand All @@ -118,7 +102,7 @@ class EmbedView extends Component {

return (
// eslint-disable-next-line wpcalypso/jsx-classname-namespace
<div ref="view" className="wpview-content wpview-type-embed">
<div ref={ this.view } className="wpview-content wpview-type-embed">
<QueryEmbed siteId={ siteId } url={ content } />

{ this.renderFrame() }
Expand Down