Adds support for browser autofill by mmintel · Pull Request #2174 · JedWatson/react-select

mmintel · 2017-11-27T10:49:02Z

Hi there,

I am preparing a fix for #1397, if you'd like to help me you can find my progress over here: https://github.com/sevenval/react-select.

What I basically changed was just updating the inputValue when the hidden input changes. To help with styling I am also adding a class "is-autofilled" in that case.

The autofilling itself is already working but I still have some styling issues. The problem is that Chrome ignores many css rules when autofill was applied. It's really hard to recreate the "natural autoComplete behaviour" because Chrome already applies the yellow background to inputs when it's just "suggesting" values, but I didn't find a way to react to those suggestions via javascript.

Also it currently only works autosize={false} because this adds a width="5px" on the input field, still have to figure out why this is needed and how I can optimize this.

Cheers!

jzaefferer · 2017-11-27T17:54:58Z

If I understand this correctly, finishing this without the autocomplete styling would be a lot easier (since that is already done)?

Recreating the native autocomplete styling seems unnecessarily complicated. It doesn't seem necessary, and getting the styling right across platforms (browser, OS) might be really hard.

Maybe we can start with a PR without the styling and get that merged (or we publish a fork for a while and test that in production). Meanwhile, do a second PR that deals with the styling. If the project owners find that unnecessary or too risky, it can be left out.

mmintel · 2017-11-28T10:02:09Z

If I remove all style changes except for the required .Select-hidden class it looks like this:

JedWatson · 2017-11-29T00:15:07Z

Thanks for working ont his @mmintel!

Just a heads up, you seem to be working in the dist and lib files; you want to be changing only the src, less and scss files (both stylesheet languages need to be kept in sync). Everything in lib and dist are build artefacts and should be excluded from PRs.

mmintel · 2017-11-29T08:57:46Z

@JedWatson I am only working on the src files, the changes from the dist files come from yarn build, but I removed those changes from my fork

coveralls · 2017-11-29T09:31:30Z

Coverage decreased (-0.2%) to 93.198% when pulling 56f4cac on sevenval:master into 75f3043 on JedWatson:master.

coveralls · 2017-11-29T09:40:36Z

Coverage decreased (-0.2%) to 93.198% when pulling 02cf4f5 on sevenval:master into 75f3043 on JedWatson:master.

coveralls · 2017-11-29T09:47:18Z

Coverage decreased (-0.2%) to 93.198% when pulling 02cf4f5 on sevenval:master into 75f3043 on JedWatson:master.

jzaefferer · 2017-11-29T14:30:42Z

If I remove all style changes except for the required .Select-hidden class it looks like this:

Gotcha, that's definitely worse. Hopefully you can get the proper styles working then.

mmintel · 2017-12-05T17:06:55Z

Turns out it's more than complicated to get the styling right, but finally I found this "hacky" way to detect autofill suggestions: https://medium.com/@brunn/detecting-autofilled-fields-in-javascript-aed598d25da7. Still working on it but it looks much better now.

mmintel · 2017-12-06T14:29:54Z

I'm already pretty close to native browser behaviour. I will now have a closer look to the differences between multiselect and autosize.

mmintel · 2017-12-12T11:09:02Z

@JedWatson @jzaefferer I pushed my recent changes including some unit tests that "should normally" work but I need to test them without a value on the select.

So I was trying to set the wrapper like this:

wrapper = createControlWithWrapper({
  value: false,
  autoComplete: 'test',
  multi: false,
  name: 'field',
  options: options,
  simpleValue: true,
});

And I am always getting this error message:

     TypeError: Cannot read property 'tagName' of null
      at Object.change (node_modules/react-dom/lib/ReactTestUtils.js:323:35)
      at simulateAutoFill (test/Select-test.js:123:22)
      at Context.<anonymous> (test/Select-test.js:660:4)

when I set it like this it works but is not what I am trying to test:

wrapper = createControlWithWrapper({
  value: 2,
  ...
});

same result when I try to change the value via wrapper.setPropsForChild.

It's crashing here:

var simulateAutoFill = (text) => {
  TestUtils.Simulate.change(getHiddenInput(instance), { target: { value: text } });
};

because getHiddenInput(instance) returns null when setting the value to false, which I dont understand because it seems like the hidden input is not even rendered in this case.. can't see any condition in the code that could lead to this result.

Help really appreciated! :)

jzaefferer

Overall looking pretty good. There's some parts I don't understand, so I hope other contributors can review this as well.

jzaefferer · 2017-12-20T11:14:20Z

 	}

+	setAutofill () {
+		const hasValue = this.getValueArray(this.props.value).length;


Why is this line here? hasValue isn't used, getValueArray doesn't seems to have any side effects.

jzaefferer · 2017-12-20T11:43:33Z

+		var options = [
+			{ value: 'UK', label: 'United Kingdom' },
+			{ value: 'ES', label: 'Spain' },
+			{ value: 'DE', label: 'Germany' },


We should probably add a few more countries here, so that users and other contributors can also test this with their specific autofill country.

jzaefferer · 2017-12-20T11:44:44Z

+	displayName: 'AutoCompleteField',
+	propTypes: {
+		label: PropTypes.string,
+		searchable: PropTypes.bool,


This isn't used. autoComplete is used, but missing

jzaefferer · 2017-12-20T11:45:47Z

+			selectValue: newValue,
+		});
+	},
+	toggleCheckbox (e) {


This looks like it was copied from another demo, but isn't needed here.

jzaefferer · 2017-12-20T11:46:50Z

+	&.is-autofill.is-focused .Select-input,
+	&.is-autofill.is-focused:not(.is-open) > .Select-control {
+		@media (-webkit-min-device-pixel-ratio:0) {
+			background-color: #faffbd;


Let's add a comment here explaining this particular color value, like

// simulate Chrome's native autofill styling

jzaefferer · 2017-12-20T11:50:09Z

 			'onOptionRef',
 			'removeValue',
 			'selectValue',
+			'setAutofill',


Looks like these four don't have to be here, since they're all called directly, and not used as event handlers. So no need for binding them.

jzaefferer · 2017-12-20T11:51:02Z

+			'clearAutofilled',
 		].forEach((fn) => this[fn] = this[fn].bind(this));

+		this.autoCompleteEnabled = !this.props.multi && this.props.autoComplete;


This looks like it should be a method that get's called where needed, since these props could change and the constructor runs only once.

jzaefferer · 2017-12-20T11:51:31Z

 		this.toggleTouchOutsideEvent(false);
 	}

+	handleAutoFillAnimation ({ target, animationName }) {


target isn't used, can it be removed?

jzaefferer · 2017-12-20T11:55:25Z

 	'aria-labelledby': PropTypes.string,  // HTML ID of an element that should be used as the label (for assistive tech)
 	arrowRenderer: PropTypes.func,        // Create drop-down caret element
 	autoBlur: PropTypes.bool,             // automatically blur the component when an option is selected
+	autoComplete: PropTypes.string,				// support for form auto completion


comment indent doesn't line up anymore

jzaefferer

Some more little stuff

jzaefferer · 2017-12-20T19:59:50Z

+var AutoCompleteField = createClass({
+	displayName: 'AutoCompleteField',
+	propTypes: {
+		autoComplete: PropTypes.string,


I think you were a bit too eager here. The label prop is still in use.

jzaefferer · 2017-12-20T20:00:21Z

+		// reveal hidden input for testing
+		document.querySelector('[autocomplete="country"]').classList.remove('Select-hidden');
+	},
+	getDefaultProps () {


While we're here, this is not needed

jzaefferer · 2017-12-20T20:01:03Z

+import createClass from 'create-react-class';
+import PropTypes from 'prop-types';
+import Select from 'react-select';
+import { COUNTRIES } from '../data/countries';


Let's use module.exports = ... in that file (or export default?!) and import countries = ... here.

I implemented it this way because other files in /data had the same structure

ok mark as resolved

jzaefferer · 2017-12-20T20:01:36Z

+		return (
+			<div className="section">
+				<h3 className="section-heading">{this.props.label} <a href="https://github.com/JedWatson/react-select/tree/master/examples/src/components/AutoComplete.js">(Source)</a></h3>
+				<div style={{ display: 'flex', justifyContent: 'flex-end' }}>


I still find this strange - its part of the example, so lets use normal layout here.

jzaefferer · 2017-12-20T20:02:47Z

 	}

+	isAutoCompleteEnabled() {
+		return !this.props.multi && this.props.autoComplete;


I wonder if this should also check for autoComplete !== 'off'

jzaefferer · 2017-12-22T14:10:36Z

This branch has conflicts that must be resolved

Could you look into those?

Also for new props a readme update would be great.

Since this is currently mostly-Chrome, do you see any way to make this work in other browsers as well?

coveralls · 2018-01-09T14:31:22Z

Coverage decreased (-1.8%) to 93.126% when pulling ce9ef64 on sevenval:master into 851d94b on JedWatson:master.

coveralls · 2018-01-09T14:37:02Z

Coverage decreased (-1.8%) to 93.126% when pulling ce9ef64 on sevenval:master into 851d94b on JedWatson:master.

mmintel · 2018-01-10T10:46:24Z

@jzaefferer I updated the README and resolved the conflicts. However I don't know how to test this with Safari and Firefox, because for me autofill doesn't work at all in those browsers. In Safari it just shows me an icon for autofill suggestions but that doesn't do anything (although the information in my contacts are filled in correctly). Firefox doesn't give me any suggestions, also there seem to be no way to edit saved autofill values in Firefox. Also tried this with other forms to validate it's not only a problem with react-select.

coveralls · 2018-01-10T10:54:03Z

Coverage decreased (-1.8%) to 93.126% when pulling 482aaf7 on sevenval:master into 851d94b on JedWatson:master.

jzaefferer

With all the unrelated changes to the README its impossible to review the relevant changes. Can you please undo those?

jzaefferer · 2018-01-10T10:57:00Z

-[![Build Status](https://travis-ci.org/JedWatson/react-select.svg?branch=master)](https://travis-ci.org/JedWatson/react-select)
-[![Coverage Status](https://coveralls.io/repos/JedWatson/react-select/badge.svg?branch=master&service=github)](https://coveralls.io/github/JedWatson/react-select?branch=master)
-[![Supported by Thinkmill](https://thinkmill.github.io/badge/heart.svg)](http://thinkmill.com.au/?utm_source=github&utm_medium=badge&utm_campaign=react-select)
+[![NPM](https://img.shields.io/npm/v/react-select.svg)](https://www.npmjs.com/package/react-select) [![CDNJS](https://img.shields.io/cdnjs/v/react-select.svg)](https://cdnjs.com/libraries/react-select) [![Build Status](https://travis-ci.org/JedWatson/react-select.svg?branch=master)](https://travis-ci.org/JedWatson/react-select) [![Coverage Status](https://coveralls.io/repos/JedWatson/react-select/badge.svg?branch=master&service=github)](https://coveralls.io/github/JedWatson/react-select?branch=master) [![Supported by Thinkmill](https://thinkmill.github.io/badge/heart.svg)](http://thinkmill.com.au/?utm_source=github&utm_medium=badge&utm_campaign=react-select)


Why did these change?

jzaefferer · 2018-01-10T10:57:43Z


-React-Select
-============
+# React-Select


Or this? Or the various other minor changes that are unrelated? I don't think we should mix a more general cleanup into this PR.

coveralls · 2018-01-11T09:45:07Z

Coverage decreased (-1.8%) to 93.126% when pulling ca4f169 on sevenval:master into 851d94b on JedWatson:master.

jzaefferer

A few more details that should improve the documentation

jzaefferer · 2018-01-11T10:28:06Z

+/>
+```
+
+You can use the `onAutoFill` callback to react to autofill state changes.


What's the usecase for that?

if you want to react to autofill state change on another level.. for example if you want to transport an is-autofilled to a parent node.

to autofill state changes, for example to adjust the styling of a wrapper component.?

coveralls · 2018-01-11T12:11:40Z

Coverage decreased (-1.8%) to 93.126% when pulling 612d15f on sevenval:master into 851d94b on JedWatson:master.

coveralls · 2018-01-11T12:23:22Z

Coverage decreased (-1.8%) to 93.126% when pulling 4323f12 on sevenval:master into 851d94b on JedWatson:master.

coveralls · 2018-01-11T13:08:31Z

Coverage decreased (-1.8%) to 93.126% when pulling 1273f3f on sevenval:master into 851d94b on JedWatson:master.

jzaefferer

Looks good to me!

mmintel · 2018-01-12T14:12:08Z

@JedWatson would you mind to review this again? We've put a lot of work into this in the last weeks and also implemented this in one of our projects successfully. The "coveralls" check is complaining about a decreased coverage but I've implemented some tests to make sure autoComplete is working so that should not be the problem. You will find a quick explanation in the README and you can try it out yourself in the examples. Please let us know :)

jzaefferer · 2018-01-15T11:41:34Z

Looks like this branch has conflicts again. Would be nice to know if this could get merged at some point soon - the longer this stays open, the more conflicts will pile up.

davidcoleman007 · 2018-01-19T16:28:50Z

@JedWatson @jzaefferer please merge this. How can I help? we need it.

davidcoleman007 · 2018-02-22T13:20:25Z

@JedWatson @jzaefferer @mmintel I have resolved the conflicts and tested it, my work is here:
https://github.com/davidcoleman007/react-select

Can we get this merged in please?

I'll raise my own PR if that is what you wish.

looks like I missed this one line after reviewing my diff remove dist restore old dist revert this Merge remote-tracking branch 'origin/master'

looks like I missed this one line after reviewing my diff remove dist restore old dist revert this Merge remote-tracking branch 'origin/master' Merge branch 'master' of https://github.com/davidcoleman007/react-select restore dist

CarlosOlave · 2018-04-23T19:01:10Z

Any updates on when this PR will get merge?

davidcoleman007 · 2018-04-23T19:48:12Z

@CarlosOlave I have raised a clean PR with no conflicts which is also still pending:
#2395

bladey · 2020-06-03T06:00:32Z

Hi all,

I'm closing this issue in favor of #2395 which is more up to date with a newer PR.

In an effort to sustain the react-select project going forward, we're closing issues that appear to now be redundant based on their age.

However, if you feel this issue is still relevant, please leave a comment and we'll do our best to get back to you, and we'll re-open it if necessary.

mmintel mentioned this pull request Nov 27, 2017

Feature Request: Support for Chrome autofill #1397

Closed

mmintel force-pushed the master branch from 2b56ab6 to a70f5ec Compare November 27, 2017 10:53

JedWatson changed the title ~~Adds support for autocomplete~~ Adds support for browser autofill Nov 29, 2017

mmintel force-pushed the master branch from a70f5ec to 56f4cac Compare November 29, 2017 09:24

mmintel force-pushed the master branch 2 times, most recently from 4dd08e5 to 02cf4f5 Compare November 29, 2017 09:37

mmintel force-pushed the master branch from 68f7cbb to 7e0bdc5 Compare December 1, 2017 10:30

jzaefferer suggested changes Dec 20, 2017

View reviewed changes

jzaefferer reviewed Dec 21, 2017

View reviewed changes

mmintel force-pushed the master branch from 5951cce to 3ac4005 Compare December 21, 2017 15:26

jzaefferer suggested changes Jan 10, 2018

View reviewed changes

mmintel force-pushed the master branch 3 times, most recently from 8952889 to ca4f169 Compare January 11, 2018 09:37

jzaefferer suggested changes Jan 11, 2018

View reviewed changes

Change input types to be hidden via CSS so autoComplete can be used

4323f12

mmintel force-pushed the master branch from 612d15f to 4323f12 Compare January 11, 2018 12:14

Update README.md

1273f3f

jzaefferer approved these changes Jan 11, 2018

View reviewed changes

davidcoleman007 added a commit to davidcoleman007/react-select that referenced this pull request Feb 22, 2018

fix merge conflicts with PR JedWatson#2174

99e4045

davidcoleman007 mentioned this pull request Feb 22, 2018

Support Chrome autofill #2395

Closed

jossmac added uncertain labels Jul 4, 2018

spencern mentioned this pull request Oct 2, 2018

Autofilling the address form does not autofill the region reactioncommerce/reaction-component-library#297

Open

sunny mentioned this pull request Jan 18, 2019

[Feature] Add autoFill prop to Select KissKissBankBank/kitten#1501

Merged

bladey removed the uncertain label May 26, 2020

bladey added v2-deprecated and removed v1-deprecated labels Jun 3, 2020

bladey closed this Jun 3, 2020

Conversation

mmintel commented Nov 27, 2017

Uh oh!

jzaefferer commented Nov 27, 2017

Uh oh!

mmintel commented Nov 28, 2017

Uh oh!

JedWatson commented Nov 29, 2017

Uh oh!

mmintel commented Nov 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Nov 29, 2017

Uh oh!

coveralls commented Nov 29, 2017

Uh oh!

coveralls commented Nov 29, 2017

Uh oh!

jzaefferer commented Nov 29, 2017

Uh oh!

mmintel commented Dec 5, 2017

Uh oh!

mmintel commented Dec 6, 2017

Uh oh!

mmintel commented Dec 12, 2017

Uh oh!

jzaefferer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jzaefferer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jzaefferer commented Dec 22, 2017

Uh oh!

coveralls commented Jan 9, 2018

Uh oh!

coveralls commented Jan 9, 2018

Uh oh!

mmintel commented Jan 10, 2018

Uh oh!

coveralls commented Jan 10, 2018

Uh oh!

jzaefferer left a comment

Choose a reason for hiding this comment

mmintel commented Nov 29, 2017 •

edited

Loading

davidcoleman007 commented Jan 19, 2018 •

edited

Loading