Skip to content

[Fiber] Clear existing text content before inserting children #8331

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.

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

Merged
merged 1 commit into from
Dec 1, 2016

Conversation

acdlite
Copy link
Collaborator

@acdlite acdlite commented Nov 17, 2016

Fixes a bug when updating from a single text child (or dangerouslySetInnerHTML) to regular children, where the previous text content never gets deleted.

Based on #8304 (compare view).

@acdlite
Copy link
Collaborator Author

acdlite commented Nov 17, 2016

Here's a description of what I've added:

  • In reconciliation phase, check if the host component is going from a single text child to normal children. If so, schedule a ContentReset effect. The logic for checking if the host component should render with a text child is now part of the host config. We can't just check for text and number children because of special cases like dangerouslySetInnerHTML.
  • Whenever a node is being inserted, check if the parent has a ContentReset effect. If so, reset the text content of the parent and remove the flag from the effect tag. Resetting the text content of a host instance was also added to the host config.
  • In the commit phase, perform any ContentReset effects that weren't already handled by child insertions.

},

resetTextContent(domElement : Instance) : void {
domElement.textContent = '';
Copy link
Contributor

Choose a reason for hiding this comment

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

If we're concerned with performance then IIRC setting textContent or innerHTML is unintuitively significantly slower than simply clearing the children one by one.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This only gets called when there's a single text child

Copy link
Collaborator

Choose a reason for hiding this comment

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

That's not true because dangerouslySetInnerHTML can have multiple children.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh right that's true

@sebmarkbage
Copy link
Collaborator

Ok, accepting this but not the things it build on top of. So this will either have to wait or rebase on top of master.

Fixes a bug when updating from a single text child (or
dangerouslySetInnerHTML) to regular children, where the previous
text content never gets deleted.
@acdlite acdlite force-pushed the fiberresettextcontent branch from 25e7cbd to 0d7225a Compare December 1, 2016 01:48
@acdlite acdlite merged commit aa6279c into facebook:master Dec 1, 2016
let nextChildren = workInProgress.pendingProps.children;
if (typeof nextChildren === 'string' || typeof nextChildren === 'number') {
const isDirectTextChild = config.shouldSetTextContent(nextProps);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: This should've been cached in a local variable above instead of using the object form.

@arcanis
Copy link
Contributor

arcanis commented Jan 12, 2017

Just a small feedback after implementing shouldSetTextContent in a custom renderer: it could be useful for the hook to also get the type of the parent instance. It would enable this kind of logic, where we want to ensure that text nodes are wrapped into a specific type, but also want to allow using this type directly in case the user wants to add extra props to the component:

createInstance(type, props) {
    return new (components.get(type))(props);
}

createTextInstance(text) {
    return new (components.get('text'))({ text });
}

shouldSetTextContent(type, props) {
    return type === 'text';
}

acusti pushed a commit to brandcast/react that referenced this pull request Mar 15, 2017
Fixes a bug when updating from a single text child (or
dangerouslySetInnerHTML) to regular children, where the previous
text content never gets deleted.
laurinenas pushed a commit to laurinenas/react that referenced this pull request May 28, 2018
Fixes a bug when updating from a single text child (or
dangerouslySetInnerHTML) to regular children, where the previous
text content never gets deleted.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants