[TextField] onBlur event argument might be undefined

[shartte]

This seems like a revival of #9027, and causes event handlers which expect to be passed an event object to fail, since it'll be undefined.

The offending line of code is here:
https://github.com/mui-org/material-ui/blob/46af75d63b249e24872d481c99ceaa9d4a5f6e21/packages/material-ui/src/InputBase/InputBase.js#L223

This might have been introduced in the conversion to a function component, but I am unsure.
The previous fix was simply passing {} as the event object as it'll at least fix the direct issue of accessing properties of undefined.

[oliviertassinari]

@shartte This is a new logic that was introduced in #15446. I would propose one of these two options:

1. We remove the onBlur call. It's a bug in React: No blur event fired when button is disabled/removed facebook/react#9142. Should we defer the problem to them?
2. We update the TypeScript definitions to account for the fact that the event argument of blur might be missing

[eps1lon]

We update the TypeScript definitions to account for the fact that the event argument of blur might be missing

Sounds like the safer option to me.

[shartte]

Yeah, if the current behavior stays in place, it should at least be documented, if possible, also on the InputBase page (onBlur is currently absent there since it's forwarded to the native element I assume).
Previously this same issue was fixed by passing {} as the event object (see #9042). If the ref is still valid at that point, one might even try to pass a fake object ({target: ref.current}). I'd wager most onBlur handlers want to get at the value.

[oliviertassinari]

Previously this same issue was fixed by passing {} as the event object (see #9042).

@shartte I would suggest you had a closer look at the pull request. It's not an accurate fact.

@eps1lon I don't know, both options have their pros & cons. I can't resolve to one of these two options 🤷‍♂️. If you have a preference for n°2, why not!

[oliviertassinari]

@shartte Could you share with us the context in which you have noticed this behavior? I think that it will help us take a better tradeoff.

[shartte]

@oliviertassinari Ah you're right, I mistook the test code for the actual implementation. Sorry!
My onBlur handler was doing validation onBlur, which was easily skipped by checking whether the event was undefined. It was just something I wasn't expecting.

[joshwooding]

Updating the typescript definition definitely makes sense if the current behaviour stays. Perhaps there should be a section on the documentation as well? So users not using typescript could potentially look there or maybe something in the prop description is better

[oliviertassinari]

I think that we can go with option n°2 with the TypeScript update, prop type description update and linking the React issue in a comment.

We can always revert later on to option n°1 if people still get errors in production because they try to access event.x in the blur event.

[oliviertassinari]

I would propose the following diff:

diff --git a/packages/material-ui/src/InputBase/InputBase.js b/packages/material-ui/src/InputBase/InputBase.js
index e425bd307..5a18eb47d 100644
--- a/packages/material-ui/src/InputBase/InputBase.js
+++ b/packages/material-ui/src/InputBase/InputBase.js
@@ -548,7 +548,9 @@ InputBase.propTypes = {
    */
   name: PropTypes.string,
   /**
-   * @ignore
+   * Callback fired when the input is blurred.
+   *
+   * Notice that the first argument (event) might be undefined.
    */
   onBlur: PropTypes.func,
   /**

@shartte Do you want to work on it?

[abnersajr]

@oliviertassinari I saw that he didn't answer so I decided to take this one.
Although, I didn't get the point to change the typescript definition. This uses a definition from React. What changes are you expecting on this type? I'm not an expert with advanced types.

[shartte]

@abnersajr The InputBase.ts.d has the following code:
https://github.com/mui-org/material-ui/blob/master/packages/material-ui/src/InputBase/InputBase.d.ts#L50

  onBlur?: React.FocusEventHandler<HTMLInputElement | HTMLTextAreaElement>;

The React types define FocusEventHandler as:

    type FocusEventHandler<T = Element> = EventHandler<FocusEvent<T>>;
    type EventHandler<E extends SyntheticEvent<any>> = { bivarianceHack(event: E): void }["bivarianceHack"];

That definition does not allow for the function parameter to be undefined, so Material-UI is using the wrong type. It could instead use (not tested, just off the top of my head):

  onBlur?: React.EventHandler<FocusEvent<HTMLInputElement | HTMLTextAreaElement>|undefined>;

[avasuro]

@oliviertassinari Looks like this is not closed... Typescript defintion is wrong - now onBlur handler (according to d.ts file) always receive event object: https://github.com/mui/material-ui/blob/master/packages/mui-material/src/InputBase/InputBase.d.ts#L134

[thecodingcrow]

@oliviertassinari please re-open this issue. In MUI 5.11.12 the event is not marked as nullable/optional

We actually just run into an runtime error because of this, thats how I ended up on this thread

[thecodingcrow]

Okay there is a hint hidden deep inside a .d.ts file.
But the typing is not correct, it would be great if the typing could be corrected here