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

Adjust input value detachment to avoid input validation in FireFox #11202

Closed
wants to merge 4 commits into from
Closed

Adjust input value detachment to avoid input validation in FireFox #11202

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d0ee875
Adjust input value detachment to avoid input validation in FireFox
nhunzaker Oct 12, 2017
152d050
Use setAttribute instead of setting type property for IE
nhunzaker Oct 12, 2017
e718df3
Update flow type for input wrapper state. Fix test
nhunzaker Oct 12, 2017
9096dd1
Format
nhunzaker Oct 12, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 40 additions & 0 deletions fixtures/dom/src/components/fixtures/text-inputs/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,46 @@ class TextInputFixtures extends React.Component {
</p>
</TestCase>

<TestCase
title="Required Inputs"
affectedBrowsers="Firefox"
relatedIssues="8395">
<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 input is invalid.
<br />
This aura looks roughly like:
<input style={{boxShadow: '0 0 1px 1px red', marginLeft: 8}} />
</TestCase.ExpectedResult>

<Fixture>
<form className="control-box">
<fieldset>
<legend>Text</legend>
<input required={true} />
</fieldset>
<fieldset>
<legend>Date</legend>
<input type="date" required={true} />
</fieldset>
</form>
</Fixture>

<p className="footnote">
Checking the date type is also important because of a
prior fix for iOS Safari that involved assigning over
value/defaultValue properties of the input to prevent a
display bug. This also triggered input validation.
</p>
</TestCase>

<TestCase title="Cursor when editing email inputs">
<TestCase.Steps>
<li>Type "user@example.com"</li>
Expand Down
28 changes: 15 additions & 13 deletions src/renderers/dom/fiber/wrappers/ReactDOMFiberInput.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,8 @@ type InputWithWrapperState = HTMLInputElement & {
_wrapperState: {
initialValue: ?string,
initialChecked: ?boolean,
controlled?: boolean,
controlled: boolean,
detached: boolean,
},
};

Expand Down Expand Up @@ -141,6 +142,7 @@ var ReactDOMInput = {
: props.defaultChecked,
initialValue: props.value != null ? props.value : defaultValue,
controlled: isControlled(props),
detached: false,
};
},

Expand Down Expand Up @@ -218,6 +220,13 @@ var ReactDOMInput = {
}
} else {
if (props.value == null && props.defaultValue != null) {
// Whenever setting defaultValue, ensure that the value
// property is detatched
if (node._wrapperState.detached === false) {
node.value = node.value;
node._wrapperState.detached = true;
}

// In Chrome, assigning defaultValue to certain input types triggers input validation.
// For number inputs, the display value loses trailing decimal points. For email inputs,
// Chrome raises "The specified value <x> is not a valid email address".
Expand All @@ -239,16 +248,7 @@ var ReactDOMInput = {
postMountWrapper: function(element: Element, props: Object) {
var node = ((element: any): InputWithWrapperState);

// Detach value from defaultValue. We won't do anything if we're working on
// submit or reset inputs as those values & defaultValues are linked. They
// are not resetable nodes so this operation doesn't matter and actually
// removes browser-default values (eg "Submit Query") when no value is
// provided.

switch (props.type) {
case 'submit':
case 'reset':
break;
case 'color':
case 'date':
case 'datetime':
Expand All @@ -258,11 +258,13 @@ var ReactDOMInput = {
case 'week':
// This fixes the no-show issue on iOS Safari and Android Chrome:
// https://github.com/facebook/react/issues/7233
node.value = '';
node.value = node.defaultValue;
//
// Important: use setAttribute instead of node.type = "x" to avoid
// an exception in IE10/11 due to an unrecognized input type
node.setAttribute('type', 'text');
Copy link
Collaborator

Choose a reason for hiding this comment

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

I was under the impression that you can't change the type of an input after it's created in at least some versions of IE. Do you mind double checking which to be sure we don't break something?

Copy link
Contributor

Choose a reason for hiding this comment

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

This is definitely true for some browsers for password types, for "security".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made a quick test:
https://codepen.io/nhunzaker/full/BwPbRK/

This works in:

  • IE 9, 10, 11
  • Firefox 47
  • Chrome 41
  • Safari Desktop 7.0
  • Safari iOS 9

Is this a reasonable test case? This page is over HTTPs. I wonder if it would be different otherwise.

In any case, this only applies to date/time/color inputs, and only when they are mounted for the first time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahh, sorry. Key detail! This fails in IE8, but not IE9 or higher:

screen shot 2017-10-12 at 5 50 48 pm

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks! I think this should be OK then.

Long-term I suppose someone could make an IE8-compatible renderer using the react-reconciler package if they wanted to.

node.setAttribute('type', props.type);
break;
default:
node.value = node.value;
break;
}

Expand Down
10 changes: 2 additions & 8 deletions src/renderers/dom/shared/wrappers/__tests__/ReactDOMInput-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1190,7 +1190,6 @@ describe('ReactDOMInput', () => {
'set min',
'set max',
'set value',
'set value',
'set checked',
'set checked',
]);
Expand Down Expand Up @@ -1232,11 +1231,6 @@ describe('ReactDOMInput', () => {
spyOn(document, 'createElement').and.callFake(function(type) {
var el = originalCreateElement.apply(this, arguments);
if (type === 'input') {
Object.defineProperty(el, 'value', {
set: function(val) {
log.push(`node.value = ${strify(val)}`);
},
});
spyOn(el, 'setAttribute').and.callFake(function(name, val) {
log.push(`node.setAttribute(${strify(name)}, ${strify(val)})`);
});
Expand All @@ -1250,8 +1244,8 @@ describe('ReactDOMInput', () => {
expect(log).toEqual([
'node.setAttribute("type", "date")',
'node.setAttribute("value", "1980-01-01")',
'node.value = ""',
'node.value = ""',
'node.setAttribute("type", "text")',
'node.setAttribute("type", "date")',
'node.setAttribute("checked", "")',
'node.setAttribute("checked", "")',
]);
Expand Down