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

prevent firefox marking required textareas invalid #16578

Merged
merged 3 commits into from Sep 18, 2019
Merged

prevent firefox marking required textareas invalid #16578

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
533b66a
prevent firefox marking required textareas invalid
halvves Aug 27, 2019
55563b9
more explicit conditional check for textContent
halvves Sep 10, 2019
ca530ef
clarify textarea test fixture's expected result
halvves Sep 10, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
38 changes: 38 additions & 0 deletions fixtures/dom/src/components/fixtures/textareas/index.js
View file
Open in desktop
@@ -1,3 +1,4 @@
import Fixture from '../../Fixture';
import FixtureSet from '../../FixtureSet';
import TestCase from '../../TestCase';

Expand Down Expand Up @@ -39,6 +40,43 @@ export default class TextAreaFixtures extends React.Component {
<textarea placeholder="Hello, world" />
</div>
</TestCase>

<TestCase
title="Required Textareas"
affectedBrowsers="Firefox"
relatedIssues="16402">
<TestCase.Steps>
<li>View this test in Firefox</li>
</TestCase.Steps>

<TestCase.ExpectedResult>
You should{' '}
<b>
<i>not</i>
</b>{' '}
see a red aura, indicating the textarea is invalid.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain this a bit better? I do see a red outline specifically when I focus the "Empty value prop string" textarea, hit any character on the keyboard, and then move the focus to one of the other textareas. Is this expected?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting. Great catch! I don't think there is anything we can do about this though, assuming this is what's happening:

  1. A user enters text
  2. The onChange callback does not update state (no value change)
  3. The text is still entered, so React must assign an empty value to "control" it
  4. Firefox triggers validation on blur, and the input has been modified at this point, so the field is marked as invalid.

So controlling the input is at odds with input validation. I think this is okay for fields that don't allow any text input because they should really have readOnly on them instead of a blank change callback.

I think these are setup this way to avoid needing to scaffold out the controlled input code. Would it be less confusing if they were set up this way?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah that seems reasonable. Maybe update the instructions then to be clearer about this case.

I think these are setup this way to avoid needing to scaffold out the controlled input code. Would it be less confusing if they were set up this way?

Not quite sure what you mean by that but I think what we have now is already better than what we had 🙂

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh! I just meant adding the callbacks and state for the controlled inputs so that the values are editable. Right now we don't have them, I think because it isn't essential for the fixture.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated expected result to read "...see a red aura on initial page load, indicating the textarea is invalid..."

do you think this is sufficient clarification?

<br />
This aura looks roughly like:
<textarea style={{boxShadow: '0 0 1px 1px red', marginLeft: 8}} />
</TestCase.ExpectedResult>

<Fixture>
<form className="control-box">
<fieldset>
<legend>Empty value prop string</legend>
<textarea value="" required={true} />
</fieldset>
<fieldset>
<legend>No value prop</legend>
<textarea required={true} />
</fieldset>
<fieldset>
<legend>Empty defaultValue prop string</legend>
<textarea required={true} defaultValue="" />
</fieldset>
</form>
</Fixture>
</TestCase>
</FixtureSet>
);
}
Expand Down
27 changes: 27 additions & 0 deletions packages/react-dom/src/__tests__/ReactDOMTextarea-test.js
View file
Open in desktop
Expand Up @@ -126,6 +126,33 @@ describe('ReactDOMTextarea', () => {
expect(node.value).toEqual('gorilla');
});

it('will not initially assign an empty value (covers case where firefox throws a validation error when required attribute is set)', () => {
const container = document.createElement('div');

let counter = 0;
const originalCreateElement = document.createElement;
spyOnDevAndProd(document, 'createElement').and.callFake(function(type) {
const el = originalCreateElement.apply(this, arguments);
let value = '';
if (type === 'textarea') {
Object.defineProperty(el, 'value', {
get: function() {
return value;
},
set: function(val) {
value = '' + val;
counter++;
},
});
}
return el;
});

ReactDOM.render(<textarea value="" readOnly={true} />, container);

expect(counter).toEqual(0);
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For others - we pulled this test from ReactDOMInput-test.js, which uses a similar technique to check mutations for an iOS Safari date bug.

I still think this is a weird test, and I wonder if it makes sense to create a common helper for this sort of thing.


it('should render defaultValue for SSR', () => {
const markup = ReactDOMServer.renderToString(<textarea defaultValue="1" />);
const div = document.createElement('div');
Expand Down
4 changes: 3 additions & 1 deletion packages/react-dom/src/client/ReactDOMTextarea.js
View file
Open in desktop
Expand Up @@ -157,7 +157,9 @@ export function postMountWrapper(element: Element, props: Object) {
// will populate textContent as well.
// https://developer.microsoft.com/microsoft-edge/platform/issues/101525/
if (textContent === node._wrapperState.initialValue) {
node.value = textContent;
if (textContent) {
halvves marked this conversation as resolved.
Show resolved Hide resolved
node.value = textContent;
}
}
}

Expand Down