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

Adds support for browser autofill #2174

Closed
wants to merge 2 commits into from
Closed

Conversation

mmintel
Copy link

@mmintel mmintel commented Nov 27, 2017

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.

nov-27-2017 11-40-48

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
Copy link

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
Copy link
Author

mmintel commented Nov 28, 2017

If I remove all style changes except for the required .Select-hidden class it looks like this:
nov-28-2017 11-01-09

@JedWatson
Copy link
Owner

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.

@JedWatson JedWatson changed the title Adds support for autocomplete Adds support for browser autofill Nov 29, 2017
@mmintel
Copy link
Author

mmintel commented Nov 29, 2017

@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
Copy link

Coverage Status

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

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

Coverage Status

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

@coveralls
Copy link

Coverage Status

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

@jzaefferer
Copy link

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
Copy link
Author

mmintel commented Dec 5, 2017

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
Copy link
Author

mmintel commented Dec 6, 2017

I'm already pretty close to native browser behaviour. I will now have a closer look to the differences between multiselect and autosize.
dec-06-2017 15-28-32

@mmintel
Copy link
Author

mmintel commented Dec 12, 2017

@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! :)

Copy link

@jzaefferer jzaefferer left a comment

Choose a reason for hiding this comment

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

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

src/Select.js Outdated
this.setState({
isOpen: false,
inputValue: this.handleInputValueChange(''),
}, this.focus);
}

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

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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

&.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;

Choose a reason for hiding this comment

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

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

// simulate Chrome's native autofill styling

src/Select.js Outdated
@@ -59,10 +64,17 @@ class Select extends React.Component {
'onOptionRef',
'removeValue',
'selectValue',
'setAutofill',

Choose a reason for hiding this comment

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

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.

src/Select.js Outdated
].forEach((fn) => this[fn] = this[fn].bind(this));

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

Choose a reason for hiding this comment

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

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.

src/Select.js Outdated
@@ -147,6 +162,22 @@ class Select extends React.Component {
this.toggleTouchOutsideEvent(false);
}

handleAutoFillAnimation ({ target, animationName }) {

Choose a reason for hiding this comment

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

target isn't used, can it be removed?

src/Select.js Outdated
@@ -1084,6 +1185,7 @@ Select.propTypes = {
'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

Choose a reason for hiding this comment

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

comment indent doesn't line up anymore

Copy link

@jzaefferer jzaefferer left a comment

Choose a reason for hiding this comment

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

Some more little stuff

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

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

Ah right

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

Choose a reason for hiding this comment

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

While we're here, this is not needed

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

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

ok mark as resolved

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' }}>

Choose a reason for hiding this comment

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

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

src/Select.js Outdated
@@ -147,6 +157,26 @@ class Select extends React.Component {
this.toggleTouchOutsideEvent(false);
}

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

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

Good idea

@jzaefferer
Copy link

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
Copy link

Coverage Status

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

@coveralls
Copy link

Coverage Status

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

@mmintel
Copy link
Author

mmintel commented Jan 10, 2018

@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
Copy link

Coverage Status

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

Copy link

@jzaefferer jzaefferer left a comment

Choose a reason for hiding this comment

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

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

README.md Outdated
[![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)

Choose a reason for hiding this comment

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

Why did these change?

README.md Outdated

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

Choose a reason for hiding this comment

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

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.

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

Coverage Status

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

Copy link

@jzaefferer jzaefferer left a comment

Choose a reason for hiding this comment

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

A few more details that should improve the documentation

README.md Outdated
/>
```

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

Choose a reason for hiding this comment

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

What's the usecase for that?

Copy link
Author

Choose a reason for hiding this comment

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

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.

Choose a reason for hiding this comment

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

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

@coveralls
Copy link

Coverage Status

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

@coveralls
Copy link

Coverage Status

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

@coveralls
Copy link

Coverage Status

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

Copy link

@jzaefferer jzaefferer left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@mmintel
Copy link
Author

mmintel commented Jan 12, 2018

@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
Copy link

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
Copy link

davidcoleman007 commented Jan 19, 2018

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

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

@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.

davidcoleman007 added a commit to davidcoleman007/react-select that referenced this pull request Feb 22, 2018
looks like I missed this one line after reviewing my diff

remove dist
restore old dist

revert this
Merge remote-tracking branch 'origin/master'
davidcoleman007 added a commit to davidcoleman007/react-select that referenced this pull request Feb 22, 2018
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
Copy link

Any updates on when this PR will get merge?

@davidcoleman007
Copy link

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

@bladey
Copy link
Contributor

bladey commented Jun 3, 2020

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.

@bladey bladey closed this Jun 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants