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

Extend input type check in selection capabilities (#12062) #12135

Merged
merged 3 commits into from May 30, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
@@ -0,0 +1,40 @@
import Fixture from '../../Fixture';

const React = window.React;

class ReplaceEmailInput extends React.Component {
state = {
formSubmitted: false,
};

onReset = () => {
this.setState({formSubmitted: false});
};

onSubmit = event => {
event.preventDefault();
this.setState({formSubmitted: true});
};

render() {
return (
<Fixture>
<form className="control-box" onSubmit={this.onSubmit}>
<fieldset>
<legend>Email</legend>
{!this.state.formSubmitted ? (
<input type="email" />
) : (
<input type="text" disabled={true} />
)}
</fieldset>
</form>
<button type="button" onClick={this.onReset}>
Reset
</button>
</Fixture>
);
}
}

export default ReplaceEmailInput;
16 changes: 16 additions & 0 deletions fixtures/dom/src/components/fixtures/text-inputs/index.js
Expand Up @@ -2,6 +2,7 @@ import Fixture from '../../Fixture';
import FixtureSet from '../../FixtureSet';
import TestCase from '../../TestCase';
import InputTestCase from './InputTestCase';
import ReplaceEmailInput from './ReplaceEmailInput';

const React = window.React;

Expand Down Expand Up @@ -110,6 +111,21 @@ class TextInputFixtures extends React.Component {
<InputTestCase type="url" defaultValue="" />
</TestCase>

<TestCase
Copy link
Contributor

Choose a reason for hiding this comment

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

For new test cases that are stateful, can you break it out into it's own component? See NumberInputExtraZeroes.js as an example.

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 was in doubt about the implementation. Thanks for the guideline!

title="Replacing email input with text disabled input"
relatedIssues="12062">
<TestCase.Steps>
<li>Type "test@test.com"</li>
<li>Press enter</li>
</TestCase.Steps>

<TestCase.ExpectedResult>
There should be no selection-related error in the console.
</TestCase.ExpectedResult>

<ReplaceEmailInput />
</TestCase>

<TestCase title="All inputs" description="General test of all inputs">
<InputTestCase type="text" defaultValue="Text" />
<InputTestCase type="email" defaultValue="user@example.com" />
Expand Down
17 changes: 15 additions & 2 deletions packages/react-dom/src/client/ReactInputSelection.js
Expand Up @@ -22,11 +22,21 @@ function isInDocument(node) {
* Input selection module for React.
*/

/**
* @hasSelectionCapabilities: we get the element types that support selection
* from https://html.spec.whatwg.org/#do-not-apply, looking at `selectionStart`
* and `selectionEnd` rows.
*/
export function hasSelectionCapabilities(elem) {
const nodeName = elem && elem.nodeName && elem.nodeName.toLowerCase();
return (
nodeName &&
((nodeName === 'input' && elem.type === 'text') ||
((nodeName === 'input' &&
(elem.type === 'text' ||
elem.type === 'search' ||
elem.type === 'tel' ||
elem.type === 'url' ||
elem.type === 'password')) ||
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is email not in this list?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry. That was very blunt! Should email be in here? I found a neat table from a spec:
https://html.spec.whatwg.org/multipage/input.html#concept-input-apply

So this matches up with the setSelectionRange column? Dang. I really want to know the reasoning behind not supporting this for all text based input.

Would it be possible to add a comment at the top of this block that references this table for future contributors?

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 will add a comment containing the link as well. As far as I can see we are only using the selectionStart and selectionEnd in the code. So I guess we match up with these two rows.

nodeName === 'textarea' ||
elem.contentEditable === 'true')
);
Expand All @@ -52,7 +62,10 @@ export function restoreSelection(priorSelectionInformation) {
const priorFocusedElem = priorSelectionInformation.focusedElem;
const priorSelectionRange = priorSelectionInformation.selectionRange;
if (curFocusedElem !== priorFocusedElem && isInDocument(priorFocusedElem)) {
if (hasSelectionCapabilities(priorFocusedElem)) {
if (
priorSelectionRange !== null &&
hasSelectionCapabilities(priorFocusedElem)
) {
setSelection(priorFocusedElem, priorSelectionRange);
}

Expand Down