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

Jump to bottom

Custom Attributes Scenario 1: Do not write badly cased attributes. Keep Whitelist. #10397

Closed
wants to merge 23 commits into from
Closed

Custom Attributes Scenario 1: Do not write badly cased attributes. Keep Whitelist. #10397

wants to merge 23 commits into from

Conversation

nhunzaker
Copy link
Contributor

@nhunzaker nhunzaker commented Aug 7, 2017 •
edited
Loading

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

  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)

This approach is more strict than scenario 2. Badly cased attributes will not show up in the DOM. However this enforcement means that we can not remove many entries in the attribute whitelist.

Build size:

react-dom.production.min.js: 116.33 KB (37.01 KB gz)

screen shot 2017-08-07 at 8 12 26 am

@nhunzaker
Allow custom attributes. Add flag to toggle custom attribute behavior
afae07c
@nhunzaker
Update custom attribute logic
3388f4f 
- Only allow string attributes
- Remove custom attribute feature flag
- Add additional tests for data, aria, and custom attributes
@nhunzaker
Allow numbers and booleans custom attributes. Cut isCustomAttribute
9f7a696
@nhunzaker
Cover objects with custom attributes in warning test
fd94756
@nhunzaker
Rename DOMProperty.isWriteable to shouldSetAttribute
f870980
@nhunzaker
Rework conditions in shouldSetProperty to avoid edge cases
ab64ef3
@nhunzaker
Update unknown property warning to include custom attribute information
7717ed7
@nhunzaker
Remove ref and key from reserved props
2b402a4
@nhunzaker
Ensure SSR test coverage for DOMProperty injections
1681e7f
@nhunzaker
Add ajaxify attribute for internal FB support
b5736c7
@nhunzaker
Ajaxify is a stringifiable object attribute
976ca82
@nhunzaker
Update test name for custom attributes on custom elements
17d3c49
@nhunzaker
Remove SSR custom injection test
40dee5a
@nhunzaker
Remove onAfterResetModules hooks in SSR render tests
b26384d
@nhunzaker
Do not allow assignment of attributes that are aliased
d672771
@nhunzaker
Update custom attribute test to check value, not just presence
a871d55
@nhunzaker
Address case where class is assigned as an attribute on custom elemen…
c748e84 
…ts. Improve SSR tests
@nhunzaker
Cover cases where className and for are given to custom elements
16dd18a
@nhunzaker
Remove unnecessary spys on console.error. Reduce extra space in tests
e68bf4d
@nhunzaker
Cover cased custom attributes in SSR tests
83e46aa
@nhunzaker
Custom attributes are case sensitive
facfa87
@nhunzaker
Allow uppercase letters in custom attributes. Address associated edge…
82e05e0 
… cases
@nhunzaker
Make ARIA enforcement dev-only
f355e75
@nhunzaker nhunzaker changed the title Custom Attributes Scenario 2 Custom Attributes Scenario 1 Aug 7, 2017
@nhunzaker nhunzaker changed the title Custom Attributes Scenario 1 Custom Attributes Scenario 1: Do not write badly cased attributes Aug 7, 2017
@nhunzaker nhunzaker changed the title Custom Attributes Scenario 1: Do not write badly cased attributes Custom Attributes Scenario 1: Do not write badly cased attributes. Keep Whitelist. Aug 7, 2017
This was referenced Aug 7, 2017
Closed
Closed
@nhunzaker
Copy link
Contributor Author

Closing this out in favor of #10385

@nhunzaker nhunzaker closed this Aug 7, 2017
@gaearon gaearon mentioned this pull request Sep 8, 2017
Closed
@gaearon gaearon deleted the nh-strip-whitelist branch October 6, 2017 21:57
@gaearon gaearon mentioned this pull request Jan 9, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
CLA Signed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@nhunzaker @facebook-github-bot