mui / material-ui

Material UI: Comprehensive React component library that implements Google's Material Design. Free forever.
https://mui.com/material-ui/
MIT License
93.86k stars 32.26k forks source link

[Select] Select onBlur after load when open is controlled and initially true #17843

Closed ccmartell closed 5 years ago

ccmartell commented 5 years ago

Current Behavior 😯

Select calls onBlur immediately when open={true}

Expected Behavior 🤔

Select should not call onBlur immediately when open={true}

Steps to Reproduce 🕹

We are generating a select box via portal that removes itself after a blur event or losing focus. There are instances that we would like the select menu to start open initially. When the control is set initially to true, the onBlur event is called immediately causing the select box to be removed.

CodeSandbox Issue

I can avoid calling the onBlur if I have a "loading" tag: CodeSandbox Workaround

Your Environment 🌎

Tech Version
Material-UI v3.9.2 but tested on 4.5.0 (see codesandbox)
React v16.8.6
Browser Chrome v77.0.3865.90
TypeScript No
ccmartell commented 5 years ago

I also tested this with using the effect to open the menu. It seems to call onBlur as well. So I would say that any programmatic opening of the select menu causes the onBlur event to fire.

eps1lon commented 5 years ago

Independent of the specific events: What are you trying to solve?

ccmartell commented 5 years ago

The main component we use this in is a spreadsheet component created using canvas. We use material ui form elements as a pop over single field editor to edit the information when double clicking into the field. We use the onBlur event or the clickaway event to say that the field is done editing and should be saved which removes the popover editor.

We also use this same select component in our full forms without auto opening.

If there was an instance where I would want to open the select dropdown programmatically, it would call onBlur immediately which we use to run field validations. We would want to avoid showing a "missing required field" error immediately on opening if possible.

I understand the use cases can be slim and there is a current workaround.

eps1lon commented 5 years ago

If there was an instance where I would want to open the select dropdown programmatically, it would call onBlur immediately which we use to run field validations.

It sounds like you're not really thinking in react. You don't "programmatically" (you probably meant imperatively) close the select. What you do is flip the open prop with some global state managment.

I guess my main question is why onClose isn't enough. We should already cover every close use case in blur events.

We use material ui form elements as a pop over single field editor to edit the information when double clicking into the field.

This is also not very clear to me. You're using a Select component which is a type of input where the user selects a value from a pre-defined list. Are you maybe looking for some sort of combobox? We have integration examples with popular combobox libraries in https://material-ui.com/components/autocomplete/#autocomplete and are currently working on complete autocomplete without any 3rd party library.

ccmartell commented 5 years ago

It sounds like you're not really thinking in react. You don't "programmatically" (you probably meant imperatively) close the select. What you do is flip the open prop with some global state managment.

Yes. We use the React.useState hook to control the open prop on whether or not to display the select field. We also use the useState hook to control the open prop on the menu for the select component.

Our use case is this: We have a canvas spreadsheet component that doesn't support editing. It does support events and will emit events for double clicking back up to our wrapper component we created. When the wrapper component receives a double click event, we set some variables on the shownEditField to determine location, size, variables, type, etc. When that state value is set, we display a popup directly over the field that we are editing like you would see in other spreadsheets.

Our select component is a set list that you choose one value or (if it is a multiselect) you choose multiple values.

We have a lot of customers use TAB to exit a field in a spreadsheet. So our onBlur event needs to save and close the editing popup. We have also recently received feedback that customers would like the select to automatically open when editing in the spreadsheet view (hence the need for starting the menu isOpen state to true).

In addition to using this select component as a standalone editor for a single field, we also use the field component in a full form with other fields. We do not want to start all the select fields in the form with isOpen as true and we do not have a blur event in the form to close the form.

We also have several validations being ran against the fields for required, length, value combinations, etc. We run a lot of those validations during onBlur. We then display error messages as helper text. When onBlur is called immediately as the menu opens, we have some validations run and show an error when there is none.

That being said, I am only presenting our use case and a bug that I found with the component. The onBlur gets called immediately when the open prop is initially set to true (shown in my original post codesandbox). That is unexpected behavior as you are still in the field. It is expected that the field completely loses focus before calling onBlur.

If you have additional questions, I would be happy to answer them. Our use case is unique and I doubt many other people would use a standalone field like we do.

oliviertassinari commented 5 years ago

@ccmartell Can you confirm that the onBlur behavior matches your expectations in v4.5.1 (after #17299)?

We run a lot of those validations during onBlur.

Interesting.

spreadsheet component

I would be curious to learn more about this component. What solution do you use? Did you build it from scratch? If you did built it from scratch, what features did you need and what's missing in the solution offered by the third parties? Thanks.

eps1lon commented 5 years ago

If you have additional questions, I would be happy to answer them. Our use case is unique and I doubt many other people would use a standalone field like we do.

Why is onClose not sufficient? It should cover blurring. Be aware the blur events bubble in react (unlike in the DOM). If you actually have the requirement that any blur event in the subtree should close then this means switching between items would blur.

oliviertassinari commented 5 years ago

I'm closing as the problem do no longer manifest on v4.5.1. It's not meant to stop the discussion.

ccmartell commented 5 years ago

Sorry for the delay.

@ccmartell Can you confirm that the onBlur behavior matches your expectations in v4.5.1 (after #17299)?

I can confirm that the behavior in v4.5.1 matches my expectations.

What solution do you use? Did you build it from scratch? If you did built it from scratch, what features did you need and what's missing in the solution offered by the third parties? Thanks.

We use a fork of Canvas Datagrid and our own code. The field editor was lacking for our use (we use a lot more than just strings and numbers) but we really liked the virtualization that's present through using a canvas spreadsheet rather than trying to use a react virtualization library.

Why is onClose not sufficient? It should cover blurring. Be aware the blur events bubble in react (unlike in the DOM). If you actually have the requirement that any blur event in the subtree should close then this means switching between items would blur.

Currently the issue we have with onClose running the onBlur event is that we want to see a preview of the updated values prior to closing the editor completely. This is mainly used for comboboxes that you want to select a value and then slightly change or add to the selected value.

We are planning on changing our combobox solution in the near future. The proposed solution actually allows the use of onClose running the onBlur events.

oliviertassinari commented 5 years ago

@ccmartell thanks for the answer. Regarding the combobox we would appreciate feedback on #17037.