[WIP] [AutoComplete] Bubble onChange, onFocus, onBlur events

[newoga]

Fixes #2294

Note: This PR does not implement the fix yet. I just wrote tests in auto-complete-spec.js that demonstrate that this is not currently supported.

Earlier, I wrote a naive implementation for this, but noticed that the way focus state is currently being handled for this component is leading to some interesting behavior. Sometimes blur and focus events are being called multiple times for a single action such as clicking away from the options menu. The way the focusing is currently being handled may be a root cause to other issues as well such as #2295 and #2299.

Will look more into this...

Also, on a completely separate note, is it alright if we add a dev dependency to react-addons-test-utils since react/addons is deprecated?

[oliviertassinari]

Also, on a completely separate note, is it alright if we add a dev dependency to react-addons-test-utils since react/addons is deprecated?

Yes it's 👍, you can event update the other test files if you want to.

[newoga]

Just an update on this, I noticed that the main problem with the current implementation is using <Menu /> to display options/results. The current <Menu /> implementation forcibly sets it's DOM focus on the menu div. This is causing the input to lose focus for the autocomplete.

I'm going to recommend switching <Menu />/<MenuItem /> for/ which doesn't set the focus.

I'll give this a go tonight...

[oliviertassinari]

I fear that I don't follow.
Can't we just bubble up the event here https://github.com/callemall/material-ui/blob/master/src/auto-complete.jsx#L222?

[newoga]

@oliviertassinari , yes, essentially that will be where we bubble the events. However, right now the events are being called several times which may make working with them externally unnatural. For example, after debugging I noticed when you click on a <AutoComplete showAllItems={true} /> component, the following happens:

(1) onFocus is called on TextField (triggers options to show)
(2) When the options show, the <Menu /> component forcibly calles focus on the DOM element, making the TextField lose focus and call onBlur (https://github.com/callemall/material-ui/blob/master/src/menu/menu.jsx#L72)
(3) onBlur is called on the TextField, the onBlur method calls focus back on the TextField DOM element (https://github.com/callemall/material-ui/blob/master/src/auto-complete.jsx#L224)

Also, each time you make a change to the TextField, onBlur and onFocus are called again.

What really should be happening is regardless of when options are showing or not, onBlur shouldn't be called unless you leave the input, and onFocus should only be called once when you select the input.

To properly support things like redux-form, we don't want to bubble unnecessary events.

Sorry for the long explanation! Let me know if that makes any sense...

[oliviertassinari]

Sorry for the long explanation

Thanks for this long explanation.
Looks like something we should really fix!

[newoga]

tl;dr If we simply bubble the events on those methods, events onFocus and onBlur events get called each time the input changes which should not be occurring.

The proper behavior should onFocus called once when you select input, onBlur called once when you leave input, onChange once when it detects the value has changes, and optionally we could add a custom prop onInputChange each time the input value changes...

[newoga]

Hey @oliviertassinari ,

Sorry for the delay in pushing this! This definitely needs feedback :P. I'm not thrilled about the current implementation / what I had to change, but this latest commit fixes #2299, #2294, #2297

The main changes that facilitated those fixes were:

• Switched out <Menu /> and <MenuItem /> for displaying results with <List /> and <ListItem />.
• Re-implement concept of "focused/selection option" so that you could still properly use the keyboard to select an item (not supported by List/ListItem)
• Created a new onChange event that distinguishes itself from the old onUpdateInput event. onChange gets called when a new value is set, onUpdateInput gets called whenever the search value changes from the user (as he or she is typing)

It's worth mentioning that there are several props that are defined on this component that now have no meaning or effect because we are no longer using <Menu />. Also, menu animation is currently broken because <List /> and <ListItem /> do not support animations like menu does.

That being said, making these changes makes the events work a lot more naturally. onBlur and onFocus are now only getting called once, you can also now properly tab through the fields. onChange gets called only when a new value is actually selected. Support for things like redux-form should work fine now.

I think there are a lot of other things that we could do to improve overall API and code but wanted to push this as is as this might be the best I can do for now in terms of maintaining the original design and props/backwards compatibilty.

Let me know what you think!

Also, I added some more documentation and created some new examples that I think better illustrate how the component works.

[alitaheri]

@newoga Please add onFocus too

[newoga]

Will do, thanks @subjectix!

[alitaheri]

I think this component should use Popover too, but that's probably for later. This is good in my knowledge, I leave the review to @oliviertassinari and the author of auto-complete @yongxu. Please review this when you have the time.

[oliviertassinari]

Switched out <Menu /> and <MenuItem /> for displaying results with <List /> and <ListItem />

We plan to use the new Menu implementation for our DropDownMenu component.
Hence, I would rather fix that Menu component than switch it out for the List.
And I feel like that's not a regular use case for the List component.
What do you guys think?

[newoga]

@oliviertassinari, I agree. When I initially started making the switch, I underestimated the impact of the switch to List from Menu would cause, but finished the pull request anyway.

I think fixing Menu is the better approach. I haven't had a chance to look into what can be done there, but it's certainly some I can look into if we agree that's what we should do.

[newoga]

Can I get your feed back on this set of lines: https://github.com/callemall/material-ui/blob/master/src/lists/list-item.jsx#L379-L397

I'm trying to get a better understanding of if there is a important reason why we are using DOM focus on the ListItem here and why we can't manage it using component state. It seems like Menu and MenuItem uses to manage focus state for keyboard/select control and is the prime reason why our input control is getting/losing control of focus when using the Menu/rendering options. Based on my current understanding, if we manage focus state in the component instead of using DOM focus, maybe we can get the Menu to work with this AutoComplete. Not completely sure though...

As a general rule though, it may be good to design material-ui to not have dependencies on ReactDOM, especially if react-native support is something we add. Along with mixins, imports of ReactDOM might be something we want to remove a general road map item.

Thoughts?

[yongxu]

Hey @newoga,
Thanks for your great work!

Personally, I think we shouldn't

Switched out <Menu /> and <MenuItem /> for displaying results with <List /> and <ListItem />

Menu has some good properties, such as clone item before display, has scroll implemented, and some extra animations. Menu can work quite well with AutoComplete and they both have some similar functions that can eventually be merged into Menu.

Also, in your pull request, supporting component item has been removed.

 <AutoComplete
+  dataSource={fruit}
+  errorText={this.state.errorText}
+  floatingLabelText="Error Text Example"
   fullWidth={true}
-  hintText="text-value data"
-  onUpdateInput={(t) => {
-    console.log(t);
-  }}
-  showAllItems={true}
-  dataSource={[
-    {
-      text: 'text-value1',
-      value:(<AutoComplete.Item primaryText={'text-value1'} secondaryText="&#9786;" />),
-    },
-    {
-      text: 'text-value2',
-      value:(<AutoComplete.Item primaryText={'text-value2'} secondaryText="&#9786;" />),
-    },
-  ]}
-  onNewRequest={(t, index) => { console.log('request:' + index); }} />

I personally think we should allow accepting object array and let components to be used as item, so customized component can be applied.

Moreover, remove this in new PR might break someone's code if they are already using it in development.

I also had some thought such as other properties add be added by using object array, such as disabled, allwaysVisible, index //allways show up at this index order, caseSensitive, filter, clickCallback or keyword // alternative search key. They just have not been implemented at this point.

AutoComplete should not only be used as input complete, but also as search bar that can potentially show anything as its item, which was the initial motivation for me to implement this. I was thinking about something like google search bar rather than a state picker for address input.

@oliviertassinari @newoga , I know the current Menu is problematic, not fit well with AutoComplete. It is quite hard to deal with the focus and my current solution with onBlur was just a simple work around, not really an ideal solution, But I believe it is better to improve on Menu instead to replace it with List.

@subjectix , Yes PopOver might be a better choice, I'd love to see that improvement and get rid of the ReactTransitionGroup. However I think it is probably better to let Menu have popup options so that we can use it directly in AutoComplete.

I might be wrong on this, using List does make the code much easier to maintain and clear. I'd like to see more ideas about replace Menu with List.

[oliviertassinari]

@newoga Having a look at https://www.google.com/design/spec/components/lists-controls.html#lists-controls-types-of-menu-controls.

A menu is a special type of list

I think that we should really use a menu.
It's not the case right now, but I think that ideally, the menu implementation would use the list.

Having said that, it seems clear to me that we should fix this menu focus/blur issue.
@hai-cea would be the best person to do so since he is the one how implemented it (44db2ab).

[newoga]

@oliviertassinari , @yongxu - Yup, I'm in agreement that we should not use <List /> and <ListItem/ > in this component and keep using <Menu /> and <MenuItem />. 👍

I agree that the right approach to fixing this is finding a way to fix focus state in <Menu /> 👍

We should not merge this PR (I kept it as WIP) but we can ultimately close it if that's best.

I haven't too much time to look into approaches to fixing menu, but as I mentioned in an earlier comment, the real problem is that there are methods on the ListItem component that control DOM focus state that menu use. Ironically, those methods aren't called when ListItem is rendered by itself without menu.

Once again though, in general, I think we should remove all the instances of ReactDOM and elem.focus() in our components and find a different approach to managing focus. That's going to be a best bet long term to supporting things like react-native.

I believe we should make focus state something that is handled by a parent component, and make the List and Menu components more stateless.

[oliviertassinari]

Once again though, in general, I think we should remove all the instances of ReactDOM and elem.focus() in our components and different approach to manage focus. That's going to be a best bet long term to supporting things like react-native.

I believe we should make focus state something that is handled by a parent component, and make the List and Menu components more stateless.

I agree.

I haven't too much time to look into approaches to fixing menu

Thank any way for this investigation.
We have a far better understanding of this issue.

[newoga]

@oliviertassinari , my gut feeling is to maybe close this PR for now?

The only thing I'd want to keep maybe are the tests I wrote and documentation. But on other hand, I know we're going through a documentation overhaul in #2433 , so maybe it makes sense to scrap and do some new docs there. And the tests are currently failing until we can fix some of the menu stuff..mmm

[oliviertassinari]

Thanks anyway!

[newoga]

No problem!

[yongxu]

@newoga Really like the tests and documents you wrote. We should definitely merge them 👍

Once again though, in general, I think we should remove all the instances of ReactDOM and elem.focus() in our components and find a different approach to managing focus. That's going to be a best bet long term to supporting things like react-native.

I totally agree with you on this. Plus I think we should remove anything that securely changes the focus, or at least require explicit parameter to turn it on. They are quite annoying sometime and may cause weird behaviors sometime, very hard to work around without touching the component's code.