Fix passing symbols and functions to textarea #13362

raunofreiberg · 2018-08-10T12:48:10Z

Somewhat related to #11734. Builds on top of @nhunzaker's work on ReactDOMFiberInput

Reproduced cases that work incorrectly (?) as of today (https://codesandbox.io/s/pj40nrp5px):

return (
  <div>
    <textarea defaultValue={() => {}} /> // Stringified into "function defaultValue() {}"
    <textarea value={() => {}} /> // Stringified into "function value() {}"
    <textarea defaultValue={Symbol("test")} /> // TypeError: Cannot convert a Symbol value to a string
    <textarea value={Symbol("test")} /> // TypeError: Cannot convert a Symbol value to a string
  </div>
);

philipp-spiess

Awesome! Thank you for the PR. I think @nhunzaker should also take a look because it's similar to his work in #11741. And maybe we want this for select as well?

We probably also need to check if we have to update the attribute snapshots: https://github.com/facebook/react/tree/master/fixtures/attribute-behavior

philipp-spiess · 2018-08-10T17:40:17Z

packages/react-dom/src/__tests__/ReactDOMTextarea-test.js

@@ -20,6 +20,8 @@ describe('ReactDOMTextarea', () => {
  let renderTextarea;

  beforeEach(() => {
+    jest.resetModules();


Is there any particular reason for this addition? I think the only reason we do that is when we have tests that require multiple separate versions of React at the same time.

Running the tests added in this PR were failing unless I ran resetModules before each.

Although running them 1 by 1 via fit seemed to fix the issue.

Ah, right, makes sense. Otherwise the warning will only pop up once 👍

Btw, thanks for the feedback. I really appreciate it 🙂

philipp-spiess · 2018-08-10T17:45:55Z

packages/react-dom/src/client/ReactDOMFiberTextarea.js

@@ -116,7 +117,9 @@ export function initWrapperState(element: Element, props: Object) {
  }

  node._wrapperState = {
-    initialValue: '' + initialValue,
+    initialValue: getSafeValue(
+      props.value != null ? props.value : initialValue,


This seems to be different than the previous implementation. Do you mind explaining why? 🙂

Ah, you're right. This has to be a mistake. I took influence from ReactDOMFiberInput and the files got oddly similar under cognitive load.

Instead, it can simply be getSafeValue(initialValue), I think :-)

Thanks!

initialValue already references props.value on L:83 so this logic is redundant.

philipp-spiess · 2018-08-10T17:47:49Z

packages/react-dom/src/__tests__/ReactDOMTextarea-test.js

+      ReactDOM.render(<textarea defaultValue={Symbol('foobar')} />, container);
+      const node = container.firstChild;
+
+      // TODO: add warnings once defaultValue is compatible for them


I don't quite understand this comment. @nhunzaker I think this is copied over from #11741 - Can you point me to something so I can read up on that?

Sorry, should've been more thorough. I've added this comment as a reminder for future references once this PR is good to go. Currently there are no warnings for reserved props in React and hence the TODOs in @nhunzaker's PR and this one :-)

I know I'm the first offender here (what is // TODO: we should warn here. even? 😿 ), but could you change this comment to something like:

// TODO: defaultValue is a reserved prop and is not validated. Check warnings when they are.

raunofreiberg · 2018-08-10T18:50:19Z

packages/react-dom/src/client/ReactDOMFiberTextarea.js

@@ -126,7 +127,7 @@ export function updateWrapper(element: Element, props: Object) {
  if (value != null) {
    // Cast `value` to a string to ensure the value is set correctly. While
    // browsers typically do this as necessary, jsdom doesn't.
-    const newValue = '' + value;
+    const newValue = getSafeValue(value);


I have a question about this change - could someone please be more specific about which cases require the casting done previously ('' + value) and what exactly breaks by removing it from here and the previous lines?

My first recollection is that we don't need to convert these to string values, but we didn't update all of the locations because it wasn't specific to the PR.

Inputs went through a lot of fixes as we ironed out all of the edge cases for special input types on Chrome/Safari, so to some degree, touching more than we needed to felt scary 🙈.

I think it would be good to take a second look at this.

nhunzaker

Looks good at first pass. I'm doing some travel today, but I'll be able to give a more thorough review once I'm settled.

Thanks for sending this out!

nhunzaker · 2018-08-10T20:44:42Z

packages/react-dom/src/client/ReactDOMFiberTextarea.js

@@ -126,7 +127,7 @@ export function updateWrapper(element: Element, props: Object) {
  if (value != null) {
    // Cast `value` to a string to ensure the value is set correctly. While
    // browsers typically do this as necessary, jsdom doesn't.
-    const newValue = '' + value;
+    const newValue = getSafeValue(value);

    // To avoid side effects (such as losing text selection), only set value if changed
    if (newValue !== node.value) {


Ah right. We need the new value to be a string for comparison (edit) because the value always reports as a string.

That does make sense to me. Is there a particular reason though why the casting is done by concatenation instead of e.g. String(value)? Because with Symbols it produces a TypeError when trying to concat a Symbol and a string.

Moreover, if a textarea receives for i.e a function as its value and we concat it into a string, then getSafeValue would fail its check and append it as a valid value.

Maybe we could add a similar check like in ReactDOMFiberInput?

Is there a particular reason though why the casting is done by concatenation instead of e.g. String(value)?

There were some performance concerns about String(): #11741 (comment)

because the value always reports as a string

I was wondering why Flow did not catch that and found the issue in the getSafeValue() function. Not sure if we want the fix since it adds some complexity but it's nice to know that all code paths indeed work. Check out: #13367

This is an attempt in improving the soundness for the safe value cast that was added in facebook#11741. We want this to avoid situations like [this one](facebook#13362 (comment)) where we need to remember why we have certain type casts. Additionally we can be sure that we only cast safe values to string. The problem was `getSafeValue()`. It used the (deprecated) `*` type to infer the type which resulted in a passing-through of the implicit `any` of the props `Object`. So `getSafeValue()` was effectively returning `any`. Once I fixed this, I found out that Flow does not allow concatenating all possible types to a string (e.g `"" + false` fails in Flow). To fix this as well, I've opted into making the SafeValue type opaque and added a function that can be used to get the string value. This is sound because we know that SafeValue is already checked. I've verified that the interim function is inlined by the compiler and also looked at a diff of the compiled react-dom bundles to see if I've regressed anything. Seems like we're good.

* Improve soundness of ReactDOMFiberInput typings This is an attempt in improving the soundness for the safe value cast that was added in #11741. We want this to avoid situations like [this one](#13362 (comment)) where we need to remember why we have certain type casts. Additionally we can be sure that we only cast safe values to string. The problem was `getSafeValue()`. It used the (deprecated) `*` type to infer the type which resulted in a passing-through of the implicit `any` of the props `Object`. So `getSafeValue()` was effectively returning `any`. Once I fixed this, I found out that Flow does not allow concatenating all possible types to a string (e.g `"" + false` fails in Flow). To fix this as well, I've opted into making the SafeValue type opaque and added a function that can be used to get the string value. This is sound because we know that SafeValue is already checked. I've verified that the interim function is inlined by the compiler and also looked at a diff of the compiled react-dom bundles to see if I've regressed anything. Seems like we're good. * Fix typo

philipp-spiess · 2018-08-12T15:14:36Z

packages/react-dom/src/client/ReactDOMFiberTextarea.js

@@ -11,6 +11,7 @@ import invariant from 'shared/invariant';
 import warning from 'shared/warning';

 import ReactControlledValuePropTypes from '../shared/ReactControlledValuePropTypes';
+import getSafeValue from './getSafeValue';

 let didWarnValDefaultVal = false;



The type of initialValue in the _wrapperState below should probably be SafeValue now, just like we did for input elements.

Edit: Nevermind you have to push your changes first 🙈

Just pushed my changes - I hope they make sense and I took the right approach :-)

raunofreiberg · 2018-08-12T15:35:16Z

packages/react-dom/src/client/ReactDOMFiberTextarea.js

@@ -54,7 +55,7 @@ export function getHostProps(element: Element, props: Object) {
    ...props,
    value: undefined,
    defaultValue: undefined,
-    children: '' + node._wrapperState.initialValue,
+    children: safeValueToString(getSafeValue(node._wrapperState.initialValue)),


Can we just do safeValueToString(node._wrapperState.initialValue) instead here?

Yes, this seems correct 👍 . _wrapperState is only set once and that is already a SafeValue.

philipp-spiess

Thanks for updating the PR against my changes so quickly 🙂 If you want, you can take a look at as well.

philipp-spiess · 2018-08-12T16:37:05Z

packages/react-dom/src/client/ReactDOMFiberTextarea.js

@@ -54,7 +55,7 @@ export function getHostProps(element: Element, props: Object) {
    ...props,
    value: undefined,
    defaultValue: undefined,
-    children: '' + node._wrapperState.initialValue,
+    children: safeValueToString(getSafeValue(node._wrapperState.initialValue)),


Yes, this seems correct 👍 . _wrapperState is only set once and that is already a SafeValue.

philipp-spiess · 2018-08-12T16:42:17Z

packages/react-dom/src/client/ReactDOMFiberTextarea.js

@@ -107,7 +109,7 @@ export function initWrapperState(element: Element, props: Object) {
        children = children[0];
      }

-      defaultValue = '' + children;
+      defaultValue = getSafeValue(children);


defaultValue was a string before and is a SafeValue now. I'm not sure if this causes issues with the equality test below.

Should we stringify here as well, just to be safe?

philipp-spiess · 2018-08-12T16:45:49Z

packages/react-dom/src/client/ReactDOMFiberTextarea.js

    // To avoid side effects (such as losing text selection), only set value if changed
-    if (newValue !== node.value) {
-      node.value = newValue;
+    if ((value: any) !== node.value) {


We compared this against a string before and now it's a SafeValue. Is this intended? I think this means when value is a non-string (i.e a number) it will always update node.value since the strict equality will always return false.

Hm, right. Would it make more sense if the comparison was props.value !== node.value and use getSafeValue for const newValue?

That would still false if props.value = 1 and node.value = '1'. I think bringing back const newValue as safeValueToString(value) is more correct.

Gotcha. Pushed a change with regards to this.

@gaearon

Following up on the changes I made in facebook#13367, @gaearon suggest that "safe" could be read as necessary for security. To avoid misleading a reader, I'm changing the name. A few names where discussed in the previous PR. I think `ToStringValue` makes sense since the value itself is not a string yet but an opaque type that can be cast to a string. For the actual string concatenation, I've used `toString` now to avoid confusion: `toStringValueToString` is super weird and it's namespaced anyhow. Definitely open for suggestions here. :) I'll wait until we wrap up facebook#13362 and take care of rebase afterwards.

@gaearon

Following up on the changes I made in #13367, @gaearon suggest that "safe" could be read as necessary for security. To avoid misleading a reader, I'm changing the name. A few names where discussed in the previous PR. I think `ToStringValue` makes sense since the value itself is not a string yet but an opaque type that can be cast to a string. For the actual string concatenation, I've used `toString` now to avoid confusion: `toStringValueToString` is super weird and it's namespaced anyhow. Definitely open for suggestions here. :) I'll wait until we wrap up #13362 and take care of rebase afterwards.

raunofreiberg · 2018-08-13T14:13:57Z

Synced up with the changes from #13376.

nhunzaker

Changes look good from my end.

raunofreiberg · 2018-08-14T08:07:25Z

Rebased and fixed conflict derived from #13361.

philipp-spiess

Looks great already! Just one comment.

philipp-spiess · 2018-08-14T08:53:56Z

packages/react-dom/src/client/ReactDOMFiberTextarea.js

@@ -110,7 +112,7 @@ export function initWrapperState(element: Element, props: Object) {
        children = children[0];
      }

-      defaultValue = '' + children;
+      defaultValue = getToStringValue(children);


Can you find out if we really need to convert to ToStringValue here?

I think we should be able to just pass children directly as the defaultValue since we convert to the ToStringValue down below anyway.

You're right, this does seem redundant. Removed it, thanks!

philipp-spiess

This looks perfect now. Thank you very much 🙂 If you want a follow-up task, you could work on the same fix for elements.

nhunzaker · 2018-08-14T14:13:38Z

Taking a quick pass to verify this with the fixtures, but this looks good.

nhunzaker · 2018-08-14T14:16:42Z

Thanks!

raunofreiberg · 2018-08-14T14:18:02Z

My pleasure 🙂

facebook-github-bot added the CLA Signed label Aug 10, 2018

raunofreiberg changed the title ~~Ignore symbols functions textarea~~ Fix passing symbols and functions to textarea Aug 10, 2018

philipp-spiess reviewed Aug 10, 2018

View reviewed changes

raunofreiberg commented Aug 10, 2018

View reviewed changes

nhunzaker reviewed Aug 10, 2018

View reviewed changes

philipp-spiess mentioned this pull request Aug 11, 2018

Improve soundness of ReactDOMFiberInput typings #13367

Merged

philipp-spiess reviewed Aug 12, 2018

View reviewed changes

raunofreiberg commented Aug 12, 2018

View reviewed changes

philipp-spiess reviewed Aug 12, 2018

View reviewed changes

philipp-spiess mentioned this pull request Aug 12, 2018

Rename SafeValue to ToStringValue #13376

Merged

nhunzaker approved these changes Aug 13, 2018

View reviewed changes

raunofreiberg added 10 commits August 14, 2018 11:03

refactor: move getSafeValue to separate file

4113aa9

fix(?): ReactDOMFiberTextarea sanitization for symbols and functions

57f34d6

tests: add TODOs for warnings

635d288

fix: restore accidentally removed test

1c4bded

fix: remove redundant logic for initialValue

0564729

refactor: integrate SafeValue typings into textarea

282571c

fix: restore stringified newValue for equality check

88f40f1

fix: remove getSafeValue from hostProps

07ab55d

refactor: SafeValue -> ToStringValue

d350db1

refactor: update TODO comment in test file

c3dc409

philipp-spiess reviewed Aug 14, 2018

View reviewed changes

refactor: no need to convert children to ToStringValue

1ff2275

philipp-spiess approved these changes Aug 14, 2018

View reviewed changes

raunofreiberg mentioned this pull request Aug 14, 2018

Ignore symbols and functions in select tag #13389

Merged

nhunzaker merged commit d04d03e into facebook:master Aug 14, 2018

raunofreiberg deleted the ignore-symbols-functions-textarea branch August 14, 2018 14:19

gaearon mentioned this pull request Sep 5, 2018

Changelog for 16.5.0 #13571

Merged

dependencies bot mentioned this pull request Oct 1, 2018

Update react-dom in /modules/frontend from 16.3.0 to 16.5.2 project-accelerate/accelerate#277

Open

This was referenced Nov 1, 2018

Update react-dom in / from 16.2.0 to 16.6.0 leonardofaria/leozera-ui#253

Closed

Update react in / from 16.2.0 to 16.6.0 leonardofaria/leozera-ui#254

Closed

This was referenced Dec 1, 2018

Update react-test-renderer in /modules/frontend from 16.3.0 to 16.6.3 project-accelerate/accelerate#312

Open

Update react in /modules/frontend from 16.3.0 to 16.6.3 project-accelerate/accelerate#313

Open

sebmarkbage mentioned this pull request Mar 14, 2022

test: Update attribute fixture snapshot #24083

Merged

Fix passing symbols and functions to textarea #13362

Fix passing symbols and functions to textarea #13362

Conversation

raunofreiberg commented Aug 10, 2018 • edited

philipp-spiess left a 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

Choose a reason for hiding this comment

raunofreiberg Aug 10, 2018 • edited

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

raunofreiberg Aug 10, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nhunzaker left a comment

Choose a reason for hiding this comment

nhunzaker Aug 10, 2018 • edited

Choose a reason for hiding this comment

raunofreiberg Aug 10, 2018 • edited

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

philipp-spiess Aug 12, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raunofreiberg Aug 12, 2018 • edited

Choose a reason for hiding this comment

philipp-spiess Aug 12, 2018 • edited

Choose a reason for hiding this comment

philipp-spiess left a comment

Choose a reason for hiding this comment

philipp-spiess Aug 12, 2018 • edited

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

raunofreiberg Aug 12, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raunofreiberg commented Aug 13, 2018

nhunzaker left a comment

Choose a reason for hiding this comment

raunofreiberg commented Aug 14, 2018

philipp-spiess left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

philipp-spiess left a comment • edited

Choose a reason for hiding this comment

nhunzaker commented Aug 14, 2018

nhunzaker commented Aug 14, 2018

raunofreiberg commented Aug 14, 2018

raunofreiberg commented Aug 10, 2018 •

edited

raunofreiberg Aug 10, 2018 •

edited

raunofreiberg Aug 10, 2018 •

edited

nhunzaker Aug 10, 2018 •

edited

raunofreiberg Aug 10, 2018 •

edited

philipp-spiess Aug 12, 2018 •

edited

raunofreiberg Aug 12, 2018 •

edited

philipp-spiess Aug 12, 2018 •

edited

philipp-spiess Aug 12, 2018 •

edited

raunofreiberg Aug 12, 2018 •

edited

philipp-spiess left a comment •

edited