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.

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

Fixing the instantiation of customized builtin elements (related to custom elements) #9313

Merged
merged 5 commits into from Apr 19, 2017

Conversation

Projects
None yet
3 participants
@joeldenning
Copy link
Contributor

commented Mar 31, 2017

The custom element spec, combined with the document.createElement spec explain that in order to create a DOM element as a customized built-in element, you should call document.createElement with an object as the second parameter. Customized builtin elements are a type of custom element that allow you to extend native DOM elements. However, React currently provides the second parameter to document.createElement as a string instead of an object. As far as I can tell, that is just a bug.

I am not sure of the full history of the custom elements spec, and so it might be possible that the spec used to say the second argument to createElement should be a string. I'm also not sure if anybody is depending on the behavior being like it is right now, or if this change would be considered a breaking change or not for React. My thoughts are that it is a bug fix, but I may be lacking some context here.

@@ -552,7 +552,7 @@ ReactDOMComponent.Mixin = {
div.innerHTML = `<${type}></${type}>`;
el = div.removeChild(div.firstChild);
} else if (props.is) {
el = ownerDocument.createElement(type, props.is);
el = ownerDocument.createElement(type, {is: props.is});

This comment has been minimized.

Copy link
@joeldenning

joeldenning Mar 31, 2017

Author Contributor

See https://dom.spec.whatwg.org/#dom-document-createelement.

The is prop indicates that the react element should be a customized builtin element.

@@ -1045,7 +1045,7 @@ describe('ReactDOMComponent', () => {
ReactDOM.render(<div is="custom-div" />, container);
expect(document.createElement).toHaveBeenCalledWith(
'div',
'custom-div',
{is: 'custom-div'},

This comment has been minimized.

Copy link
@joeldenning

joeldenning Mar 31, 2017

Author Contributor

document.createElement should not have a second argument of a string, but rather an options object

@joeldenning

This comment has been minimized.

Copy link
Contributor Author

commented Apr 8, 2017

Updated this to fix the build -- I had forgotten to npm run prettier before creating this pr.

@gaearon
Copy link
Member

left a comment

The Fiber tests are failing (that’s a new engine that’s hidden behind a flag).

There is equivalent code in ReactDOMFiberComponent.js, could you please fix it too and then run scripts/fiber/record-tests?

@joeldenning

This comment has been minimized.

Copy link
Contributor Author

commented Apr 18, 2017

@gaearon thanks for the help -- I have updated ReactDOMFiberComponent as suggested, but the fiber tests are failing for me locally. The same tests fail for me whether I'm on the master branch or on my branch, so I don't think it's actually related to my code changes. And it changes the tests-failing and tests-passing files, even on the master branch. So what I have done is push my change to ReactDOMFiberComponent without also adding my changes to scripts/fiber/tests-failing.txt and scripts/fiber/tests-passing.txt. I am hoping that maybe the CI environment will be better configured or something so that the tests pass there even though they failed for me locally.

The offending tests are:
src/isomorphic/classic/types/tests/ReactPropTypes-test.js

  • should warn if called manually in development
  • should warn if called manually in development
  • should warn if called manually in development

Maybe related to react@15.5, since it's related to PropTypes??? I am not sure if it's just my local env that is causing issues or if maybe the tests-failing.txt file has not been updated since react@15.5?? Would appreciate any help you could offer.

@gaearon

This comment has been minimized.

Copy link
Member

commented Apr 19, 2017

I am not sure of the full history of the custom elements spec, and so it might be possible that the spec used to say the second argument to createElement should be a string

Pretty sure this is the case. MDN:

For backwards compatibility with previous versions of the Custom Elements specification, some browsers will allow you to pass a string here instead of an object, where the string's value is the custom element's tag name.

The feature itself was originally added in #6570, and I don’t think it was extensively tested or updated to match the latest spec.

For safety I’ll merge this to master but won’t backport to 15.x. Thanks!

@gaearon gaearon merged commit 233195c into facebook:master Apr 19, 2017

1 check passed

ci/circleci Your tests passed on CircleCI!
Details

@gaearon gaearon added this to the 16.0 milestone Apr 19, 2017

@joeldenning joeldenning deleted the joeldenning:customized-builtin-elements branch Apr 19, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.