-
Notifications
You must be signed in to change notification settings - Fork 46.5k
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
fix: support cancel event for input[type="file"] #27897
Conversation
Comparing: f1039be...a04f48a Critical size changesIncludes critical production bundles, as well as any change greater than 2%:
Significant size changesIncludes any change greater than 0.2%: Expand to show
|
Awesome fix, this looks good to me, and I confirmed it works in the codesandbox. I'd merge, but I don't know the events system super well. @sebmarkbage or @sophiebits could you give it a quick review to confirm I'm not missing any obvious issues? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one note inline, otherwise lgtm
Actually, can we check if you render
then is the dialog onCancel executed twice when canceling a file input selection? |
Nice catch @sophiebits, the dialog gets the cancel event, and for some reason adding the dialog causes multiple cancel events to fire. Here's the sandbox: https://codesandbox.io/p/sandbox/react-forked-6j48sl?file=%2Fsrc%2FApp.js Without function cancel() {
console.log("cancel");
}
return <input type="file" cancel={() => consol} />;
// cancel With function cancel() {
console.log("cancel");
}
function dontCancel() {
console.log("no");
}
return (
<dialog onCancel={dontCancel} open>
<input type="file" onCancel={cancel} />
</dialog>
);
// cancel
// no
// cancel |
9e48615
to
93cee36
Compare
When use input and dialog simultaneously and add So I prevent native cancel event being added to event queue when After my testing, it is now working normally. Here's the codesandbox: https://codesandbox.io/p/sandbox/react-forked-pyzffz?file=%2Fsrc%2FApp.js%3A12%2C22 But I'm not entirely sure if this modification is appropriate. Could you please review it again? @rickhanlonii @sophiebits |
!isAtTarget | ||
) { | ||
return; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is quite the right fix. In fact we do want the input cancel event to bubble as that's what would happen in the native browser event system and we are looking to move closer to that over time.
But it shouldn't double-trigger the higher listener. The reason that happens is that React has native listeners on the input itself as well as on the dialog; when the bubbling native event is dispatched, it triggers both listeners, and React then bubbles each with its own event system. Really I think the correct fix is probably to change listenToNonDelegatedEvent so that it only responds to events whose target is equal to the element we are adding the listener on (equivalently, ones where event.target === event.currentTarget when the native listener is fired). The intent of that function (unless I'm mistaken) is only to catch events of that kind. We should verify that all current uses of that are not meant to catch any bubbled events though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At first, I also wanted to fix it according to this idea (validate target === currentTarget
). But I found that in the original HTML, the cancel event can bubble and be fired when target !== currentTarget
(This seems to be inconsistent with the standard defined by MDN). Therefore, validating
target === currentTarget
can't solve this problem.
But my current solution is not quite correct either, because the triggering timing of eventPhase is inconsistent with the behavior of native HTML. I think this is due to placing the cancel event in nonDelegatedEvents
, causing some compatibility issues.
In fact, the actual behavior of the cancel event in the browser is not consistent with the standard defined by MDN; it behaves more like a regular event. Therefore, should we consider removing it from nonDelegatedEvents
so that it works correctly?
Here's the codesandbox: https://codesandbox.io/p/sandbox/reverent-margulis-gvgwq2?file=%2Findex.html%3A14%2C72
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I'm not mistaken, the issue is that for the dialog cancel event it does not bubble.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I'm not mistaken, the issue is that for the dialog cancel event it does not bubble.
Yes, according to the standard defined by MDN,the dialog cancel event should not bubble, but in testing with native HTML in the browser, the result is that it does bubble. So, we should decide whether to modify it based on the actual behavior of the browser or adhere to the standard defined by MDN.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure? I'm having trouble reproing the event at all but in the spec it's not mentioned as bubbling https://html.spec.whatwg.org/multipage/interactive-elements.html and in Blink source I don't see bubbling either. https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/html/html_dialog_element.cc;drc=67d90538f11c6b232dbfd716075db52aeb34fd15;l=346?q=showModal%20lang:cpp%20filepath:third_party%2Fblink&ss=chromium
If the spec and browsers are inconsistent then we should file a bug with them; if MDN is inconsistent with the spec and browsers then we should file a bug with MDN.
This pull request has been automatically marked as stale. If this pull request is still relevant, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize reviewing it yet. Your contribution is very much appreciated. |
Closing this pull request after a prolonged period of inactivity. If this issue is still present in the latest release, please ask for this pull request to be reopened. Thank you! |
Summary
close #27858.
Add
cancel
event forinput[type="file"]
. Follow MDN cancel eventHow did you test this change?
I have added tests for this PR.