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.

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

Remove loose check on non-number controlled inputs. Fix trailing dot issue. #9584

Merged
merged 3 commits into from
May 3, 2017

Conversation

nhunzaker
Copy link
Contributor

In #7359 I added a loose check before assigning the value attribute on all controlled inputs. This was originally added to account for some edge cases in Chrome/Safari when working on number inputs (see the discussion). However, at some point I added a specific check for number inputs and never removed this check for other input types.

So that created an issue: #9561

This commit removes that loose check, adds tests for the associated cases, and updates old tests to be in line with the new expectation. I also added a new DOM test fixture and updated the input DOM fixtures to use the new format we're using in other tests.

Test page:

http://nh-controlled-text-input-numbers.surge.sh/text-inputs

Related issues:

#9561

This commit removes a check I added when working on number input
issues where we perform a loose check on an input's value before we
assign it. This prevented controlled text inputs from disallowing
numeric text entry.

I also added a DOM fixture text case.

Related issues:

facebook#9561 (comment)
@gaearon
Copy link
Collaborator

gaearon commented May 2, 2017

Maybe unrelated, but why does editing "controlled email" in the middle throw me to the end of the input?

@gaearon
Copy link
Collaborator

gaearon commented May 2, 2017

Same question for URLs and passwords. I'm on latest stable Chrome on OSX.

@aweary
Copy link
Contributor

aweary commented May 2, 2017

It's because it's now alway setting node.value. I'm guessing that implies a new, unrelated value and cursor position is discarded (example), which isn't what we want.

@nhunzaker could we just change the loose equality check to a strict one?

@aweary aweary self-assigned this May 2, 2017
@gaearon
Copy link
Collaborator

gaearon commented May 2, 2017

Haha, first instance of fixtures saving us? 😄

gaearon
gaearon previously requested changes May 2, 2017
Copy link
Collaborator

@gaearon gaearon left a comment

Choose a reason for hiding this comment

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

See the bug above.

@aweary
Copy link
Contributor

aweary commented May 2, 2017

It's already paying off 😄 @nhunzaker thanks for deploying the fixtures by the way, that really makes it easier to review. It would be cool if we could include some instructions on doing that for the more ambitious contributors.

This commit adds back the guard around assigning the value property to
an input, however it does it using a strict equals. This prevents
validated inputs, like emails and urls from losing the cursor
position.

It also adds associated test fixtures.
@nhunzaker nhunzaker force-pushed the nh-controlled-text-input-numbers branch from b0df2fc to d421181 Compare May 3, 2017 00:12
@nhunzaker
Copy link
Contributor Author

@aweary Good thought. For the time being, I've just added an extra bit to the build script to copy over index.html to 200.html for surge.sh. Could something like that live in the readme?

Good call too! The strict equality check totally did it.

http://nh-controlled-text-input-numbers.surge.sh/text-inputs

@gaearon
Copy link
Collaborator

gaearon commented May 3, 2017

@aweary Mind doing a full review?

Copy link
Contributor

@aweary aweary left a comment

Choose a reason for hiding this comment

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

I ran through the fixtures for text and number inputs in Chrome, Firefox, Safari, IE11, and Edge. Everything seems to be working great!

We need to include a requestAnimationFrame polyfill to test in older browsers, but I think we can do that in a follow-up.

@gaearon
Copy link
Collaborator

gaearon commented May 3, 2017

Do you want to take this into 15.6?

@aweary
Copy link
Contributor

aweary commented May 3, 2017

@gaearon I was just typing that same question! 😄 I think including it in the next 15 release is a good idea, since it fixes a regression. What needs to be done to make sure this is included in 15.6?

@gaearon
Copy link
Collaborator

gaearon commented May 3, 2017

@flarnie What are your thoughts, would this be difficult? Seems like it’s a fix we want to get in (since it fixes a 15.5 regression).

@flarnie flarnie mentioned this pull request May 3, 2017
49 tasks
@flarnie
Copy link
Contributor

flarnie commented May 3, 2017

I agree - this would be good to get into the 15.6 release. I'll cherry-pick it.
Is there a reason we haven't merged this to master yet?

Also "We need to include a requestAnimationFrame polyfill to test in older browsers, but I think we can do that in a follow-up." - I'm not seeing any use of 'requestAnimationFrame' in these changes, is there still a need for a polyfill there?

@aweary
Copy link
Contributor

aweary commented May 3, 2017

Is there a reason we haven't merged this to master yet?

I didn't want to merge until I was clear on how including this in 15.6 was handled.

Also "We need to include a requestAnimationFrame polyfill to test in older browsers, but I think we can do that in a follow-up." - I'm not seeing any use of 'requestAnimationFrame' in these changes, is there still a need for a polyfill there?

Nothing specific to these changes, Fiber requires requestAnimationFrame, so none of the fixtures are currently working in browsers that do not support it.

@aweary aweary merged commit 6875fa8 into facebook:master May 3, 2017
renderControlled = (type) => {
let id = `controlled_${type}`;
<TestCase.ExpectedResult>
The text field's value should not update.
Copy link
Contributor

Choose a reason for hiding this comment

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

Might change to {`The text field's value should not update.`} to make syntax highlighters happy.

flarnie pushed a commit that referenced this pull request May 3, 2017
…issue. (#9584)

* Remove loose check when assigning non-number inputs

This commit removes a check I added when working on number input
issues where we perform a loose check on an input's value before we
assign it. This prevented controlled text inputs from disallowing
numeric text entry.

I also added a DOM fixture text case.

Related issues:

#9561 (comment)

* Use strict equality as a guard before assigning input.value

This commit adds back the guard around assigning the value property to
an input, however it does it using a strict equals. This prevents
validated inputs, like emails and urls from losing the cursor
position.

It also adds associated test fixtures.

* Add copy command after build for interup with surge.sh
flarnie added a commit to flarnie/react that referenced this pull request May 3, 2017
**what is the change?:**
This seems useful, even for this short-lived branch, and some commits we
cherry-picked were making updates to it. So we just pulled the fixture
in from master.

**why make this change?:**
To bring things into a bit more consistency with master.

**test plan:**
Manually tested the fixture - things seem to work.

**issue:**
None - but related to cherry-picking
facebook#9584
flarnie added a commit to flarnie/react that referenced this pull request May 3, 2017
Updating the change log~ :)

**issue:**
facebook#9398
flarnie added a commit that referenced this pull request May 9, 2017
Updating the change log~ :)

**issue:**
#9398
flarnie pushed a commit that referenced this pull request May 31, 2017
This is a follow-up on
#9584 (comment). There
is no need to assign the value property of an input if the value
property of the React component changes types, but stringifies to the
same value. For example:

```javascript
DOM.render(<input value="true" />, el)
DOM.render(<input value={true} />, el)
```

In this case, the assignment to `input.value` will always be
cast to the string "true". There is no need to perform this
assignment. Particularly when we already cast the value to a string
later:

```javascript
// Cast `value` to a string to ensure the value is set correctly. While
// browsers typically do this as necessary, jsdom doesn't.
node.value = '' + value;
```
flarnie pushed a commit to flarnie/react that referenced this pull request Jun 7, 2017
…issue. (facebook#9584)

* Remove loose check when assigning non-number inputs

This commit removes a check I added when working on number input
issues where we perform a loose check on an input's value before we
assign it. This prevented controlled text inputs from disallowing
numeric text entry.

I also added a DOM fixture text case.

Related issues:

facebook#9561 (comment)

* Use strict equality as a guard before assigning input.value

This commit adds back the guard around assigning the value property to
an input, however it does it using a strict equals. This prevents
validated inputs, like emails and urls from losing the cursor
position.

It also adds associated test fixtures.

* Add copy command after build for interup with surge.sh
flarnie pushed a commit to flarnie/react that referenced this pull request Jun 7, 2017
This is a follow-up on
facebook#9584 (comment). There
is no need to assign the value property of an input if the value
property of the React component changes types, but stringifies to the
same value. For example:

```javascript
DOM.render(<input value="true" />, el)
DOM.render(<input value={true} />, el)
```

In this case, the assignment to `input.value` will always be
cast to the string "true". There is no need to perform this
assignment. Particularly when we already cast the value to a string
later:

```javascript
// Cast `value` to a string to ensure the value is set correctly. While
// browsers typically do this as necessary, jsdom doesn't.
node.value = '' + value;
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants