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

Custom Attributes Scenario 2: Write badly cased attributes. Remove most of whitelist. #10385

Merged
merged 49 commits into from Aug 15, 2017

Conversation

Projects
None yet
10 participants
@nhunzaker
Collaborator

nhunzaker commented Aug 4, 2017

Follow-up work from #7311. This PR implements scenario 2 for custom attributes, as described in #7311 (comment).

  1. Badly cased attribute names still write to the DOM.
  2. Cases like classname, htmlfor, and arabicform, do not alias. They write as classname, htmlfor, and arabicform.
  3. All attribute whitelist entries that enforce a specific casing have been removed.
  4. The ARIA whitelist has been moved to a developer-only enforcement (but the logic is the same as master)

Build size:

react-dom.production.min.js: 109.01kb (34.40kb gz)

screen shot 2017-08-07 at 10 33 16 am

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Aug 4, 2017

Member

:O

Member

gaearon commented Aug 4, 2017

:O

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Aug 4, 2017

Member

How did you verify the changes? Is it worth creating a test page with all these attributes to ensure their support didn’t regress? (At least once.)

Member

gaearon commented Aug 4, 2017

How did you verify the changes? Is it worth creating a test page with all these attributes to ensure their support didn’t regress? (At least once.)

@nhunzaker

This comment has been minimized.

Show comment
Hide comment
@nhunzaker

nhunzaker Aug 4, 2017

Collaborator

My testing could be more exhaustive. I'll make a test page.

Collaborator

nhunzaker commented Aug 4, 2017

My testing could be more exhaustive. I'll make a test page.

srcSet: 0,
start: HAS_NUMERIC_VALUE,
step: 0,
// Style must be explicitly set in the attribute list. React components

This comment has been minimized.

@gaearon

gaearon Aug 4, 2017

Member

Is this a note about server renderer? I imagine we don't get into style branch on the client because there’s a hardcoded one earlier.

What about custom elements? Does style still work correctly on those? (And did it at all in the past?)

@gaearon

gaearon Aug 4, 2017

Member

Is this a note about server renderer? I imagine we don't get into style branch on the client because there’s a hardcoded one earlier.

What about custom elements? Does style still work correctly on those? (And did it at all in the past?)

This comment has been minimized.

@nhunzaker

nhunzaker Aug 4, 2017

Collaborator

Excellent questions. I will verify.

@nhunzaker

nhunzaker Aug 4, 2017

Collaborator

Excellent questions. I will verify.

This comment has been minimized.

@nhunzaker

nhunzaker Aug 4, 2017

Collaborator

I imagine we don't get into style branch on the client because there’s a hardcoded one earlier.

We don't get to the style branch, but we validate all properties here:
https://github.com/facebook/react/blob/master/src/renderers/dom/fiber/ReactDOMFiberComponent.js#L406

The UnknownPropertyHook calls DOMProperty.shouldSetAttribute and sees that the style property is using an object, so it warns. This check doesn't happen if the style property is in the attribute config.

So keeping style: 0 here prevents the warning.

What about custom elements? Does style still work correctly on those? (And did it at all in the past?)

What is the intended behavior of passing style to custom elements? Should it pass the value "as is", or provide style pre-processing? Right now it looks like it's sending down a style string.

@nhunzaker

nhunzaker Aug 4, 2017

Collaborator

I imagine we don't get into style branch on the client because there’s a hardcoded one earlier.

We don't get to the style branch, but we validate all properties here:
https://github.com/facebook/react/blob/master/src/renderers/dom/fiber/ReactDOMFiberComponent.js#L406

The UnknownPropertyHook calls DOMProperty.shouldSetAttribute and sees that the style property is using an object, so it warns. This check doesn't happen if the style property is in the attribute config.

So keeping style: 0 here prevents the warning.

What about custom elements? Does style still work correctly on those? (And did it at all in the past?)

What is the intended behavior of passing style to custom elements? Should it pass the value "as is", or provide style pre-processing? Right now it looks like it's sending down a style string.

This comment has been minimized.

@gaearon

gaearon Aug 4, 2017

Member

I think for now we're okay with doing whatever we used to be doing.

@gaearon

gaearon Aug 4, 2017

Member

I think for now we're okay with doing whatever we used to be doing.

@nhunzaker nhunzaker changed the title from Remove remaining entries in whitelist that are only needed for developer warnings to Custom Attributes Scenario 2 Aug 7, 2017

@nhunzaker nhunzaker changed the title from Custom Attributes Scenario 2 to Custom Attributes Scenario 1: Write badly cased attributes. Remove most of whitelist. Aug 7, 2017

@nhunzaker nhunzaker changed the title from Custom Attributes Scenario 1: Write badly cased attributes. Remove most of whitelist. to Custom Attributes Scenario 2: Write badly cased attributes. Remove most of whitelist. Aug 7, 2017

}
if (DOMAttributeNamespaces.hasOwnProperty(propName)) {
propertyInfo.attributeNamespace = DOMAttributeNamespaces[propName];
}
if (DOMPropertyNames.hasOwnProperty(propName)) {
propertyInfo.propertyName = DOMPropertyNames[propName];
}

This comment has been minimized.

@nhunzaker

nhunzaker Aug 7, 2017

Collaborator

DOMPropertyNames is not used by any injection.

@nhunzaker

nhunzaker Aug 7, 2017

Collaborator

DOMPropertyNames is not used by any injection.

@nhunzaker nhunzaker changed the base branch from nh-strip-whitelist to master Aug 7, 2017

@gaearon

This is amazing work.

There's a (hopefully!) last change in semantics we need to make that I wrote about in #10399 (comment).

Thanks again for working through this! I really hope this is the last change we need to make. If not, I’ll try to address the rest myself tomorrow.

additive: 0,
alignmentBaseline: 'alignment-baseline',
allowReorder: 'allowReorder',
alphabetic: 0,

This comment has been minimized.

@nhunzaker

nhunzaker Aug 7, 2017

Collaborator

I can't figure out if this is a boolean attribute or not.

@nhunzaker

nhunzaker Aug 7, 2017

Collaborator

I can't figure out if this is a boolean attribute or not.

This comment has been minimized.

This comment has been minimized.

@nhunzaker

nhunzaker Aug 7, 2017

Collaborator

I just discovered this page. True life saver.

@nhunzaker

nhunzaker Aug 7, 2017

Collaborator

I just discovered this page. True life saver.

ascent: 0,
attributeName: 'attributeName',
attributeType: 'attributeType',
autoReverse: 'autoReverse',

This comment has been minimized.

@nhunzaker

nhunzaker Aug 7, 2017

Collaborator

This one either.

@nhunzaker

nhunzaker Aug 7, 2017

Collaborator

This one either.

This comment has been minimized.

This comment has been minimized.

@nhunzaker

nhunzaker Aug 8, 2017

Collaborator

This is not in the spec.

@nhunzaker

nhunzaker Aug 8, 2017

Collaborator

This is not in the spec.

This comment has been minimized.

@nhunzaker

nhunzaker Aug 8, 2017

Collaborator

Oooh fun.

@nhunzaker

nhunzaker Aug 8, 2017

Collaborator

Oooh fun.

hanging: 0,
horizAdvX: 'horiz-adv-x',
horizOriginX: 'horiz-origin-x',
ideographic: 0,

This comment has been minimized.

@nhunzaker

nhunzaker Aug 7, 2017

Collaborator

This is a value for dominantBaseline, and I can't find a description of it anywhere.

@nhunzaker

nhunzaker Aug 7, 2017

Collaborator

This is a value for dominantBaseline, and I can't find a description of it anywhere.

This comment has been minimized.

@nhunzaker

nhunzaker Aug 7, 2017

Collaborator

Number.
https://www.w3.org/TR/SVG/fonts.html#FontFaceElementIdeographicAttribute

Jeez Nate. Why don't you read the spec?

@nhunzaker

nhunzaker Aug 7, 2017

Collaborator

Number.
https://www.w3.org/TR/SVG/fonts.html#FontFaceElementIdeographicAttribute

Jeez Nate. Why don't you read the spec?

This comment has been minimized.

This comment has been minimized.

@gaearon

gaearon Aug 8, 2017

Member

Haha :-) It's a bit tough to find.

@gaearon

gaearon Aug 8, 2017

Member

Haha :-) It's a bit tough to find.

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Aug 8, 2017

Member

We'll really need some automated way to test we didn't regress here (just once). I'm thinking maybe we could take all existing whitelists, make a page that renders all these properties with React 15, and then verify these values are still there with 16. I'll try to do it tomorrow.

Member

gaearon commented Aug 8, 2017

We'll really need some automated way to test we didn't regress here (just once). I'm thinking maybe we could take all existing whitelists, make a page that renders all these properties with React 15, and then verify these values are still there with 16. I'll try to do it tomorrow.

@nhunzaker

This comment has been minimized.

Show comment
Hide comment
@nhunzaker

nhunzaker Aug 8, 2017

Collaborator

@gaearon Okay. This is not in a fully tested, and possibly not in a working state, but I've pushed up commits that add back what I believe are all the boolean properties. In addition to the ones you identified, I went through the entire SVG spec (okay okay I cmd + f'ed through it) and the only boolean SVG attributes I could find were:

  • autoReverse
  • externalResourcesRequired
  • preserveAlpha

autoReverse is technically from SMIL. I'll browse through and see if we aren't missing anything.

Collaborator

nhunzaker commented Aug 8, 2017

@gaearon Okay. This is not in a fully tested, and possibly not in a working state, but I've pushed up commits that add back what I believe are all the boolean properties. In addition to the ones you identified, I went through the entire SVG spec (okay okay I cmd + f'ed through it) and the only boolean SVG attributes I could find were:

  • autoReverse
  • externalResourcesRequired
  • preserveAlpha

autoReverse is technically from SMIL. I'll browse through and see if we aren't missing anything.

@nhunzaker

This comment has been minimized.

Show comment
Hide comment
@nhunzaker

nhunzaker Aug 8, 2017

Collaborator

SMIL state module defines a few more:
https://www.w3.org/TR/smil/smil-DTD.html

  • 'skipContent'
  • 'external'
  • 'syncMaster'

I think that's it.

Collaborator

nhunzaker commented Aug 8, 2017

SMIL state module defines a few more:
https://www.w3.org/TR/smil/smil-DTD.html

  • 'skipContent'
  • 'external'
  • 'syncMaster'

I think that's it.

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Aug 8, 2017

Member

If we didn't support them explicitly before I'm fine with skipping them now.

Member

gaearon commented Aug 8, 2017

If we didn't support them explicitly before I'm fine with skipping them now.

@nhunzaker

This comment has been minimized.

Show comment
Hide comment
@nhunzaker

nhunzaker Aug 8, 2017

Collaborator
  1. Upstreamed with master
  2. Adds warning about camel cased aria attributes. Also recommends using the real attribute when possible ariaHasPopup -> aria-haspopup
  3. Data and ARIA attributes accept booleans
  4. NaN raises a warning. Though only on unknown attributes.
Collaborator

nhunzaker commented Aug 8, 2017

  1. Upstreamed with master
  2. Adds warning about camel cased aria attributes. Also recommends using the real attribute when possible ariaHasPopup -> aria-haspopup
  3. Data and ARIA attributes accept booleans
  4. NaN raises a warning. Though only on unknown attributes.

@gaearon gaearon dismissed their stale review Aug 8, 2017

old

autofocus: true,
defaultvalue: true,
defaultchecked: true,
innerhtml: true,

This comment has been minimized.

@gaearon

gaearon Aug 8, 2017

Member

This is slightly confusing because if I use it, it says:

Warning: Invalid DOM property innerhtml. Did you mean innerHTML?

But then if I try innerHTML, of course it isn’t supported:

Warning: Directly setting property innerHTML is not permitted. For more information, lookup documentation on dangerouslySetInnerHTML.

It seems better if the same warning was displayed immediately (or if we just got a generic "unknown property" warning for wrong casing like innerhtml).

@gaearon

gaearon Aug 8, 2017

Member

This is slightly confusing because if I use it, it says:

Warning: Invalid DOM property innerhtml. Did you mean innerHTML?

But then if I try innerHTML, of course it isn’t supported:

Warning: Directly setting property innerHTML is not permitted. For more information, lookup documentation on dangerouslySetInnerHTML.

It seems better if the same warning was displayed immediately (or if we just got a generic "unknown property" warning for wrong casing like innerhtml).

This comment has been minimized.

@nhunzaker

nhunzaker Aug 8, 2017

Collaborator

I'm working on making the warning for onFocusIn and onFocusOut case insensitive. I can do that for innerHTML too.

These show up in assertValidProps:
https://github.com/facebook/react/blob/master/src/renderers/dom/shared/utils/assertValidProps.js

The easiest thing to do is move them to ReactDOMUnknownPropertyHook.

@nhunzaker

nhunzaker Aug 8, 2017

Collaborator

I'm working on making the warning for onFocusIn and onFocusOut case insensitive. I can do that for innerHTML too.

These show up in assertValidProps:
https://github.com/facebook/react/blob/master/src/renderers/dom/shared/utils/assertValidProps.js

The easiest thing to do is move them to ReactDOMUnknownPropertyHook.

innerhtml: true,
suppresscontenteditablewarning: true,
onfocusin: true,
onfocusout: true,

This comment has been minimized.

@gaearon

gaearon Aug 8, 2017

Member

These two are currently not validated at all. If I type

<div onfocusin={function() {}} />

it will silently ignore it. I would expect it to warn.

@gaearon

gaearon Aug 8, 2017

Member

These two are currently not validated at all. If I type

<div onfocusin={function() {}} />

it will silently ignore it. I would expect it to warn.

This comment has been minimized.

@nhunzaker

nhunzaker Aug 8, 2017

Collaborator

Good catch. I these from possibleStandardNames to prevent the UnknownDOMPropertyHook from picking it up in addition to the dev warning about onfocusin.

I think we just need to make the onFocusIn warning case insensitive. What do you think?

@nhunzaker

nhunzaker Aug 8, 2017

Collaborator

Good catch. I these from possibleStandardNames to prevent the UnknownDOMPropertyHook from picking it up in addition to the dev warning about onfocusin.

I think we just need to make the onFocusIn warning case insensitive. What do you think?

This comment has been minimized.

@gaearon

gaearon Aug 8, 2017

Member

Works for me.

@gaearon

gaearon Aug 8, 2017

Member

Works for me.

This comment has been minimized.

@nhunzaker

nhunzaker Aug 8, 2017

Collaborator

I'll get this.

@nhunzaker

nhunzaker Aug 8, 2017

Collaborator

I'll get this.

if (DOMMutationMethods.hasOwnProperty(propName)) {
propertyInfo.mutationMethod = DOMMutationMethods[propName];
}
// Downcase references to whitelist properties to check for membership

This comment has been minimized.

@gaearon

gaearon Aug 8, 2017

Member

Where do these become lowercase? This comment confused me a bit because I expected to find toLowerCase() call somewhere close but it’s not there.

@gaearon

gaearon Aug 8, 2017

Member

Where do these become lowercase? This comment confused me a bit because I expected to find toLowerCase() call somewhere close but it’s not there.

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Aug 8, 2017

Member

It looks like the “bad type” mistakes are not caught if there was a previous valid value.
For example:

      ReactDOM.render(
        <h1 lol={4}>Hello World!</h1>,
        document.getElementById('container')
      );
      ReactDOM.render(
        <h1 lol={{}}>Hello World!</h1>,
        document.getElementById('container')
      );

In this case lol={{}} is ignored silently.

We should make sure we warn in such cases. This is a bug.

In fact this reproduces even with two different DOM containers:

      ReactDOM.render(
        <h1 lol={'asda'}>Hello World!</h1>,
        document.getElementById('container1')
      );
      ReactDOM.render(
        <h1 lol={{}}>Hello World!</h1>,
        document.getElementById('container2')
      );

So this is probably caused by some boolean “don’t validate anymore” flag that is set too early.

Member

gaearon commented Aug 8, 2017

It looks like the “bad type” mistakes are not caught if there was a previous valid value.
For example:

      ReactDOM.render(
        <h1 lol={4}>Hello World!</h1>,
        document.getElementById('container')
      );
      ReactDOM.render(
        <h1 lol={{}}>Hello World!</h1>,
        document.getElementById('container')
      );

In this case lol={{}} is ignored silently.

We should make sure we warn in such cases. This is a bug.

In fact this reproduces even with two different DOM containers:

      ReactDOM.render(
        <h1 lol={'asda'}>Hello World!</h1>,
        document.getElementById('container1')
      );
      ReactDOM.render(
        <h1 lol={{}}>Hello World!</h1>,
        document.getElementById('container2')
      );

So this is probably caused by some boolean “don’t validate anymore” flag that is set too early.

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Aug 8, 2017

Member

There is a breaking change here which I did not realize before.
This used to work in 15:

      var obj = {toString() {return 'lol'}}
      ReactDOM.render(
        <img src={obj} />,
        document.getElementById('container1')
      );

But it won’t work now because src goes through the “custom” code path.

Since the list of attributes that go through the “custom” code path is an implementation detail, it is highly confusing that this won’t work with src but will work with an attribute like arabicForm.

It would make sense to me if we prevented implicit toString calls for all attributes, and forced you to always pass primitives. But then we should change the logic to do it, and emit a warning in this case. I’d need to do a pass over FB codebase and check how often it fires, to see whether it is feasible to get this change in at this point or not.

Member

gaearon commented Aug 8, 2017

There is a breaking change here which I did not realize before.
This used to work in 15:

      var obj = {toString() {return 'lol'}}
      ReactDOM.render(
        <img src={obj} />,
        document.getElementById('container1')
      );

But it won’t work now because src goes through the “custom” code path.

Since the list of attributes that go through the “custom” code path is an implementation detail, it is highly confusing that this won’t work with src but will work with an attribute like arabicForm.

It would make sense to me if we prevented implicit toString calls for all attributes, and forced you to always pass primitives. But then we should change the logic to do it, and emit a warning in this case. I’d need to do a pass over FB codebase and check how often it fires, to see whether it is feasible to get this change in at this point or not.

@nhunzaker nhunzaker referenced this pull request Aug 8, 2017

Closed

Outstanding work for Custom Attributes PR. #10404

8 of 9 tasks complete
@nhunzaker

This comment has been minimized.

Show comment
Hide comment
@nhunzaker

nhunzaker Aug 8, 2017

Collaborator

@gaearon That's a really good point. Would it be sufficient to do a simple check like:

typeof value === 'object' && value.toString !== Object.prototype.toString

Is that too crude?

Collaborator

nhunzaker commented Aug 8, 2017

@gaearon That's a really good point. Would it be sufficient to do a simple check like:

typeof value === 'object' && value.toString !== Object.prototype.toString

Is that too crude?

@flarnie flarnie merged commit 2999811 into master Aug 15, 2017

1 check passed

ci/circleci Your tests passed on CircleCI!
Details
expectDev(console.error.calls.argsFor(0)[0]).toContain(
'Warning: Invalid ARIA attribute ariaHasPopup. ' +
'Did you mean aria-haspopup?',
);

This comment has been minimized.

@gaearon

gaearon Aug 15, 2017

Member

Minor nit: this warning doesn't use backticks but the one for DOM props does.

@gaearon

gaearon Aug 15, 2017

Member

Minor nit: this warning doesn't use backticks but the one for DOM props does.

});
itRenders(
'no className prop when given a badly cased alias',

This comment has been minimized.

@gaearon

gaearon Aug 15, 2017

Member

Need to change test title to match new semantics.

@gaearon

gaearon Aug 15, 2017

Member

Need to change test title to match new semantics.

expect(e.getAttribute('textLength')).toBe('10');
});
itRenders('no badly cased aliased SVG attribute alias', async render => {

This comment has been minimized.

@gaearon

gaearon Aug 15, 2017

Member

Same.

@gaearon

gaearon Aug 15, 2017

Member

Same.

});
itRenders(
'no badly cased original SVG attribute that is aliased',

This comment has been minimized.

@gaearon

gaearon Aug 15, 2017

Member

Same?

@gaearon

gaearon Aug 15, 2017

Member

Same?

@nhunzaker

This comment has been minimized.

Show comment
Hide comment
@nhunzaker

nhunzaker Aug 16, 2017

Collaborator

Filed an issue for the minification issue:
#10469

PR shortly for the feedback @gaearon sent over after merge.

Collaborator

nhunzaker commented Aug 16, 2017

Filed an issue for the minification issue:
#10469

PR shortly for the feedback @gaearon sent over after merge.

@pisrael

This comment has been minimized.

Show comment
Hide comment
@pisrael

pisrael Aug 25, 2017

Is there a work-around that can be used in the current production version?
I was able to solve for known HTML tags, but it doesn't work for custom tags such as 'amp-list'.

For known tags, this work:

import { DOMProperty } from 'react-dom/lib/ReactInjection'
DOMProperty.injectDOMPropertyConfig({
    Properties: {
        '[src]': DOMProperty.MUST_USE_PROPERTY
    }
})

But for custom tags, the react code blocks it through a validation of on DOMPropertyOperations at function isAttributeNameSafe(attributeName)

Is there any workaround for placing a amp-bind attribute '[src]' inside a 'amp-list' tag on server-side-rendering? The only way I'm considering now is to use dangerouslySetInnerHTML in a parent component.

pisrael commented Aug 25, 2017

Is there a work-around that can be used in the current production version?
I was able to solve for known HTML tags, but it doesn't work for custom tags such as 'amp-list'.

For known tags, this work:

import { DOMProperty } from 'react-dom/lib/ReactInjection'
DOMProperty.injectDOMPropertyConfig({
    Properties: {
        '[src]': DOMProperty.MUST_USE_PROPERTY
    }
})

But for custom tags, the react code blocks it through a validation of on DOMPropertyOperations at function isAttributeNameSafe(attributeName)

Is there any workaround for placing a amp-bind attribute '[src]' inside a 'amp-list' tag on server-side-rendering? The only way I'm considering now is to use dangerouslySetInnerHTML in a parent component.

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Aug 25, 2017

Member

@pisrael No, this is not supported in current version of React except for dangerouslySetInnerHTML. It's tracked in #10064.

Member

gaearon commented Aug 25, 2017

@pisrael No, this is not supported in current version of React except for dangerouslySetInnerHTML. It's tracked in #10064.

@bvaughn bvaughn referenced this pull request Sep 7, 2017

Closed

React 16 RC #10294

@gaearon gaearon deleted the nh-gut-whitelist branch Oct 6, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment