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

Problem when input type changes from email to text #12062

Closed
adrianimboden opened this issue Jan 20, 2018 · 12 comments
Closed

Problem when input type changes from email to text #12062

adrianimboden opened this issue Jan 20, 2018 · 12 comments
Labels
Component: DOM Type: Bug

Comments

@adrianimboden
Copy link

adrianimboden commented Jan 20, 2018 •
edited
Loading

Do you want to request a feature or report a bug?
Bug

What is the current behavior?
When an input field changes from "email" to "text", an exception TypeError will be thrown from setSelection.
Screenshot

Reproduction
See here: https://github.com/adrianimboden/react-bug-reproduction

What is the expected behavior?
It should not crash because of an uncaught exception

Which versions of React, and which browser / OS are affected by this issue? Did this work in previous versions of React?
master (4ca7855)
Firefox

This would be my proposed change to fix the issue: adrianimboden@db923b8
@nhunzaker
Copy link
Contributor

Dang. Just a gut triage, I think this is happening because React is trying to restore selection before the input actually changes types. For what ever reason, email and number inputs do not support the text selection API. There's an order of operations here that is out of sync.

Great catch!

@nhunzaker
Copy link
Contributor

Circling back! Thanks for providing a possible fix!

  1. I want to make sure that checking the type is sufficient. Should we instead compare the value of selectionStart? For example:
var input = document.createElement('input')
input.selectionStart // 0

input.type = 'email'
input.selectionStart // null
input.selectionEnd // null
  1. From there, it would be awesome to make a DOM test fixture

Is that something you'd like to take on?

@leonascimento
Copy link

Ping @adrianimboden adrianimboden@db923b8#diff-a654f37b01573fc8006b426d56ad53ceR30

@adrianimboden
Copy link
Author

Sorry, I was a little bit busy. I don't know when I can free some time to implement a proper fix with tests.
If someone wants to pick up on the matter, I would not be disappointed.

@spirosikmd
Copy link
Contributor

I can pick this up! Would that be ok @adrianimboden?

@spirosikmd
Copy link
Contributor

@leonascimento regarding the tests, any idea where could they be located? If there aren't any tests for ReactInputSelection then maybe creating react/packages/react-dom/src/client/__tests__/ReactInputSelection-test.js is an option to explicitly test hasSelectionCapabilities function?

Otherwise, if we want to test this on a more integration level, should we extend react/packages/react-dom/src/events/__tests__/SelectEventPlugin-test.js tests?

@Nasir13
Copy link

Nasir13 commented Feb 1, 2018 via email

@spirosikmd
Copy link
Contributor

Hi @Nasir13! I am currently working on this issue. I mention it so we don't end up doing duplicate work!

@nhunzaker
Copy link
Contributor

Awesome. @spirosikmd thanks for picking this up!

@nhunzaker nhunzaker assigned nhunzaker and unassigned nhunzaker Feb 1, 2018
spirosikmd added a commit to spirosikmd/react that referenced this issue Feb 1, 2018
@spirosikmd
Extend input type check in selection capabilities (facebook#12062)
e9ce874 
When an email input was replaced by a disabled text input on the same
DOM position, an error would occur when trying to `setSelection`. The
reason was that in `getSelectionInformation` the `selectionRange` was
set to `null` as `hasSelectionCapabilities` was returning `false` for
an `email` input type.

`email` and `tel` input types do have selection capabilities, so in
that case, `hasSelectionCapabilities` should return `true`.
@spirosikmd spirosikmd mentioned this issue Feb 1, 2018
Merged
spirosikmd added a commit to spirosikmd/react that referenced this issue Feb 1, 2018
@spirosikmd
Extend input type check in selection capabilities (facebook#12062)
fac6ec0 
When an email input was replaced by a disabled text input on the same
DOM position, an error would occur when trying to `setSelection`. The
reason was that in `getSelectionInformation` the `selectionRange` was
set to `null` as `hasSelectionCapabilities` was returning `false` for
an `email` input type.

`email` and `tel` input types do have selection capabilities, so in
that case, `hasSelectionCapabilities` should return `true`.
spirosikmd added a commit to spirosikmd/react that referenced this issue Feb 1, 2018
@spirosikmd
Extend input type check in selection capabilities (facebook#12062)
14771a7 
When an email input was replaced by a disabled text input on the same
DOM position, an error would occur when trying to `setSelection`. The
reason was that in `getSelectionInformation` the `selectionRange` was
set to `null` as `hasSelectionCapabilities` was returning `false` for
an `email` input type.

`email` and `tel` input types do have selection capabilities, so in
that case, `hasSelectionCapabilities` should return `true`.
spirosikmd added a commit to spirosikmd/react that referenced this issue Feb 2, 2018
@spirosikmd
Extend input type check in selection capabilities (facebook#12062)
aa115f7 
When an email input was replaced by a disabled text input on the same
DOM position, an error would occur when trying to `setSelection`. The
reason was that in `getSelectionInformation` the `selectionRange` was
set to `null` as `hasSelectionCapabilities` was returning `false` for
an `email` input type.

`email` and `tel` input types do have selection capabilities, so in
that case, `hasSelectionCapabilities` should return `true`.
spirosikmd added a commit to spirosikmd/react that referenced this issue Feb 4, 2018
@spirosikmd
Extend input type check in selection capabilities (facebook#12062)
c9515d3 
When an email input was replaced by a disabled text input on the same
DOM position, an error would occur when trying to `setSelection`. The
reason was that in `getSelectionInformation` the `selectionRange` was
set to `null` as `hasSelectionCapabilities` was returning `false` for
an `email` input type.

`email` and `tel` input types do have selection capabilities, so in
that case, `hasSelectionCapabilities` should return `true`.
spirosikmd added a commit to spirosikmd/react that referenced this issue Feb 15, 2018
@spirosikmd
Do not set selection when prior selection is undefined (facebook#12062)
bc7d887 
`restoreSelection` did not account for input elements that have changed
type after the commit phase. The new `text` input supported selection
but the old `email` did not and `setSelection` was incorrectly trying to
restore `null` selection state.

We also extend input type check in selection capabilities to cover cases
where input type is `search`, `tel`, `url`, or `password`.
@diegoborda
Copy link

Somebody working on this? The PR is quiet. If you want i can take it! @aweary @spirosikmd

@spirosikmd
Copy link
Contributor

Hi @diegoborda! I am still waiting for an answer from @aweary on #12135. It should be ready to merge.

spirosikmd added a commit to spirosikmd/react that referenced this issue Apr 7, 2018
@spirosikmd
Do not set selection when prior selection is undefined (facebook#12062)
b4993d7 
`restoreSelection` did not account for input elements that have changed
type after the commit phase. The new `text` input supported selection
but the old `email` did not and `setSelection` was incorrectly trying to
restore `null` selection state.

We also extend input type check in selection capabilities to cover cases
where input type is `search`, `tel`, `url`, or `password`.
spirosikmd added a commit to spirosikmd/react that referenced this issue Apr 12, 2018
@spirosikmd
Do not set selection when prior selection is undefined (facebook#12062)
8b8f836 
`restoreSelection` did not account for input elements that have changed
type after the commit phase. The new `text` input supported selection
but the old `email` did not and `setSelection` was incorrectly trying to
restore `null` selection state.

We also extend input type check in selection capabilities to cover cases
where input type is `search`, `tel`, `url`, or `password`.
spirosikmd added a commit to spirosikmd/react that referenced this issue Apr 12, 2018
@spirosikmd
Do not set selection when prior selection is undefined (facebook#12062)
7ec68da 
`restoreSelection` did not account for input elements that have changed
type after the commit phase. The new `text` input supported selection
but the old `email` did not and `setSelection` was incorrectly trying to
restore `null` selection state.

We also extend input type check in selection capabilities to cover cases
where input type is `search`, `tel`, `url`, or `password`.
nhunzaker pushed a commit that referenced this issue May 30, 2018
@spirosikmd @nhunzaker
Extend input type check in selection capabilities (#12062) (#12135)
e0a03c1 
* Do not set selection when prior selection is undefined (#12062)

`restoreSelection` did not account for input elements that have changed
type after the commit phase. The new `text` input supported selection
but the old `email` did not and `setSelection` was incorrectly trying to
restore `null` selection state.

We also extend input type check in selection capabilities to cover cases
where input type is `search`, `tel`, `url`, or `password`.

* Add link to HTML spec for element types and selection

* Add reset button to ReplaceEmailInput

This commit adds a button to restore the original state of the
ReplaceEmailInput fixture so that it can be run multiple times without
refreshing the page.
@gaearon
Copy link
Collaborator

gaearon commented Jun 13, 2018

Fixed in React 16.4.1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Component: DOM Type: Bug
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
7 participants
@nhunzaker @gaearon @spirosikmd @adrianimboden @leonascimento @diegoborda @Nasir13