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

Allow custom attributes by default #7311

Closed
wants to merge 22 commits into
base: master
from

Conversation

Projects
None yet
@nhunzaker
Collaborator

nhunzaker commented Jul 20, 2016

This was closed in favor of #10385

This is a resubmission of pull request #6459. Just to recap...

The addition of the unsupported property warning (#6800) reminded my old PR (#6459) and I was curious about reopening it... So here we are.

This pull request configures DOMPropertyOperations to allow all attributes to pass through. It maintains the whitelist only for attributes that need special behavior, such as boolean props and name translations (like className).

While I was at it, I also eliminated the duplicated list of "reserved props" between ReactDOMUnknownPropertyDevtool and ReactDOMComponent through a new isReservedProp method on DOMProperty.

Related Issues:

@nhunzaker

This comment has been minimized.

Show comment
Hide comment
@nhunzaker

nhunzaker Jul 20, 2016

Collaborator

Oh interesting. Looks like npm test locally wasn't running the server-side tests. I'll take a look at this one tomorrow:

https://travis-ci.org/facebook/react/jobs/145983385#L392

Collaborator

nhunzaker commented Jul 20, 2016

Oh interesting. Looks like npm test locally wasn't running the server-side tests. I'll take a look at this one tomorrow:

https://travis-ci.org/facebook/react/jobs/145983385#L392

@nhunzaker

This comment has been minimized.

Show comment
Hide comment
@nhunzaker

nhunzaker Jul 20, 2016

Collaborator

Okay. I believe I've got this. I just had to downcase unrecognized properties during server rendering. Thoughts?

Collaborator

nhunzaker commented Jul 20, 2016

Okay. I believe I've got this. I just had to downcase unrecognized properties during server rendering. Thoughts?

@ghost ghost added CLA Signed labels Jul 20, 2016

@zpao zpao added Component: DOM and removed Component: DOM labels Aug 3, 2016

@ghost ghost added the CLA Signed label Aug 3, 2016

@ghost ghost added the CLA Signed label Aug 15, 2016

@ghost ghost added the CLA Signed label Sep 16, 2016

@nhunzaker

This comment has been minimized.

Show comment
Hide comment
@nhunzaker

nhunzaker Oct 24, 2016

Collaborator

I can't PR against another PR for the sake of review, but I've done the exact same thing for custom events:

nhunzaker#2

I think this might be all of the outstanding work for stripping away nearly all of the attribute whitelist.

Collaborator

nhunzaker commented Oct 24, 2016

I can't PR against another PR for the sake of review, but I've done the exact same thing for custom events:

nhunzaker#2

I think this might be all of the outstanding work for stripping away nearly all of the attribute whitelist.

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Oct 24, 2016

Member

AFAIK the biggest blocker on this is internal callsites. We need to find places in FB code that still cause "unknown prop" warnings and clean them up.

@spicyj Is that correct?

Member

gaearon commented Oct 24, 2016

AFAIK the biggest blocker on this is internal callsites. We need to find places in FB code that still cause "unknown prop" warnings and clean them up.

@spicyj Is that correct?

@gaearon gaearon added this to the 16.0 milestone Oct 24, 2016

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Oct 24, 2016

Member

Tagging just so we remember to go back to this and decide on it.

Member

gaearon commented Oct 24, 2016

Tagging just so we remember to go back to this and decide on it.

@nhunzaker

This comment has been minimized.

Show comment
Hide comment
@nhunzaker

nhunzaker Oct 24, 2016

Collaborator

Okay, awesome! Thanks for the update!

Collaborator

nhunzaker commented Oct 24, 2016

Okay, awesome! Thanks for the update!

@sophiebits

This comment has been minimized.

Show comment
Hide comment
@sophiebits

sophiebits Oct 30, 2016

Member

Yes, I think so.

Member

sophiebits commented Oct 30, 2016

Yes, I think so.

@nhunzaker

This comment has been minimized.

Show comment
Hide comment
@nhunzaker

nhunzaker Nov 1, 2016

Collaborator

Just stripped the ARIA list. Took the same strategy: move the props over to developer warnings.

Collaborator

nhunzaker commented Nov 1, 2016

Just stripped the ARIA list. Took the same strategy: move the props over to developer warnings.

@nhunzaker

This comment has been minimized.

Show comment
Hide comment
@nhunzaker

nhunzaker Nov 19, 2016

Collaborator

Rebased with master.

Just to be sure I did the rebase correctly: it looks like ReactDOMComponent::_updateDOMProperties is no longer responsible for removing event listeners? If so, that's really cool.

Collaborator

nhunzaker commented Nov 19, 2016

Rebased with master.

Just to be sure I did the rebase correctly: it looks like ReactDOMComponent::_updateDOMProperties is no longer responsible for removing event listeners? If so, that's really cool.

@nhunzaker

This comment has been minimized.

Show comment
Hide comment
@nhunzaker

nhunzaker Nov 20, 2016

Collaborator

Migrated over the required changes for Fiber.

Collaborator

nhunzaker commented Nov 20, 2016

Migrated over the required changes for Fiber.

@nhunzaker

This comment has been minimized.

Show comment
Hide comment
@nhunzaker

nhunzaker Apr 26, 2017

Collaborator

Took a pass modernizing this PR. I believe it accomplishes the following from #9477:

  • Introduce a new feature flag to ReactDOMFeatureFlags. Something like shouldSetCustomAttributes. Set it to true.
  • Keep the warning about unknown DOM props but only enable it if shouldSetCustomAttributes is false. Make sure tests still cover this case (you can override feature flag in tests—see existing tests concerning feature flags for how to do it).
  • Add new behavior of falling back to setAttribute for any unknown properties (rather than skipping them like we do now). Add tests for it. Those tests shouldn’t need to touch the feature flag (since it’s the new default behavior). Make sure this works both for SVG and HTML.
  • Make sure Fiber tests pass (when you create a PR, there are instructions on running them).
  • This might affect server rendering test suite previously added by @aickin. You might need to change those tests to verify the new behavior. It’s fine to only verify the new behavior there (with flag set to true) since we don’t use server rendering ourselves.
  • Good point from @syranide: #9477 (comment). We should still warn for known attributes that are miscapitalized. (It’s fine if that’s a different warning message than the one behind the flag.)

So the feature flag is left. I think that'll require changing how this PR works. I moved all of the attributes out of the property configs. The original motivation was to strip them out of the build, but it looks like I'll probably just have to move them back.

EDIT: Actually I think confused myself. I thought the feature flag would disallow the actual assignment of custom attributes. If it just warns, I only need to add the complete whitelist to the unknown property hook.

Collaborator

nhunzaker commented Apr 26, 2017

Took a pass modernizing this PR. I believe it accomplishes the following from #9477:

  • Introduce a new feature flag to ReactDOMFeatureFlags. Something like shouldSetCustomAttributes. Set it to true.
  • Keep the warning about unknown DOM props but only enable it if shouldSetCustomAttributes is false. Make sure tests still cover this case (you can override feature flag in tests—see existing tests concerning feature flags for how to do it).
  • Add new behavior of falling back to setAttribute for any unknown properties (rather than skipping them like we do now). Add tests for it. Those tests shouldn’t need to touch the feature flag (since it’s the new default behavior). Make sure this works both for SVG and HTML.
  • Make sure Fiber tests pass (when you create a PR, there are instructions on running them).
  • This might affect server rendering test suite previously added by @aickin. You might need to change those tests to verify the new behavior. It’s fine to only verify the new behavior there (with flag set to true) since we don’t use server rendering ourselves.
  • Good point from @syranide: #9477 (comment). We should still warn for known attributes that are miscapitalized. (It’s fine if that’s a different warning message than the one behind the flag.)

So the feature flag is left. I think that'll require changing how this PR works. I moved all of the attributes out of the property configs. The original motivation was to strip them out of the build, but it looks like I'll probably just have to move them back.

EDIT: Actually I think confused myself. I thought the feature flag would disallow the actual assignment of custom attributes. If it just warns, I only need to add the complete whitelist to the unknown property hook.

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Aug 6, 2017

Member

Sorry to bug you on weekend, but is there any chance you could write up a summary of possible solutions with pros and cons of each? It’s still a bit hard for me to understand all the constraints involved here.

The invariants I care about, in the order of importance:

  • There shouldn't be two ways to spell a known attribute for it to appear in the DOM. We should always enforce one canonical way for known attributes.
  • Code that doesn't warn on client side shouldn't suddenly cause mismatch warnings when you try to SSR it.
  • I want to be able to drop whitelist entries for SVGs where attribute name is the same in JSX and DOM, and I want to drop ARIA whitelist.

Which solutions satisfy them, and what are the tradeoffs of each?

Member

gaearon commented Aug 6, 2017

Sorry to bug you on weekend, but is there any chance you could write up a summary of possible solutions with pros and cons of each? It’s still a bit hard for me to understand all the constraints involved here.

The invariants I care about, in the order of importance:

  • There shouldn't be two ways to spell a known attribute for it to appear in the DOM. We should always enforce one canonical way for known attributes.
  • Code that doesn't warn on client side shouldn't suddenly cause mismatch warnings when you try to SSR it.
  • I want to be able to drop whitelist entries for SVGs where attribute name is the same in JSX and DOM, and I want to drop ARIA whitelist.

Which solutions satisfy them, and what are the tradeoffs of each?

@nhunzaker

This comment has been minimized.

Show comment
Hide comment
@nhunzaker

nhunzaker Aug 6, 2017

Collaborator

No problem! Thanks for sticking with me too!

To me, this boils down to whether or not we want to prevent attributes from being written on incorrect casing, or simply warn. From doing some research it looks like the browser doesn't care about case sensitivity. Enforcing casing is not protecting users from the DOM, it is protecting users from passing multiple properties within the same object, and deviating from community conventions.

Regardless of the approach, as long as we are okay with custom aria attributes showing up in the DOM, we can remove the ARIA whitelist and make it a developer-only warning.

So the approaches are: enforce a whitelist in production React or only as developer warnings.

1. Strictly enforce a whitelist in production React

We maintain a list of all "acceptable" attributes in React. When an attribute is missing from that list, only write it if it is all lower case. This means custom attributes, or attributes we don't know about, can only be lower case.

// Good
<div className="foobar" />
<div data-foobar="test" />
<svg><text textLength="100" /></svg>

// Bad
<div classname="foobar" /> <-- "class" will not show up
<svg><text textlength="100" /></svg> <-- textlength will not show up
<div data-fooBar="test" /> <-- data-fooBar *will* show up, but we could prevent this

Pros

  • It's the current behavior. We don't have to change much. This is the way this PR, at the time of writing (facfa87), works.
  • There is one canonical way of casing every attribute (that we know about)
  • Attributes we deem invalidly cased do not show up in the DOM (even though the DOM doesn't care about casing)

Cons

  • The whitelist can never go away. We can never remove entries like crossOrigin or textLength from the production build
  • We must mandate each attribute's casing, almost always conflicting with the HTML specification.. As new attributes are developed, or we miss them, we have to decide what the "nice" casing should be (crossorigin vs crossOrigin?)
  • Attributes not within the whitelist must be case insensitive. If we forget an SVG property, users must pass it as lower-case, even though the spec might describe the attribute name with a specific casing.

Other notes

  • React master does not handle casing for data attributes. If we go with this approach, we need to explore the ramifications of enforcing casing on data attributes.

2. Enforce certain attributes, like className and htmlFor, but otherwise only "enforce" attribute assignment through developer warnings

This approach would follow the whitelist approach in spirit. However it would be permissive to casing, and only enforce attribute casing through developer warnings. We could still prevent incorrect casing of certain attributes, like className and htmlFor, but overall enforcement would be less strict.

The DOM doesn't care about casing, but React needs it for proper diffing of properties. We could progressively track attribute assignment, providing a warning if the casing became inconsistent across multiple components. This could be a simple map we maintain as the UnknownDOMPropertyHook works.

// Good
<div className="foobar" />
<div data-foobar="test" />
<svg><text textLength="100" /></svg>

// Bad 
<div classname="foobar" /> <-- no class name (we'd keep className, which is too common)
<svg><text textlength="100" /></svg> <-- Ends up as textLength, but we'd still warn
<div data-fooBar="test" /> <-- shows up as data-foobar, but we could warn on this too

Pros

  • We can drastically remove entries from the whitelist (see #10385)
  • We do not have to worry about how to handle new attributes, unless they have namespaces like xlink:href
  • The overall approach is more flexible and doesn't get in the way of authors working with bleeding edge specs (where we now frequently recommend assigning attributes in a lifecycle hook).
  • We can avoid doing quite as much work on boot. The overhead of the property injection could be eliminated, or significantly reduced.

Cons

  • "Badly" cased attributes would show up in the DOM. Though again, we'd warn about this, and the browser doesn't care about the casing.

So with regards to your invariants:

There shouldn't be two ways to spell a known attribute for it to appear in the DOM. We should always enforce one canonical way for known attributes.

The whitelist approach wins if a developer warning is not sufficient enforcement.

Code that doesn't warn on client side shouldn't suddenly cause mismatch warnings when you try to SSR it.

This might not matter if we make server-side diffing case-insensitive, but I think both approaches can handle this assuming they utilize the same developer hooks.

I want to be able to drop whitelist entries for SVGs where attribute name is the same in JSX and DOM, and I want to drop ARIA whitelist.

The second approach wins here with aces. I can't think of a way to enforce casing without a whitelist.

Collaborator

nhunzaker commented Aug 6, 2017

No problem! Thanks for sticking with me too!

To me, this boils down to whether or not we want to prevent attributes from being written on incorrect casing, or simply warn. From doing some research it looks like the browser doesn't care about case sensitivity. Enforcing casing is not protecting users from the DOM, it is protecting users from passing multiple properties within the same object, and deviating from community conventions.

Regardless of the approach, as long as we are okay with custom aria attributes showing up in the DOM, we can remove the ARIA whitelist and make it a developer-only warning.

So the approaches are: enforce a whitelist in production React or only as developer warnings.

1. Strictly enforce a whitelist in production React

We maintain a list of all "acceptable" attributes in React. When an attribute is missing from that list, only write it if it is all lower case. This means custom attributes, or attributes we don't know about, can only be lower case.

// Good
<div className="foobar" />
<div data-foobar="test" />
<svg><text textLength="100" /></svg>

// Bad
<div classname="foobar" /> <-- "class" will not show up
<svg><text textlength="100" /></svg> <-- textlength will not show up
<div data-fooBar="test" /> <-- data-fooBar *will* show up, but we could prevent this

Pros

  • It's the current behavior. We don't have to change much. This is the way this PR, at the time of writing (facfa87), works.
  • There is one canonical way of casing every attribute (that we know about)
  • Attributes we deem invalidly cased do not show up in the DOM (even though the DOM doesn't care about casing)

Cons

  • The whitelist can never go away. We can never remove entries like crossOrigin or textLength from the production build
  • We must mandate each attribute's casing, almost always conflicting with the HTML specification.. As new attributes are developed, or we miss them, we have to decide what the "nice" casing should be (crossorigin vs crossOrigin?)
  • Attributes not within the whitelist must be case insensitive. If we forget an SVG property, users must pass it as lower-case, even though the spec might describe the attribute name with a specific casing.

Other notes

  • React master does not handle casing for data attributes. If we go with this approach, we need to explore the ramifications of enforcing casing on data attributes.

2. Enforce certain attributes, like className and htmlFor, but otherwise only "enforce" attribute assignment through developer warnings

This approach would follow the whitelist approach in spirit. However it would be permissive to casing, and only enforce attribute casing through developer warnings. We could still prevent incorrect casing of certain attributes, like className and htmlFor, but overall enforcement would be less strict.

The DOM doesn't care about casing, but React needs it for proper diffing of properties. We could progressively track attribute assignment, providing a warning if the casing became inconsistent across multiple components. This could be a simple map we maintain as the UnknownDOMPropertyHook works.

// Good
<div className="foobar" />
<div data-foobar="test" />
<svg><text textLength="100" /></svg>

// Bad 
<div classname="foobar" /> <-- no class name (we'd keep className, which is too common)
<svg><text textlength="100" /></svg> <-- Ends up as textLength, but we'd still warn
<div data-fooBar="test" /> <-- shows up as data-foobar, but we could warn on this too

Pros

  • We can drastically remove entries from the whitelist (see #10385)
  • We do not have to worry about how to handle new attributes, unless they have namespaces like xlink:href
  • The overall approach is more flexible and doesn't get in the way of authors working with bleeding edge specs (where we now frequently recommend assigning attributes in a lifecycle hook).
  • We can avoid doing quite as much work on boot. The overhead of the property injection could be eliminated, or significantly reduced.

Cons

  • "Badly" cased attributes would show up in the DOM. Though again, we'd warn about this, and the browser doesn't care about the casing.

So with regards to your invariants:

There shouldn't be two ways to spell a known attribute for it to appear in the DOM. We should always enforce one canonical way for known attributes.

The whitelist approach wins if a developer warning is not sufficient enforcement.

Code that doesn't warn on client side shouldn't suddenly cause mismatch warnings when you try to SSR it.

This might not matter if we make server-side diffing case-insensitive, but I think both approaches can handle this assuming they utilize the same developer hooks.

I want to be able to drop whitelist entries for SVGs where attribute name is the same in JSX and DOM, and I want to drop ARIA whitelist.

The second approach wins here with aces. I can't think of a way to enforce casing without a whitelist.

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Aug 6, 2017

Member

Thanks, this is helpful. Are there any differences in these two approaches concerning custom elements?

Member

gaearon commented Aug 6, 2017

Thanks, this is helpful. Are there any differences in these two approaches concerning custom elements?

@nhunzaker

This comment has been minimized.

Show comment
Hide comment
@nhunzaker

nhunzaker Aug 6, 2017

Collaborator

I don't believe there is any difference for custom elements. They get whatever attributes we send them, regardless of casing. For example:

https://github.com/facebook/react/pull/7311/files#diff-b9a8e245b3916a2e61eb0431d868a6a3R662

Collaborator

nhunzaker commented Aug 6, 2017

I don't believe there is any difference for custom elements. They get whatever attributes we send them, regardless of casing. For example:

https://github.com/facebook/react/pull/7311/files#diff-b9a8e245b3916a2e61eb0431d868a6a3R662

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Aug 6, 2017

Member

We could still prevent incorrect casing of certain attributes, like className and htmlFor, but overall enforcement would be less strict.

I have a few questions about this approach.

  1. How would you choose which ones to enforce? Is it not confusing that tabindex works but classname doesn’t?

  2. What would we do if you passed both versions for ones we only warn? For example <text alignmentBaseline="hanging" alignment-baseline="hanging" />. Would we use the last value?

  3. Does this mean adding support for a new attribute in a minor causes existing codebases to emit warnings? Since they might’ve been using an arbitrary casing but now we’re asking for a specific one. I guess it’s still better than it disappearing though (which is what would happen if we went with the whitelist approach but still pass custom attributes through).

Finally, does the current code in this PR represent the second approach fully?

Member

gaearon commented Aug 6, 2017

We could still prevent incorrect casing of certain attributes, like className and htmlFor, but overall enforcement would be less strict.

I have a few questions about this approach.

  1. How would you choose which ones to enforce? Is it not confusing that tabindex works but classname doesn’t?

  2. What would we do if you passed both versions for ones we only warn? For example <text alignmentBaseline="hanging" alignment-baseline="hanging" />. Would we use the last value?

  3. Does this mean adding support for a new attribute in a minor causes existing codebases to emit warnings? Since they might’ve been using an arbitrary casing but now we’re asking for a specific one. I guess it’s still better than it disappearing though (which is what would happen if we went with the whitelist approach but still pass custom attributes through).

Finally, does the current code in this PR represent the second approach fully?

@nhunzaker

This comment has been minimized.

Show comment
Hide comment
@nhunzaker

nhunzaker Aug 6, 2017

Collaborator

How would you choose which ones to enforce? Is it not confusing that tabindex works but classname doesn’t?

Slightly confusing yes. Would it be less confusing to allow mis-cased aliased attributes like alignmentbaseline, htmlfor and classname, but with the same warning? That part of the behavior would then be consistent.

What would we do if you passed both versions for ones we only warn? For example . Would we use the last value?

In this PR, alignment-baseline="hanging" is dropped, and the user would be asked to use alignmentBaseline. This would continue to be the case until we removed the alignmentBaseline alias. That's set up here:

https://github.com/facebook/react/pull/7311/files#diff-320a1076e356380287a6e67f7c894dbfR228

Does this mean adding support for a new attribute in a minor causes existing codebases to emit warnings? Still they might’ve been using an arbitrary casing but now we’re asking for a specific one. I guess it’s still better than it disappearing though (which is what would happen if we went with the whitelist approach but still pass custom attributes through).

Yes. We have to deal with this for both approaches unless we disallowed dashes. I would rather just stop adding aliases and start to talk about the feasibility of removing everything but htmlFor and className, which protect JS reserved words. We'd probably have to throw a few majors in front of it though.

Still, I think the community would adapt quickly. SVG properties are a good example. I am curious about how many users are passing things like dominantBaseline down the component tree without already spreading it into an SVG element, but that's just my opinion, we should poll the community.

Finally, does the current code in this PR represent the second approach fully?

I believe this PR represents the first approach. It would invalidate my other PR where the whitelist is significantly reduced (#10385)

Collaborator

nhunzaker commented Aug 6, 2017

How would you choose which ones to enforce? Is it not confusing that tabindex works but classname doesn’t?

Slightly confusing yes. Would it be less confusing to allow mis-cased aliased attributes like alignmentbaseline, htmlfor and classname, but with the same warning? That part of the behavior would then be consistent.

What would we do if you passed both versions for ones we only warn? For example . Would we use the last value?

In this PR, alignment-baseline="hanging" is dropped, and the user would be asked to use alignmentBaseline. This would continue to be the case until we removed the alignmentBaseline alias. That's set up here:

https://github.com/facebook/react/pull/7311/files#diff-320a1076e356380287a6e67f7c894dbfR228

Does this mean adding support for a new attribute in a minor causes existing codebases to emit warnings? Still they might’ve been using an arbitrary casing but now we’re asking for a specific one. I guess it’s still better than it disappearing though (which is what would happen if we went with the whitelist approach but still pass custom attributes through).

Yes. We have to deal with this for both approaches unless we disallowed dashes. I would rather just stop adding aliases and start to talk about the feasibility of removing everything but htmlFor and className, which protect JS reserved words. We'd probably have to throw a few majors in front of it though.

Still, I think the community would adapt quickly. SVG properties are a good example. I am curious about how many users are passing things like dominantBaseline down the component tree without already spreading it into an SVG element, but that's just my opinion, we should poll the community.

Finally, does the current code in this PR represent the second approach fully?

I believe this PR represents the first approach. It would invalidate my other PR where the whitelist is significantly reduced (#10385)

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Aug 6, 2017

Member

Would it be less confusing to allow mis-cased aliased attributes like alignmentbaseline, htmlfor and classname, but with the same warning? That part of the behavior would then be consistent.

If we go with the second approach, yes, that would make sense to me. From user’s point of view, there is no expected semantic difference between them.

I believe this PR represents the first approach. It would invalidate my other PR where the whitelist is significantly reduced (#10385)

I don’t fully understand the second sentence, and the relationship between this PR and #10385.

If this PR represents the first approach, does #10385 represent the second one? Is it meant to be applied on top of this PR, or as an alternative? Are both breaking changes? If we pick this PR now, does that make taking the other PR later harder?

We are coming close to the point where we can’t make breaking changes to 16, and from that point any massive changes will have to wait another 6 months or so. I’m trying to figure out which changes we can make right now. I see two distinct goals:

  • Enable passthrough for unknown attributes. (new feature, addressed both by 1st and 2nd scenarios)
  • Get rid of some of the whitelist. (drop weight, addressed only by 2nd scenario)

Let’s imagine two worlds. In one world we want to go ahead with 1st scenario. In other world we want to go ahead with 2nd scenario. Which PRs, in which order, would we need to take before releasing 16, in these worlds? Note that for 2nd scenario, the weight loss doesn’t have to be immediate (can happen anytime during 16.x cycle) as long as the pieces that make it a minor rather than major change are already in place.

Member

gaearon commented Aug 6, 2017

Would it be less confusing to allow mis-cased aliased attributes like alignmentbaseline, htmlfor and classname, but with the same warning? That part of the behavior would then be consistent.

If we go with the second approach, yes, that would make sense to me. From user’s point of view, there is no expected semantic difference between them.

I believe this PR represents the first approach. It would invalidate my other PR where the whitelist is significantly reduced (#10385)

I don’t fully understand the second sentence, and the relationship between this PR and #10385.

If this PR represents the first approach, does #10385 represent the second one? Is it meant to be applied on top of this PR, or as an alternative? Are both breaking changes? If we pick this PR now, does that make taking the other PR later harder?

We are coming close to the point where we can’t make breaking changes to 16, and from that point any massive changes will have to wait another 6 months or so. I’m trying to figure out which changes we can make right now. I see two distinct goals:

  • Enable passthrough for unknown attributes. (new feature, addressed both by 1st and 2nd scenarios)
  • Get rid of some of the whitelist. (drop weight, addressed only by 2nd scenario)

Let’s imagine two worlds. In one world we want to go ahead with 1st scenario. In other world we want to go ahead with 2nd scenario. Which PRs, in which order, would we need to take before releasing 16, in these worlds? Note that for 2nd scenario, the weight loss doesn’t have to be immediate (can happen anytime during 16.x cycle) as long as the pieces that make it a minor rather than major change are already in place.

@nhunzaker

This comment has been minimized.

Show comment
Hide comment
@nhunzaker

nhunzaker Aug 6, 2017

Collaborator

I don’t fully understand the second sentence, and the relationship between this PR and #10385.

#10385 removes most of the whitelist. Examples include tabIndex, autoComplete, and crossOrigin. This depends on allowing uppercase letters in unrecognized attributes and allowing incorrectly cased attributes to show up in the DOM. Otherwise, if we do not allow uppercase characters in unrecognized attributes (approach no. 1), they need to be added back in #10385 because React will not write bad casings, so it needs to know the correct casing in production.

I apologize for being confusing, I meant to say that it is a next step if we go with approach no. 2.

If this PR represents the first approach, does #10385 represent the second one? Is it meant to be applied on top of this PR, or as an alternative?

Until I added the validation to prevent uppercased custom attributes, it was meant to be applied on top, based on prior conversation (#7311 (comment)).

Let’s imagine two worlds. In one world we want to go ahead with 1st scenario. In other world we want to go ahead with 2nd scenario.

Scenario 1

  1. We merge this PR.
  2. Removing attribute whitelist entries like tabIndex is a breaking change because React does not allow badly cased attributes, and we prevent uppercase characters in unrecognized attributes.
  3. Entries like tabIndex go back in the whitelist in #10385. #10385 would only remove entries in the whitelist that were lowercase, and we could talk about what to do about the ARIA whitelist.
  4. We could merge #10385 at a later time, but removing the ARIA whitelist would be a breaking change unless we get it into 16.0.0

Scenario 2

  1. We allow uppercase letters in custom attributes in this PR (remove https://github.com/facebook/react/pull/7311/files#diff-320a1076e356380287a6e67f7c894dbfR241)
  2. We ensure classname and htmlfor are written to the DOM, but class and for aren't. This needs to also cover server-side rendering.
  3. #10385 can be merged at a later time, though we would want to get the ARIA hook changes in during the 16.0.0 release. Removing the ARIA whitelist is a breaking change.
Collaborator

nhunzaker commented Aug 6, 2017

I don’t fully understand the second sentence, and the relationship between this PR and #10385.

#10385 removes most of the whitelist. Examples include tabIndex, autoComplete, and crossOrigin. This depends on allowing uppercase letters in unrecognized attributes and allowing incorrectly cased attributes to show up in the DOM. Otherwise, if we do not allow uppercase characters in unrecognized attributes (approach no. 1), they need to be added back in #10385 because React will not write bad casings, so it needs to know the correct casing in production.

I apologize for being confusing, I meant to say that it is a next step if we go with approach no. 2.

If this PR represents the first approach, does #10385 represent the second one? Is it meant to be applied on top of this PR, or as an alternative?

Until I added the validation to prevent uppercased custom attributes, it was meant to be applied on top, based on prior conversation (#7311 (comment)).

Let’s imagine two worlds. In one world we want to go ahead with 1st scenario. In other world we want to go ahead with 2nd scenario.

Scenario 1

  1. We merge this PR.
  2. Removing attribute whitelist entries like tabIndex is a breaking change because React does not allow badly cased attributes, and we prevent uppercase characters in unrecognized attributes.
  3. Entries like tabIndex go back in the whitelist in #10385. #10385 would only remove entries in the whitelist that were lowercase, and we could talk about what to do about the ARIA whitelist.
  4. We could merge #10385 at a later time, but removing the ARIA whitelist would be a breaking change unless we get it into 16.0.0

Scenario 2

  1. We allow uppercase letters in custom attributes in this PR (remove https://github.com/facebook/react/pull/7311/files#diff-320a1076e356380287a6e67f7c894dbfR241)
  2. We ensure classname and htmlfor are written to the DOM, but class and for aren't. This needs to also cover server-side rendering.
  3. #10385 can be merged at a later time, though we would want to get the ARIA hook changes in during the 16.0.0 release. Removing the ARIA whitelist is a breaking change.
@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Aug 6, 2017

Member

we prevent uppercase characters in unrecognized attributes

Can you remind me why this is necessary with approach (1)?

and we could talk about what to do about the ARIA whitelist

Can you remind me what makes ARIA attributes special (rather than automatically becoming “supported” as if they were custom properties if we remove them from whitelist)?

Member

gaearon commented Aug 6, 2017

we prevent uppercase characters in unrecognized attributes

Can you remind me why this is necessary with approach (1)?

and we could talk about what to do about the ARIA whitelist

Can you remind me what makes ARIA attributes special (rather than automatically becoming “supported” as if they were custom properties if we remove them from whitelist)?

@nhunzaker

This comment has been minimized.

Show comment
Hide comment
@nhunzaker

nhunzaker Aug 7, 2017

Collaborator

Can you remind me why this is necessary with approach (1)?

😨. I don't remember. Jeez. I might have taken "maybe we should also force people to specify custom attributes as lowercase?" too far from #7311 (review) (the reply #7311 (comment)).

Thinking on it again. I don't have a reason why we couldn't allow case sensitive custom attributes. I wish I could recall why I thought this. 😨. We just need to be sure something like capheight gets blocked, when it should turn capHeight into cap-height. Likewise for classname -> class.

So then the big difference between approach 1 and 2 is if we allow badly cased attributes to be written to the page. If we allow it, we can remove most of the attribute whitelist. If we don't, we have to keep it.

Can you remind me what makes ARIA attributes special (rather than automatically becoming “supported” as if they were custom properties if we remove them from whitelist)?

We just need to figure out what to do with the ARIA dev hook. This prevents assignment of "invalid" ARIA attributes via a white list. So if we allow custom attributes, that means custom ARIA attributes too. This whitelist is dead weight.

Collaborator

nhunzaker commented Aug 7, 2017

Can you remind me why this is necessary with approach (1)?

😨. I don't remember. Jeez. I might have taken "maybe we should also force people to specify custom attributes as lowercase?" too far from #7311 (review) (the reply #7311 (comment)).

Thinking on it again. I don't have a reason why we couldn't allow case sensitive custom attributes. I wish I could recall why I thought this. 😨. We just need to be sure something like capheight gets blocked, when it should turn capHeight into cap-height. Likewise for classname -> class.

So then the big difference between approach 1 and 2 is if we allow badly cased attributes to be written to the page. If we allow it, we can remove most of the attribute whitelist. If we don't, we have to keep it.

Can you remind me what makes ARIA attributes special (rather than automatically becoming “supported” as if they were custom properties if we remove them from whitelist)?

We just need to figure out what to do with the ARIA dev hook. This prevents assignment of "invalid" ARIA attributes via a white list. So if we allow custom attributes, that means custom ARIA attributes too. This whitelist is dead weight.

@nhunzaker

This comment has been minimized.

Show comment
Hide comment
@nhunzaker

nhunzaker Aug 7, 2017

Collaborator

If we allow custom attributes with uppercase letters, we just need to make sure the SSR diffing process is case-insensitive. Otherwise they will report as all lower-case, but the property will have upper case letters, so a mismatch will fire.

Collaborator

nhunzaker commented Aug 7, 2017

If we allow custom attributes with uppercase letters, we just need to make sure the SSR diffing process is case-insensitive. Otherwise they will report as all lower-case, but the property will have upper case letters, so a mismatch will fire.

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Aug 7, 2017

Member

So then the big difference between approach 1 and 2 is if we allow badly cased attributes to be written to the page. If we allow it, we can remove most of the attribute whitelist. If we don't, we have to keep it.

Can you reorganize two PRs so that this one implements scenario (1), and the second one is purely additive on top (it’s fine to duplicate same commits) and implements scenario (2)? Then it would be easier to pick and decide which one gets in.

Member

gaearon commented Aug 7, 2017

So then the big difference between approach 1 and 2 is if we allow badly cased attributes to be written to the page. If we allow it, we can remove most of the attribute whitelist. If we don't, we have to keep it.

Can you reorganize two PRs so that this one implements scenario (1), and the second one is purely additive on top (it’s fine to duplicate same commits) and implements scenario (2)? Then it would be easier to pick and decide which one gets in.

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Aug 7, 2017

Member

We just need to figure out what to do with the ARIA dev hook. This prevents assignment of "invalid" ARIA attributes via a white list. So if we allow custom attributes, that means custom ARIA attributes too. This whitelist is dead weight.

I’m fine with refactoring the hooks in any way. E.g. if merging them would solve the problem. Or if ARIA hook is no longer necessary.

Member

gaearon commented Aug 7, 2017

We just need to figure out what to do with the ARIA dev hook. This prevents assignment of "invalid" ARIA attributes via a white list. So if we allow custom attributes, that means custom ARIA attributes too. This whitelist is dead weight.

I’m fine with refactoring the hooks in any way. E.g. if merging them would solve the problem. Or if ARIA hook is no longer necessary.

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Aug 7, 2017

Member

Based on discussion in #7744 I think it might still be a good idea to warn about anything starting with aria that’s not a valid ARIA attribute in DEV. Or maybe just for common misspellings for known ARIA props (e.g. specifically their camel case versions).

Member

gaearon commented Aug 7, 2017

Based on discussion in #7744 I think it might still be a good idea to warn about anything starting with aria that’s not a valid ARIA attribute in DEV. Or maybe just for common misspellings for known ARIA props (e.g. specifically their camel case versions).

@nhunzaker

This comment has been minimized.

Show comment
Hide comment
@nhunzaker

nhunzaker Aug 7, 2017

Collaborator

Cool. In 82e05e0 I've allowed uppercase letters for custom attributes. I also added some tests to cover all of the different name aliasing types across different casing.

Aside from the ARIA stuff, this covers scenario (1). I'll start on scenario (2).

Collaborator

nhunzaker commented Aug 7, 2017

Cool. In 82e05e0 I've allowed uppercase letters for custom attributes. I also added some tests to cover all of the different name aliasing types across different casing.

Aside from the ARIA stuff, this covers scenario (1). I'll start on scenario (2).

@nhunzaker

This comment has been minimized.

Show comment
Hide comment
@nhunzaker

nhunzaker Aug 7, 2017

Collaborator

Done, though I'm going to circle back tomorrow morning with fresh eyes. I'll also update the descriptions and titles.

Scenario 1:
#7311
(this PR)

Scenario 2:
https://github.com/facebook/react/pull/10385/files


Scenario two covers what I see as the necessary steps for moving over the ARIA dev warnings. I'll work on moving it over to the first PR (this one) tomorrow morning.

Collaborator

nhunzaker commented Aug 7, 2017

Done, though I'm going to circle back tomorrow morning with fresh eyes. I'll also update the descriptions and titles.

Scenario 1:
#7311
(this PR)

Scenario 2:
https://github.com/facebook/react/pull/10385/files


Scenario two covers what I see as the necessary steps for moving over the ARIA dev warnings. I'll work on moving it over to the first PR (this one) tomorrow morning.

@nhunzaker

This comment has been minimized.

Show comment
Hide comment
@nhunzaker

nhunzaker Aug 7, 2017

Collaborator

I'm going to send another PR for this branch with a description of scenario one to keep discussion focused to that specific implementation. Also this PR has like 150 comments. It might be easier to talk.

Collaborator

nhunzaker commented Aug 7, 2017

I'm going to send another PR for this branch with a description of scenario one to keep discussion focused to that specific implementation. Also this PR has like 150 comments. It might be easier to talk.

@nhunzaker

This comment has been minimized.

Show comment
Hide comment
@nhunzaker

nhunzaker Aug 7, 2017

Collaborator

New PRs are up:

Scenario 1: #10397

  1. The attribute whitelist enforces casing for all known attributes
  2. Badly cased entries in the attribute whitelist are not written to the DOM
  3. Attributes not within the whitelist are assigned regardless of casing
  4. The ARIA whitelist has been moved to a developer-only enforcement (but the logic is the same as master)

Scenario 2: #10385

  1. Badly cased attribute names still write to the DOM.
  2. Cases like classname, htmlfor, and arabicform, though badly cased, write to their aliases (class, for, and arabic-form. A dev-only warning is given for bad casing.
  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)

Both entries make ARIA enforcement DEV only.

Collaborator

nhunzaker commented Aug 7, 2017

New PRs are up:

Scenario 1: #10397

  1. The attribute whitelist enforces casing for all known attributes
  2. Badly cased entries in the attribute whitelist are not written to the DOM
  3. Attributes not within the whitelist are assigned regardless of casing
  4. The ARIA whitelist has been moved to a developer-only enforcement (but the logic is the same as master)

Scenario 2: #10385

  1. Badly cased attribute names still write to the DOM.
  2. Cases like classname, htmlfor, and arabicform, though badly cased, write to their aliases (class, for, and arabic-form. A dev-only warning is given for bad casing.
  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)

Both entries make ARIA enforcement DEV only.

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Aug 7, 2017

Member

Nice, thanks! Mind measuring sizes in both?

Member

gaearon commented Aug 7, 2017

Nice, thanks! Mind measuring sizes in both?

@nhunzaker

This comment has been minimized.

Show comment
Hide comment
@nhunzaker

nhunzaker Aug 7, 2017

Collaborator

Done. For production ReactDOM specifically:

Scenario 1: 116.33 KB (37.01 KB gz) (-2%)
Scenario 2: 109kb (34.72kb gz) (-7%)

Scenario 2 also cuts react-dom-server down to 15.5 kb (6.11kb gz). Roughly -25%.

Edit: A screenshot of the readout has been attached to each PR.

Collaborator

nhunzaker commented Aug 7, 2017

Done. For production ReactDOM specifically:

Scenario 1: 116.33 KB (37.01 KB gz) (-2%)
Scenario 2: 109kb (34.72kb gz) (-7%)

Scenario 2 also cuts react-dom-server down to 15.5 kb (6.11kb gz). Roughly -25%.

Edit: A screenshot of the readout has been attached to each PR.

@nhunzaker

This comment has been minimized.

Show comment
Hide comment
@nhunzaker

nhunzaker Aug 8, 2017

Collaborator

Closing this out in favor of #10385

Collaborator

nhunzaker commented Aug 8, 2017

Closing this out in favor of #10385

@hph

This comment has been minimized.

Show comment
Hide comment
@hph

hph Sep 27, 2017

What is the reason for not passing boolean props to the DOM? For instance, currently <div custom /> becomes <div></div> instead of <div custom></div>. Boolean attributes are allowed as per the spec. I noticed that there are tests in place to enforce this behaviour, so this must be a conscious decision.

Sorry for commenting on an old PR (and probably a dumb question), but after digging through the codebase and trying to figure it out this seemed like the most appropriate place to ask.

[Edit: sorry, I meant to write this over in #10564 -- too many open tabs! Left it here to reduce noise, but I can also comment there]

hph commented Sep 27, 2017

What is the reason for not passing boolean props to the DOM? For instance, currently <div custom /> becomes <div></div> instead of <div custom></div>. Boolean attributes are allowed as per the spec. I noticed that there are tests in place to enforce this behaviour, so this must be a conscious decision.

Sorry for commenting on an old PR (and probably a dumb question), but after digging through the codebase and trying to figure it out this seemed like the most appropriate place to ask.

[Edit: sorry, I meant to write this over in #10564 -- too many open tabs! Left it here to reduce noise, but I can also comment there]

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Sep 27, 2017

Member

You can do this explicitly:

<div custom="true" />

Unfortunately, browsers can be wonky about booleans, so we decided we can't really "know" how to correctly set it if we don't know the attribute/property. For example, "false" is actually truthy, and this can be quite surprising.

Member

gaearon commented Sep 27, 2017

You can do this explicitly:

<div custom="true" />

Unfortunately, browsers can be wonky about booleans, so we decided we can't really "know" how to correctly set it if we don't know the attribute/property. For example, "false" is actually truthy, and this can be quite surprising.

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