Add warning if rendering and HTMLUnknownElement #9163

arshabh · 2017-03-12T21:15:42Z

Fixes 3726

syranide · 2017-03-15T08:40:38Z

src/renderers/dom/stack/client/ReactDOMComponent.js

@@ -603,6 +608,20 @@ ReactDOMComponent.Mixin = {
        );
        didWarnShadyDOM = true;
      }
+
+      if (this._namespaceURI === DOMNamespaces.html) {
+        if (__DEV__) {


This should be one line up, outside the namespace-check.

syranide · 2017-03-15T08:41:05Z

src/renderers/dom/stack/client/ReactDOMComponent.js

@@ -458,7 +462,7 @@ ReactDOMComponent.Mixin = {
    hostParent,
    hostContainerInfo,
    context
-  ) {
+  ) {    


Need to trim trailing spaces in your file.

syranide · 2017-03-15T08:45:05Z

src/renderers/dom/stack/client/ReactDOMComponent.js

@@ -391,6 +391,10 @@ var VALID_TAG_REGEX = /^[a-zA-Z][a-zA-Z:_\.\-\d]*$/; // Simplified subset
 var validatedTagCache = {};
 var hasOwnProperty = {}.hasOwnProperty;

+// In IE8 We dont have HTMLUnknownElement - thus we use the equivalent HTMLGenericElement
+var HTMLUnknownElement = typeof window.HTMLGenericElement !== 'undefined' ?


I don't believe React actually supports IE8 in DEV anymore so this should be redundant then. Devs?

Correct, we do not need to explicitly support IE8 anymore.

aweary · 2017-03-16T00:00:06Z

src/renderers/dom/stack/client/ReactDOMComponent.js

+      if (__DEV__) {
+        if (this._namespaceURI === DOMNamespaces.html) {
+          warning(
+            !(document.createElement(this._tag) instanceof window.HTMLUnknownElement) ||


Can we just use the element that was already created above (stored in the el variable)?

If there's some reason we need to create another element here, we should use ownerDocument instead of document to create the element.

Well, we cant really test SVG elements using HTMLUnknownElement. Thus i will use ownerDocument as you suggested (Edit: Please ignore this.)

Using el instead of creating the element again.

@aweary This is done. Can you please take a look into it ?

aweary · 2017-03-16T00:04:02Z

Does JSDOM still return HTMLUnknownElement for elements it does not support yet (ref)?

arshabh · 2017-03-16T02:44:48Z

Yes it still behaves the same. All tests were failing which created a SVG element.

syranide · 2017-03-16T10:44:10Z

src/renderers/dom/stack/client/ReactDOMComponent.js

+        if (this._namespaceURI === DOMNamespaces.html) {
+          warning(
+            !(ownerDocument.createElement(this._tag) instanceof window.HTMLUnknownElement) ||
+              isCustomComponent(this._tag, props) || voidElementTags[this._tag],


It's not apparent to me why checking against voidElementTags makes sense here.

This was added to handle specific scenario of menuitem tag. The <menuitem></menuitem> is supported only on firefox. In all other browsers the instanceof window.HTMLUnknownElement would throw a false positive. menuitem is included in voidElementsTags array.

My idea was that since all items in voidElementTags are known elements it would be safe to check if the to be rendered element is in that array or not - because that array is guaranteed to contain only valid elements.

Maybe i can add a descriptive comment about that or do you have any other solution in mind on how to handle menuitem ?

Intuitively you either have a whitelist of all the currently known elements and the warning is then used to raise awareness that you have made a typo. Or you don't have a whitelist and the warning is used to indicate that you're rendering an element that isn't understood by the current browser.

There are many tags not supported by every browser, whitelisting just menuitem (and self-closing tags) seems like an arbitrary in-between. IMHO you should pick one or the other (@aweary?).

ok - totally agree - which means while using menuitem in chrome we should get the error message as expected. Fine but for that i will have to make some changes in the unit tests concerning menuitem to make them pass with this implementation.

syranide · 2017-03-16T13:17:01Z

I'm a little curious about the performance implications of this, there probably needs to be caching as-is. This PR effectively causes every element to be created twice, which makes me wonder why it doesn't just check the actual element el already created inside the same function just above?

arshabh · 2017-03-16T14:06:31Z

Yes i agree - and understood now what you were saying about using the el. But i will check that only in the condition when there is html namespace - as svg elements cannot be tested

aweary · 2017-03-27T16:51:18Z

src/renderers/dom/stack/client/ReactDOMComponent.js

@@ -583,6 +584,20 @@ ReactDOMComponent.Mixin = {
        );
        didWarnShadyDOM = true;
      }
+
+      if (__DEV__) {
+        if (this._namespaceURI === DOMNamespaces.html) {


I'm not sure we should add the warning if it's not consistent. Users could still mess up inside an svg tag and they wouldn't get any warnings, which is extra confusing since they would get warnings anywhere else. @spicyj what do you think?

I see @gaearon did mention previously he'd be OK with that

I'm also okay with that, thanks for checking!

Since menuitem is treated as an unknown element in jsdom it triggers the unknown element warning.

aweary · 2017-04-09T21:10:16Z

@spicyj @arshabh I went ahead and made a couple changes to get this going:

Fix a spacing issue in the warning
Reduce the number of times we call isCustomComponent
Add the warning to Fiber
"Fix" a test that uses menuitem, which is considered an instance of HTMLUnkownElement

@aickin this breaks a few of your SSR integration tests related to non-standard elements. I wasn't sure how to address this with the abstractions in ReactDOMServerIntegrations so hopefully we can look at that together.

gaearon · 2017-04-14T00:44:40Z

src/renderers/dom/fiber/ReactDOMFiberComponent.js

+      if (namespaceURI === HTML_NAMESPACE) {
+        warning(
+          isCustomComponentTag ||
+            !(domElement instanceof window.HTMLUnknownElement),


Can we avoid instanceof checks here? They don't work across iframes.

It is more resilient.

outdated review

gaearon · 2017-04-14T01:13:56Z

I think this should be ready now, and server related issues seem gone. Let's wait for CI to pass and merge?

facebook-github-bot added the CLA Signed label Mar 12, 2017

syranide reviewed Mar 15, 2017

View reviewed changes

aweary previously requested changes Mar 16, 2017

View reviewed changes

syranide reviewed Mar 16, 2017

View reviewed changes

aweary self-assigned this Mar 24, 2017

aweary reviewed Mar 27, 2017

View reviewed changes

arshabh added 7 commits April 9, 2017 14:51

Add warning if rendering and HTMLUnknownElement

b45105c

Records fiber tests

29f816a

Fixes linting and server render tests

f8310b9

Incorporates review comments

a0296ee

Uses ownerDocument instead of document

8cd9a87

uses el instead of creating a new element

c428eee

Removes warning check of voidElementTags

f923d47

aweary force-pushed the issue-3726 branch 2 times, most recently from 7910c19 to 0ee4ddc Compare April 9, 2017 20:09

Add missing space to unknown element warning

468ac42

aweary force-pushed the issue-3726 branch from 0ee4ddc to 468ac42 Compare April 9, 2017 20:10

Only call isCustomComponent once

c8665e8

aweary force-pushed the issue-3726 branch from b6545da to c8665e8 Compare April 9, 2017 20:37

Brandon Dail added 2 commits April 9, 2017 16:05

Spy on console in menuitem test

c9a0a2d

Since menuitem is treated as an unknown element in jsdom it triggers the unknown element warning.

Add unknown element warning to Fiber

e7e3812

gaearon mentioned this pull request Apr 14, 2017

Fix #3596: Add warning for components not following proper case convention #7089

Closed

gaearon reviewed Apr 14, 2017

View reviewed changes

gaearon added 2 commits April 14, 2017 02:09

Replace instanceof with toString check

55110f4

It is more resilient.

Record tests

96af2b1

aweary approved these changes Apr 14, 2017

View reviewed changes

aweary merged commit f54fdd5 into facebook:master Apr 14, 2017

paulmanta mentioned this pull request May 7, 2017

Disable ESLint's no-extend-native for two lines #9625

Merged

ConnorAtherton mentioned this pull request Jul 29, 2017

Warn if rendering invalid element, fixes #3726 #8396

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add warning if rendering and HTMLUnknownElement #9163

Add warning if rendering and HTMLUnknownElement #9163

arshabh commented Mar 12, 2017 •

edited

Loading

syranide Mar 15, 2017

syranide Mar 15, 2017

syranide Mar 15, 2017

aweary Mar 15, 2017

arshabh Mar 16, 2017

aweary Mar 16, 2017

arshabh Mar 16, 2017 •

edited

Loading

arshabh Mar 16, 2017

arshabh Mar 24, 2017

aweary commented Mar 16, 2017

arshabh commented Mar 16, 2017

syranide Mar 16, 2017

arshabh Mar 16, 2017 •

edited

Loading

syranide Mar 16, 2017

arshabh Mar 16, 2017

syranide commented Mar 16, 2017

arshabh commented Mar 16, 2017 •

edited

Loading

aweary Mar 27, 2017

aweary Mar 27, 2017

sophiebits Mar 27, 2017

aweary commented Apr 9, 2017

gaearon Apr 14, 2017

gaearon commented Apr 14, 2017

Add warning if rendering and HTMLUnknownElement #9163

Add warning if rendering and HTMLUnknownElement #9163

Conversation

arshabh commented Mar 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arshabh Mar 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aweary commented Mar 16, 2017

arshabh commented Mar 16, 2017

Choose a reason for hiding this comment

arshabh Mar 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

syranide commented Mar 16, 2017

arshabh commented Mar 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aweary commented Apr 9, 2017

Choose a reason for hiding this comment

gaearon commented Apr 14, 2017

arshabh commented Mar 12, 2017 •

edited

Loading

arshabh Mar 16, 2017 •

edited

Loading

arshabh Mar 16, 2017 •

edited

Loading

arshabh commented Mar 16, 2017 •

edited

Loading