facebook / react

The library for web and native user interfaces.
https://react.dev
MIT License
228.6k stars 46.8k forks source link

Proposition about onInput/onChange #19150

Open mr21 opened 4 years ago

mr21 commented 4 years ago

Hi :)

In ReactDom we can find:

function getTargetInstForInputOrChangeEvent(topLevelType, targetInst) {
    if (topLevelType === TOP_INPUT || topLevelType === TOP_CHANGE) {
        return getInstIfValueChanged(targetInst);
    }
}

Why not adding an extra condition here like:

function getTargetInstForInputOrChangeEvent(topLevelType, targetInst) {
    if ((!React.$$useRealOnChange && topLevelType === TOP_INPUT) || topLevelType === TOP_CHANGE) {
        return getInstIfValueChanged(targetInst);
    }
}

By checking React.$$useRealOnChange in this function, a user could add this line:

React.$$useRealOnChange = true;

anywhere in their code (before or after including ReactDom) to find back the more native behavior.

I'm sorry in advance if this proposition has already been proposed

roryokane commented 4 years ago

If I understand your proposed solution correctly, React.$$useRealOnChange would be a global variable that would change the behavior of onChange in code such as <input onChange={event => handleChange(event)} />. The goal is to easily listen to the browser’s built-in change event.

I, too, wish that React allowed me to listen to the native change event. However, I have concerns about that specific solution. I worry that adding a global variable like React.$$useRealOnChange would cause surprising bugs and complicate testing. If you set React.$$useRealOnChange to true in one part of the code, it could affects parts of the code that were written assuming a value of false for it.

I favor the solution proposed by this older issue: #14857 “Easily handle browser 'change' events”. That issue proposed supporting a new attribute on input elements that act like a browser-native onchange attribute. It suggested these names as possibilities:

That issue was automatically closed for being “stale”, so perhaps this issue should become the new active issue about accessing the browser-native onchange.

mr21 commented 4 years ago

would cause surprising bugs and complicate testing

Indeed yes, but any dependence could correct their code by changing the onchange by oninput, this would not break anything, i suppose. Anyway changing this setting would be something advanced maybe react could console.warn us at each load in dev mode idk.

I have to admit, the other solution is safer you're right.

But even without adding a secret variable, why not just expose the function responsible of that and let the possibility to redo the function as we wish.

rachelnabors commented 4 years ago

Hey hey! Could you add an example in CodeSandbox? That would really help us look into this :)

roryokane commented 4 years ago

@rachelnabors Here’s an example I made that demonstrates the desired behavior, React not providing access to that behavior, and the way I personally wish React would give us access to that behavior:

CodeSandbox demo requesting native onchange in React

Mirror of that demo page

As CodeSandbox is a third-party site I am unfamiliar with, I don’t trust it to stay up forever, so here’s a mirror of the important parts of that demo page: ### `public/index.html` > ~~~html >
> >

Demo of native onchange attribute

> >
> ~~~ > ## Demo of native `onchange` attribute > > The above number-type `input` element logs a message when its [`change` event](https://developer.mozilla.org/en-US/docs/Web/API/HTMLElement/change_event) is fired. Such an event is fired when a change is *committed*. The exact conditions of the event firing depend on the type of input element and possibly the browser, but in my testing with this particular `input` element, the `change` event seems to fire when any of the following happen: > > - The field is blurred after its value was changed. > - The user presses Enter/Return on the keyboard after changing its value. > - The user increments or decrements the number in the field by pressing Up or Down on the keyboard. ### `src/App.js` > ~~~js > export default function App() { > function handleOnChange(event) { > console.log("React change event handler called", event.target.value); > } > > return ( >
>

> Demo of React onChange attribute >

> >
> ); > } > ~~~ > ## Demo of React `onChange` attribute > > The above number `input` logs a message on every `input` event, rather than only on `change` events. This is because React’s `onChange` attribute works the same as an `onInput` attribute. > > ## Requested feature for React > > It’s too late to redefine React’s `onChange` attribute, but can we add a way to get the same behavior as HTML’s `onchange` attribute without manually handling all the cases listed in that section? > > Multiple possible solutions are discussed in [React issue \#19150](https://github.com/facebook/react/issues/19150), but the idea I currently favor is supporting a new attribute `onChangeCompleted` on `input` elements. If such an attribute existed, the following commented-out input element would work the same as in the native demo: > ~~~js > > ~~~
rachelnabors commented 4 years ago

Thanks so much for explaining your approach to me! Unfortunately, a global configuration option would force every third party component author to be compatible with both modes—which we feel would put too much strain on the ecosystem. There have been previous discussions, and if you have feedback, we'd love for you to join the conversation!

roryokane commented 4 years ago

@rachelnabors

Unfortunately, a global configuration option would force every third party component author to be compatible with both modes

I think you overlooked a large part of the demo I provided. I was not suggesting a global configuration option—that was only @mr21’s idea. In my demo, I suggested adding a new attribute to React input elements, onChangeCompleted. Such an addition would be backwards-compatible because currently that attribute is ignored (and logs a warning that it is unknown).

There have been previous discussions, and if you have feedback, we'd love for you to join the conversation!

Discussing how to listen to input change events with React is a bit confusing because there have been multiple solutions discussed in multiple places:

In which places would you like me to discuss my preferred solution of adding a backwards-compatible new attribute such as onChangeCompleted? You suggested #9657, but I’m thinking it would make the conversation clearer if you reopened issue #14857, or if I created a new issue and posted links to it in the other issues.

gaearon commented 4 years ago

I think you overlooked a large part of the demo I provided.

I believe @rachelnabors was responding to the original post proposal. Yours is indeed different.

In which places would you like me to discuss my preferred solution of adding a backwards-compatible new attribute such as onChangeCompleted?

If you have a fully-formed proposal, http://github.com/reactjs/rfcs would be a good place to submit it as a PR.

stale[bot] commented 3 years ago

This issue has been automatically marked as stale. If this issue is still affecting you, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize it yet. If you have any new additional information, please include it with your comment!

tkrotoff commented 3 years ago

Bump

AgainPsychoX commented 3 years ago

Bump

celmaun commented 3 years ago

+1 for onChangeNative, Redux goes ballistic when you dispatch actions on each key stroke 😟

stale[bot] commented 2 years ago

This issue has been automatically marked as stale. If this issue is still affecting you, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize it yet. If you have any new additional information, please include it with your comment!

roryokane commented 2 years ago

Bump. This problem has not been solved yet: in React’s documentation for onChange there is still no mention of an alternative attribute (which would have a name like onChangeCompleted or onChangeNative) that uses the browser’s native onchange event handler.

NWPoul commented 1 year ago

Definitely a bug IMO Broken useful native behavior of browser's tag without smooth backward compatible/workarounds ( Sad that this issue does not fixed yet

ahmadmirjalili commented 1 year ago

unsubscribe https://github.com/notifications/unsubscribe-auth/AUFSVP5YS762KBURJRPQK33WRI77ZANCNFSM4OBGJHDQ

On Sun, Jan 8, 2023 at 8:07 AM Pavel @.***> wrote:

Definitely a bug IMO Broken useful native behavior of browser's tag without smooth backward compatible/workarounds ( Sad that this issue does not fixed yet

— Reply to this email directly, view it on GitHub https://github.com/facebook/react/issues/19150#issuecomment-1374706395, or unsubscribe https://github.com/notifications/unsubscribe-auth/AUFSVP5YS762KBURJRPQK33WRI77ZANCNFSM4OBGJHDQ . You are receiving this because you are subscribed to this thread.Message ID: @.***>

RobMayer commented 9 months ago

throwing in a plea for some kind of "onCommit", "onChangeComplete" or similar that utilizes the native change event.

obviously one could trivially add a custom handler using the addEventListener, but like all DOM events, it won't bubble up the React Tree like a synthetic event would.

or if there was a way to add a custom SyntheticEvent that will bubble up the React Tree instead of the DOM tree, that'd work to.

github-actions[bot] commented 6 months ago

This issue has been automatically marked as stale. If this issue is still affecting you, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize it yet. If you have any new additional information, please include it with your comment!

tkrotoff commented 6 months ago

Bump

github-actions[bot] commented 3 months ago

This issue has been automatically marked as stale. If this issue is still affecting you, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize it yet. If you have any new additional information, please include it with your comment!

tkrotoff commented 3 months ago

This issue has been automatically marked as stale

I hate bots, please don't close this issue

github-actions[bot] commented 1 week ago

This issue has been automatically marked as stale. If this issue is still affecting you, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize it yet. If you have any new additional information, please include it with your comment!

RobMayer commented 1 week ago

bad bot; bump