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

[Fiber] Set DOM attributes #7992

Closed
wants to merge 1 commit into from
Closed

[Fiber] Set DOM attributes #7992

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 4 additions & 0 deletions src/isomorphic/modern/class/__tests__/ReactES6Class-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,10 @@ describe('ReactES6Class', () => {
var renderedName = null;

beforeEach(() => {
// TODO: Fiber doesn't currently handle errors well
// so one test failure brings the other tests down.
jest.resetModuleRegistry();

React = require('React');
ReactDOM = require('ReactDOM');
container = document.createElement('div');
Expand Down
2 changes: 1 addition & 1 deletion src/renderers/dom/__tests__/ReactDOMProduction-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ describe('ReactDOMProduction', () => {
container
);

expect(container.firstChild).toBe(inst);
expect(container.firstChild === inst).toBe(true);
expect(inst.className).toBe('blue');
expect(inst.textContent).toBe('ABC');

Expand Down
13 changes: 10 additions & 3 deletions src/renderers/dom/client/wrappers/ReactDOMInput.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,15 +191,22 @@ var ReactDOMInput = {

// TODO: Shouldn't this be getChecked(props)?
var checked = props.checked;
var node = ReactDOMComponentTree.getNodeFromInstance(inst);

var debugID = 0;
if (__DEV__) {
debugID = inst._debugID;
}

if (checked != null) {
DOMPropertyOperations.setValueForProperty(
ReactDOMComponentTree.getNodeFromInstance(inst),
node,
'checked',
checked || false
checked || false,
debugID
);
}

var node = ReactDOMComponentTree.getNodeFromInstance(inst);
var value = LinkedValueUtils.getValue(props);
if (value != null) {

Expand Down
68 changes: 68 additions & 0 deletions src/renderers/dom/fiber/ReactDOMFiber.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,19 @@
import type { HostChildren } from 'ReactFiberReconciler';

var ReactFiberReconciler = require('ReactFiberReconciler');
var ReactDefaultInjection = require('ReactDefaultInjection');
var ReactDOMFeatureFlags = require('ReactDOMFeatureFlags');

var {
properties: DOMProperties,
isCustomAttribute,
} = require('DOMProperty');

var {
deleteValueForProperty,
setValueForProperty,
} = require('DOMPropertyOperations');

var warning = require('warning');

type DOMContainerElement = Element & { _reactRootContainer: ?Object };
Expand All @@ -26,6 +37,11 @@ type Props = { };
type Instance = Element;
type TextInstance = Text;

// TODO: this brings a lot of non-Fiber code into the Fiber bundle.
// However, without it tests that use both ReactDOM (Fiber) and
// ReactDOMServer (non-Fiber) in the same file break due to "double injection".
ReactDefaultInjection.inject();

function recursivelyAppendChildren(parent : Element, child : HostChildren<Instance | TextInstance>) {
if (!child) {
return;
Expand All @@ -43,6 +59,55 @@ function recursivelyAppendChildren(parent : Element, child : HostChildren<Instan
}
}

function removeMissingPrevDOMProperties(
domElement: Element,
oldProps: Props,
newProps: Props,
) {
for (const propKey in oldProps) {
if (newProps.hasOwnProperty(propKey) ||
!oldProps.hasOwnProperty(propKey) ||
oldProps[propKey] == null) {
continue;
}
// TODO: unregister events, remove styles, handle custom components
if (DOMProperties[propKey] || isCustomAttribute(propKey)) {
deleteValueForProperty(domElement, propKey, 0);
}
}
}

function applyNextDOMProperties(
domElement: Element,
oldProps: Props | null,
newProps: Props,
) {
// TODO: associate DOM nodes with components for ReactPerf
Copy link
Contributor

Choose a reason for hiding this comment

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

Trying to understand how some things work, it looks like the ReactDOMComponentTree logic was hoisted out of ReactDOMProperties as a stepping stone to this TODO.

If I understand correctly: had this change not hoisted ReactDOMComponentTree out of the (set|delete)ValueForProperty calls the tests would fail. With this change a subset of the tests and functionality pass, though the ReactPerf and ReactDevtools would still not be able to take advantage of this.

Is this an okay-ish understanding or am I off?

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 is right. (I also was the person who put that DOMComponentTree thing into DOMOperations during ReactPerf rewrite because I was too lazy to pass debugID explicitly and now it's my time to pay for this.)

const debugID = 1;
for (const propKey in newProps) {
const newProp = newProps[propKey];
const oldProp = oldProps != null ? oldProps[propKey] : undefined;

if (!newProps.hasOwnProperty(propKey) ||
newProp === oldProp ||
newProp == null && oldProp == null) {
continue;
}

// TODO: register events, add styles, handle custom components
if (DOMProperties[propKey] || isCustomAttribute(propKey)) {
// If we're updating to null or undefined, we should remove the property
// from the DOM node instead of inadvertently setting to a string. This
// brings us in line with the same behavior we have on initial render.
if (newProp != null) {
setValueForProperty(domElement, propKey, newProp, debugID);
} else {
deleteValueForProperty(domElement, propKey, debugID);
}
}
}
}

var DOMRenderer = ReactFiberReconciler({

updateContainer(container : Container, children : HostChildren<Instance | TextInstance>) : void {
Expand All @@ -53,6 +118,7 @@ var DOMRenderer = ReactFiberReconciler({

createInstance(type : string, props : Props, children : HostChildren<Instance | TextInstance>) : Instance {
const domElement = document.createElement(type);
applyNextDOMProperties(domElement, null, props);
recursivelyAppendChildren(domElement, children);
if (typeof props.children === 'string' ||
typeof props.children === 'number') {
Expand All @@ -70,6 +136,8 @@ var DOMRenderer = ReactFiberReconciler({
},

commitUpdate(domElement : Instance, oldProps : Props, newProps : Props) : void {
removeMissingPrevDOMProperties(domElement, oldProps, newProps);
applyNextDOMProperties(domElement, oldProps, newProps);
if (typeof newProps.children === 'string' ||
typeof newProps.children === 'number') {
domElement.textContent = newProps.children;
Expand Down
21 changes: 10 additions & 11 deletions src/renderers/dom/shared/DOMPropertyOperations.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@
'use strict';

var DOMProperty = require('DOMProperty');
var ReactDOMComponentTree = require('ReactDOMComponentTree');
var ReactInstrumentation = require('ReactInstrumentation');

var quoteAttributeValueForBrowser = require('quoteAttributeValueForBrowser');
Expand Down Expand Up @@ -130,15 +129,15 @@ var DOMPropertyOperations = {
* @param {string} name
* @param {*} value
*/
setValueForProperty: function(node, name, value) {
setValueForProperty: function(node, name, value, debugID) {
var propertyInfo = DOMProperty.properties.hasOwnProperty(name) ?
DOMProperty.properties[name] : null;
if (propertyInfo) {
var mutationMethod = propertyInfo.mutationMethod;
if (mutationMethod) {
mutationMethod(node, value);
} else if (shouldIgnoreValue(propertyInfo, value)) {
this.deleteValueForProperty(node, name);
this.deleteValueForProperty(node, name, debugID);
return;
} else if (propertyInfo.mustUseProperty) {
// Contrary to `setAttribute`, object properties are properly
Expand All @@ -159,22 +158,22 @@ var DOMPropertyOperations = {
}
}
} else if (DOMProperty.isCustomAttribute(name)) {
DOMPropertyOperations.setValueForAttribute(node, name, value);
DOMPropertyOperations.setValueForAttribute(node, name, value, debugID);
return;
}

if (__DEV__) {
var payload = {};
payload[name] = value;
ReactInstrumentation.debugTool.onHostOperation({
instanceID: ReactDOMComponentTree.getInstanceFromNode(node)._debugID,
instanceID: debugID,
type: 'update attribute',
payload: payload,
});
}
},

setValueForAttribute: function(node, name, value) {
setValueForAttribute: function(node, name, value, debugID) {
if (!isAttributeNameSafe(name)) {
return;
}
Expand All @@ -188,7 +187,7 @@ var DOMPropertyOperations = {
var payload = {};
payload[name] = value;
ReactInstrumentation.debugTool.onHostOperation({
instanceID: ReactDOMComponentTree.getInstanceFromNode(node)._debugID,
instanceID: debugID,
type: 'update attribute',
payload: payload,
});
Expand All @@ -201,11 +200,11 @@ var DOMPropertyOperations = {
* @param {DOMElement} node
* @param {string} name
*/
deleteValueForAttribute: function(node, name) {
deleteValueForAttribute: function(node, name, debugID) {
node.removeAttribute(name);
if (__DEV__) {
ReactInstrumentation.debugTool.onHostOperation({
instanceID: ReactDOMComponentTree.getInstanceFromNode(node)._debugID,
instanceID: debugID,
type: 'remove attribute',
payload: name,
});
Expand All @@ -218,7 +217,7 @@ var DOMPropertyOperations = {
* @param {DOMElement} node
* @param {string} name
*/
deleteValueForProperty: function(node, name) {
deleteValueForProperty: function(node, name, debugID) {
var propertyInfo = DOMProperty.properties.hasOwnProperty(name) ?
DOMProperty.properties[name] : null;
if (propertyInfo) {
Expand All @@ -241,7 +240,7 @@ var DOMPropertyOperations = {

if (__DEV__) {
ReactInstrumentation.debugTool.onHostOperation({
instanceID: ReactDOMComponentTree.getInstanceFromNode(node)._debugID,
instanceID: debugID,
type: 'remove attribute',
payload: name,
});
Expand Down
16 changes: 11 additions & 5 deletions src/renderers/dom/shared/ReactDOMComponent.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -963,6 +963,10 @@ ReactDOMComponent.Mixin = {
transaction,
isCustomComponentTag
) {
var debugID = 0;
if (__DEV__) {
debugID = this._debugID;
}
var propKey;
var styleName;
var styleUpdates;
Expand Down Expand Up @@ -992,13 +996,14 @@ ReactDOMComponent.Mixin = {
if (!RESERVED_PROPS.hasOwnProperty(propKey)) {
DOMPropertyOperations.deleteValueForAttribute(
getNode(this),
propKey
propKey,
debugID
);
}
} else if (
DOMProperty.properties[propKey] ||
DOMProperty.isCustomAttribute(propKey)) {
DOMPropertyOperations.deleteValueForProperty(getNode(this), propKey);
DOMPropertyOperations.deleteValueForProperty(getNode(this), propKey, debugID);
}
}
for (propKey in nextProps) {
Expand Down Expand Up @@ -1057,7 +1062,8 @@ ReactDOMComponent.Mixin = {
DOMPropertyOperations.setValueForAttribute(
getNode(this),
propKey,
nextProp
nextProp,
debugID
);
}
} else if (
Expand All @@ -1068,9 +1074,9 @@ ReactDOMComponent.Mixin = {
// from the DOM node instead of inadvertently setting to a string. This
// brings us in line with the same behavior we have on initial render.
if (nextProp != null) {
DOMPropertyOperations.setValueForProperty(node, propKey, nextProp);
DOMPropertyOperations.setValueForProperty(node, propKey, nextProp, debugID);
} else {
DOMPropertyOperations.deleteValueForProperty(node, propKey);
DOMPropertyOperations.deleteValueForProperty(node, propKey, debugID);
}
}
}
Expand Down
Loading