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

spirosikmd · 2018-02-01T20:12:40Z

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.

Fix #12062

aweary · 2018-02-01T21:19:16Z

fixtures/dom/src/components/fixtures/text-inputs/index.js

@@ -110,6 +114,37 @@ class TextInputFixtures extends React.Component {
          <InputTestCase type="url" defaultValue="" />
        </TestCase>

+        <TestCase


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

I was in doubt about the implementation. Thanks for the guideline!

aweary · 2018-02-01T21:21:16Z

packages/react-dom/src/client/ReactInputSelection.js

@@ -26,7 +26,8 @@ export function hasSelectionCapabilities(elem) {
  const nodeName = elem && elem.nodeName && elem.nodeName.toLowerCase();
  return (
    nodeName &&
-    ((nodeName === 'input' && elem.type === 'text') ||
+    ((nodeName === 'input' &&
+      ['text', 'email', 'tel'].indexOf(elem.type) >= 0) ||


Instead of defining an inline array, let's just add some additional cases to the boolean expression.

( nodeName === 'input' && (elem.type === 'text' || elem.type === 'email' || elem.type === 'tel') )

aweary

Thanks for doing this @spirosikmd!

spirosikmd · 2018-02-07T17:28:26Z

Hi @aweary! It would be great if you can have another look at this one!

aweary

Hey @spirosikmd,

Circling back to this, this change isn't correct. hasSelectionCapabilities should only return true if the element actually has selection capabilities and email inputs do not.

This is clearly stated in the "do not apply" table in the HTML spec. The only inputs that have selection capabilities are text, search, password, and tel. We don't account for any of those except text so we're potentially missing out on restoring selection information in some cases. Let's add those types to account for that.

The root issue of the bug reported in #12062 is that restoreSelection does not account for input elements that may have changed type after the commit phase. If the new input type supports selection state (text) but the old type didn't (email) it will incorrectly attempt to restore null selection state

react/packages/react-dom/src/client/ReactInputSelection.js

Lines 54 to 57 in 1fd205a

    
           if (curFocusedElem !== priorFocusedElem && isInDocument(priorFocusedElem)) { 
        
             if (hasSelectionCapabilities(priorFocusedElem)) { 
        
               setSelection(priorFocusedElem, priorSelectionRange); 
        
             }

What we need to do is also check if priorSelectionRange isn't null before calling setSelection

if (priorSelectionRange !== null && hasSelectionCapabilities(priorFocusedElem)) {
      setSelection(priorFocusedElem, priorSelectionRange);
    }

Could you make these changes please @spirosikmd? Thanks!

spirosikmd · 2018-02-15T20:34:54Z

Hey @aweary!

Thank you for the tip! I added the url type as well. According to the HTML spec, url does have selection capabilities.

Regarding the scripts/rollup/results.json. Should I revert my changes to avoid conflicts? Or is it a requirement for each PR to update this file?

aweary · 2018-04-07T00:36:00Z

Thanks @spirosikmd, this looks good to me. I'd like to hear from @nhunzaker and/or @jquense on this as well before merging.

Regarding the scripts/rollup/results.json. Should I revert my changes to avoid conflicts? Or is it a requirement for each PR to update this file?

You should try rebasing and running the build command again, to get an updated report without conflicts.

nhunzaker · 2018-04-07T15:38:50Z

packages/react-dom/src/client/ReactInputSelection.js

+        elem.type === 'search' ||
+        elem.type === 'tel' ||
+        elem.type === 'url' ||
+        elem.type === 'password')) ||


Why is email not in this list?

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?

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.

nhunzaker

This looks good to me. I think it would be great to leave a comment about why we specifically compare these input types, but I'm happy with the approach.

pull-bot · 2018-04-12T18:57:11Z

ReactDOM: size: 🔺+0.1%, gzip: 🔺+0.1%

Details of bundled changes.

Comparing: 5dfbfe9...3425970

react-dom

File	Filesize Diff	Gzip Diff	Prev Size	Current Size	Prev Gzip	Current Gzip	ENV
react-dom.development.js	+0.1%	+0.1%	609.28 KB	609.6 KB	140.41 KB	140.52 KB	UMD_DEV
react-dom.production.min.js	🔺+0.1%	🔺+0.1%	100.18 KB	100.26 KB	31.97 KB	31.99 KB	UMD_PROD
react-dom.development.js	+0.1%	+0.1%	593.65 KB	593.97 KB	136.25 KB	136.37 KB	NODE_DEV
react-dom.production.min.js	🔺+0.1%	🔺+0.1%	98.64 KB	98.72 KB	31.11 KB	31.13 KB	NODE_PROD
ReactDOM-dev.js	+0.1%	+0.1%	618.04 KB	618.42 KB	139.01 KB	139.12 KB	FB_DEV
ReactDOM-prod.js	🔺+0.1%	🔺+0.1%	284.83 KB	285.02 KB	52.32 KB	52.35 KB	FB_PROD

Generated by 🚫 dangerJS

gaearon · 2018-04-12T19:08:55Z

scripts/rollup/results.json

-      "size": 467421,
-      "gzip": 99671
+      "size": 467747,
+      "gzip": 99762
    },
    {
      "filename": "ReactNativeRenderer-prod.js",
      "bundleType": "RN_PROD",
      "packageName": "react-native-renderer",


Can you please revert this file?

`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 · 2018-04-17T20:12:16Z

Hi @aweary! This should be ready to merge!

nhunzaker · 2018-05-15T22:27:04Z

The requested feedback checks out for me. @aweary what say you?

nhunzaker · 2018-05-15T22:28:24Z

@spirosikmd I'll check in to make sure this is still on the team's radar if we haven't come to consensus by next week.

Thanks for your contribution!

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.

nhunzaker · 2018-05-30T10:53:58Z

This looks great to me. I pushed a commit that adds a reset button to the test fixture so that the test can be run multiple times without refreshing the page. That also covers the inverse case.

I'll wait for CI, then merge.

spirosikmd · 2018-05-30T11:08:18Z

Great! Thank you @nhunzaker!

nhunzaker · 2018-05-30T11:08:26Z

Thanks, @spirosikmd!

193: Update dependency react to v16.4.1 r=magopian a=renovate[bot] This Pull Request updates dependency [react](https://github.com/facebook/react) from `v16.4.0` to `v16.4.1` <details> <summary>Release Notes</summary> ### [`v16.4.1`](https://github.com/facebook/react/blob/master/CHANGELOG.md#1641-June-13-2018) [Compare Source](facebook/react@v16.4.0...v16.4.1) ##### React * You can now assign `propTypes` to components returned by `React.ForwardRef`. ([@bvaughn] in [#12911](`https://github.com/facebook/react/pull/12911`)) ##### React DOM * Fix a crash when the input `type` changes from some other types to `text`. ([@spirosikmd] in [#12135](`https://github.com/facebook/react/pull/12135`)) * Fix a crash in IE11 when restoring focus to an SVG element. ([@ThaddeusJiang] in [#12996](`https://github.com/facebook/react/pull/12996`)) * Fix a range input not updating in some cases. ([@Illu] in [#12939](`https://github.com/facebook/react/pull/12939`)) * Fix input validation triggering unnecessarily in Firefox. ([@nhunzaker] in [#12925](`https://github.com/facebook/react/pull/12925`)) * Fix an incorrect `event.target` value for the `onChange` event in IE9. ([@nhunzaker] in [#12976](`https://github.com/facebook/react/pull/12976`)) * Fix a false positive error when returning an empty `<React.Fragment />` from a component. ([@philipp-spiess] in [#12966](`https://github.com/facebook/react/pull/12966`)) ##### React DOM Server * Fix an incorrect value being provided by new context API. ([@ericsoderberghp] in [#12985](`https://github.com/facebook/react/pull/12985`), [@gaearon] in [#13019](`https://github.com/facebook/react/pull/13019`)) ##### React Test Renderer * Allow multiple root children in test renderer traversal API. ([@gaearon] in [#13017](`https://github.com/facebook/react/pull/13017`)) * Fix `getDerivedStateFromProps()` in the shallow renderer to not discard the pending state. ([@fatfisz] in [#13030](`https://github.com/facebook/react/pull/13030`)) --- </details> --- This PR has been generated by [Renovate Bot](https://renovatebot.com). Co-authored-by: Renovate Bot <bot@renovateapp.com>

25: Update react monorepo to v16.4.1 r=renovate[bot] a=renovate[bot] This Pull Request renovates the package group "react monorepo". - [react-dom](https://github.com/facebook/react) (`dependencies`): from `16.4.0` to `16.4.1` - [react](https://github.com/facebook/react) (`dependencies`): from `16.4.0` to `16.4.1` # Release Notes <details> <summary>facebook/react</summary> ### [`v16.4.1`](https://github.com/facebook/react/blob/master/CHANGELOG.md#1641-June-13-2018) [Compare Source](facebook/react@v16.4.0...v16.4.1) ##### React * You can now assign `propTypes` to components returned by `React.ForwardRef`. ([@bvaughn] in [#12911](`https://github.com/facebook/react/pull/12911`)) ##### React DOM * Fix a crash when the input `type` changes from some other types to `text`. ([@spirosikmd] in [#12135](`https://github.com/facebook/react/pull/12135`)) * Fix a crash in IE11 when restoring focus to an SVG element. ([@ThaddeusJiang] in [#12996](`https://github.com/facebook/react/pull/12996`)) * Fix a range input not updating in some cases. ([@Illu] in [#12939](`https://github.com/facebook/react/pull/12939`)) * Fix input validation triggering unnecessarily in Firefox. ([@nhunzaker] in [#12925](`https://github.com/facebook/react/pull/12925`)) * Fix an incorrect `event.target` value for the `onChange` event in IE9. ([@nhunzaker] in [#12976](`https://github.com/facebook/react/pull/12976`)) * Fix a false positive error when returning an empty `<React.Fragment />` from a component. ([@philipp-spiess] in [#12966](`https://github.com/facebook/react/pull/12966`)) ##### React DOM Server * Fix an incorrect value being provided by new context API. ([@ericsoderberghp] in [#12985](`https://github.com/facebook/react/pull/12985`), [@gaearon] in [#13019](`https://github.com/facebook/react/pull/13019`)) ##### React Test Renderer * Allow multiple root children in test renderer traversal API. ([@gaearon] in [#13017](`https://github.com/facebook/react/pull/13017`)) * Fix `getDerivedStateFromProps()` in the shallow renderer to not discard the pending state. ([@fatfisz] in [#13030](`https://github.com/facebook/react/pull/13030`)) --- </details> --- This PR has been generated by [Renovate Bot](https://renovatebot.com). Co-authored-by: Renovate Bot <bot@renovateapp.com>

facebook-github-bot added the CLA Signed label Feb 1, 2018

spirosikmd force-pushed the email-input-selection branch from e9ce874 to fac6ec0 Compare February 1, 2018 20:39

aweary reviewed Feb 1, 2018

View reviewed changes

aweary suggested changes Feb 1, 2018

View reviewed changes

spirosikmd force-pushed the email-input-selection branch 2 times, most recently from 14771a7 to aa115f7 Compare February 2, 2018 22:34

mihalskiy approved these changes Feb 2, 2018

View reviewed changes

spirosikmd force-pushed the email-input-selection branch from aa115f7 to c9515d3 Compare February 4, 2018 17:34

aweary suggested changes Feb 15, 2018

View reviewed changes

aweary self-assigned this Feb 15, 2018

spirosikmd force-pushed the email-input-selection branch from c9515d3 to bc7d887 Compare February 15, 2018 20:29

spirosikmd mentioned this pull request Apr 6, 2018

Problem when input type changes from email to text #12062

Closed

aweary approved these changes Apr 7, 2018

View reviewed changes

aweary requested review from jquense and nhunzaker April 7, 2018 00:34

spirosikmd force-pushed the email-input-selection branch from bc7d887 to b4993d7 Compare April 7, 2018 09:12

nhunzaker reviewed Apr 7, 2018

View reviewed changes

nhunzaker approved these changes Apr 7, 2018

View reviewed changes

spirosikmd force-pushed the email-input-selection branch from 9af22e3 to 8296f94 Compare April 12, 2018 18:48

gaearon reviewed Apr 12, 2018

View reviewed changes

spirosikmd added 2 commits April 12, 2018 21:28

Add link to HTML spec for element types and selection

a96034f

spirosikmd force-pushed the email-input-selection branch from 8296f94 to a96034f Compare April 12, 2018 19:28

Add reset button to ReplaceEmailInput

3425970

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.

nhunzaker merged commit e0a03c1 into facebook:master May 30, 2018

spirosikmd deleted the email-input-selection branch May 30, 2018 11:12

dependencies bot mentioned this pull request Oct 1, 2018

Update react-dom in /modules/frontend from 16.3.0 to 16.5.2 project-accelerate/accelerate#277

Open

This was referenced Nov 1, 2018

Update react-dom in / from 16.2.0 to 16.6.0 leonardofaria/leozera-ui#253

Closed

Update react in / from 16.2.0 to 16.6.0 leonardofaria/leozera-ui#254

Closed

This was referenced Dec 1, 2018

Update react-test-renderer in /modules/frontend from 16.3.0 to 16.6.3 project-accelerate/accelerate#312

Open

Update react in /modules/frontend from 16.3.0 to 16.6.3 project-accelerate/accelerate#313

Open

This was referenced Mar 15, 2021

chore(deps): bump react-dom from 16.4.0 to 16.4.2 in /intro-react-v5 samuelsilvadev/front-end-master-courses#9

Merged

Bump react-dom from 16.4.0 to 16.4.2 in /client cdhenry/pivot#1

Open

This was referenced Sep 5, 2024

[Snyk] Fix for 104 vulnerabilities Omrisnyk/lock-walker#65

Open

[Snyk] Fix for 104 vulnerabilities Omrisnyk/lock-walker#66

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

spirosikmd commented Feb 1, 2018 •

edited

Loading

aweary Feb 1, 2018

spirosikmd Feb 1, 2018

aweary Feb 1, 2018

aweary left a comment

spirosikmd commented Feb 7, 2018

aweary left a comment •

edited

Loading

spirosikmd commented Feb 15, 2018

aweary commented Apr 7, 2018

nhunzaker Apr 7, 2018

nhunzaker Apr 7, 2018

spirosikmd Apr 7, 2018

nhunzaker left a comment

pull-bot commented Apr 12, 2018 •

edited

Loading

react-dom

gaearon Apr 12, 2018

spirosikmd commented Apr 17, 2018

nhunzaker commented May 15, 2018

nhunzaker commented May 15, 2018

nhunzaker commented May 30, 2018

spirosikmd commented May 30, 2018

nhunzaker commented May 30, 2018 •

edited

Loading

	if (curFocusedElem !== priorFocusedElem && isInDocument(priorFocusedElem)) {
	if (hasSelectionCapabilities(priorFocusedElem)) {
	setSelection(priorFocusedElem, priorSelectionRange);
	}

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

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

Conversation

spirosikmd commented Feb 1, 2018 • edited Loading

aweary Feb 1, 2018

Choose a reason for hiding this comment

spirosikmd Feb 1, 2018

Choose a reason for hiding this comment

aweary Feb 1, 2018

Choose a reason for hiding this comment

aweary left a comment

Choose a reason for hiding this comment

spirosikmd commented Feb 7, 2018

aweary left a comment • edited Loading

Choose a reason for hiding this comment

spirosikmd commented Feb 15, 2018

aweary commented Apr 7, 2018

nhunzaker Apr 7, 2018

Choose a reason for hiding this comment

nhunzaker Apr 7, 2018

Choose a reason for hiding this comment

spirosikmd Apr 7, 2018

Choose a reason for hiding this comment

nhunzaker left a comment

Choose a reason for hiding this comment

pull-bot commented Apr 12, 2018 • edited Loading

react-dom

gaearon Apr 12, 2018

Choose a reason for hiding this comment

spirosikmd commented Apr 17, 2018

nhunzaker commented May 15, 2018

nhunzaker commented May 15, 2018

nhunzaker commented May 30, 2018

spirosikmd commented May 30, 2018

nhunzaker commented May 30, 2018 • edited Loading

spirosikmd commented Feb 1, 2018 •

edited

Loading

aweary left a comment •

edited

Loading

pull-bot commented Apr 12, 2018 •

edited

Loading

nhunzaker commented May 30, 2018 •

edited

Loading