-
Notifications
You must be signed in to change notification settings - Fork 46.2k
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
Move mouse event disabling on interactive elements to SimpleEventPlugin. Related perf tweak to click handlers. #7642
Move mouse event disabling on interactive elements to SimpleEventPlugin. Related perf tweak to click handlers. #7642
Conversation
@@ -634,7 +656,7 @@ var SimpleEventPlugin = { | |||
}, | |||
|
|||
willDeleteListener: function(inst, registrationName) { | |||
if (registrationName === 'onClick') { | |||
if (registrationName === 'onClick' && isInteractive(inst._tag)) { |
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.
Isn't this the opposite of what we want? i.e., we want to add the empty click handler to elements that aren't considered interactive?
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.
Oof... Yes. Looks like there's also no test coverage here. I'll add that as well.
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.
Done. I inverted the check and added test cases.
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 understand what this change is for. Can you explain?
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 had it reversed (sorry). It now reads:
if (registrationName === 'onClick' && !isInteractive(inst._tag)) {
Inverting the isInteractive check.
Basically, iOS safari only exhibits the event bubbling issue for
non-interactive elements (like divs). Extra event handlers can be
eliminated by only attaching this event listener on those non-interactive
elements. Since the check was already created for the mouse events filter,
I went ahead and added it here.
This trades an event listener attachment for function invocation (that'll
probably get inlined), when I ran the numbers it seemed much faster
https://gist.github.com/nhunzaker/020335ee8ec0f1d487f97c7ddcfbd4a2.
I like the idea of removing a level of indirection here, but if it breaks |
cc @spicyj |
@aweary Did a bit more research. I modified the Checkout: nhunzaker/react@nh-disabled-mouse-extraction...nhunzaker:nh-disabled-mouse-extraction-simulation As for The trouble is that, when // ReactDOMInput-test.js:248
it('should properly control a value of number `0`', function() {
var stub = <input type="text" value={0} onChange={emptyFunction} />;
stub = ReactTestUtils.renderIntoDocument(stub);
var node = ReactDOM.findDOMNode(stub);
node.value = 'giraffe';
ReactTestUtils.Simulate.change(node);
expect(node.value).toBe('0');
}); Basically, So far I'm exploring 2 options:
What do you think? |
GH line comments are broken, seemingly so I'll just write here. I might be okay with breaking Simulate in this case actually. Need to think more. This suppresses for contextmenu, mouseover, mouseout too which the original didn't. Is that intentional? If it is intentional for over/out then we should probably do enter/leave too right? If we want this before the next major maybe we can limit it to the 5 events that are currently suppressed. When is Can you reformat the isInteractive to be
or a switch statement returning true/false? |
@spicyj did you mean to mark as accepted? Seems like there's still potential changes to make |
4734a43
to
8fb7749
Compare
I'm starting to think this is an okay behavior. If I tap into
I've reordered the event switch to rule out these cases, similarly to the exceptions for Firefox. Though I'm unclear if the other events fire on
It otherwise fails the following test: Though I'm curious if this could just be an invariant in
Yep. You know I wrote a |
Sort of unrelated, but when I was working through an attempt to update |
@aweary I think I've addressed all of the comments. The only thing left is to figure out how the change to |
Thanks!
I'm going to defer to @spicyj on that |
@@ -137,6 +137,23 @@ function getDictionaryKey(inst) { | |||
return '.' + inst._rootNodeID; | |||
} | |||
|
|||
function isInteractive(tag) { | |||
return tag === 'button' || tag === 'input' || |
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.
return (
tag === 'button' || tag === 'input' ||
tag === 'select' || tag === 'textarea'
);
FB style doesn't do large indents to line up with previous lines – we just move the previous line down instead.
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.
👍
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.
Sorry, for whatever reason I totally caught your comment wrong before. Just a sec.
I'm fine with the simulate change for a minor release. Can you fix up the style and rebase? Otherwise this looks great, thank you! |
@spicyj Done. |
f07a542
to
fcafa61
Compare
Thank you! |
…in. Related perf tweak to click handlers. (facebook#7642) * Cull disabled mouse events at plugin level. Remove component level filters * DisabledInputUtils tests are now for SimpleEventPlugin * Add click bubbling test * Add isInteractive function. Use in iOS click exception rules * Invert interactive check in local click listener. Add test coverage * Reduce number of mouse events disabable. Formatting in isIteractive() * Switch isInteractive tag order for alignment * Update formatting of isInteractive method
…in. Related perf tweak to click handlers. (#7642) * Cull disabled mouse events at plugin level. Remove component level filters * DisabledInputUtils tests are now for SimpleEventPlugin * Add click bubbling test * Add isInteractive function. Use in iOS click exception rules * Invert interactive check in local click listener. Add test coverage * Reduce number of mouse events disabable. Formatting in isIteractive() * Switch isInteractive tag order for alignment * Update formatting of isInteractive method (cherry picked from commit 73c50e7)
Eliminates
DisabledInputUtils
in favor of filtering out mouse events for disabled interactive elements inSimpleEventPlugin
.I've published the build of this branch, with a test page here: http:https://natehunzaker.com/disabled-react-inputs/
Drawback (and a question):
ReactTestUtils.Simulate
now allows disabled inputs to receive mouse events. This made it necessary to useReactTestUtils.SimulateNative
when I updated the tests. I'm not sure what to do about this. It's as ifReact.TestUtils.Simulate
is bypassingSimpleEventPlugin
. Is this expected?Click bubbling performance tweak:
I noticed that there's a fix for click event bubbling in iOS. iOS doesn't allow click bubbling for non-interactive elements, so a workaround is made by adding a local listener (explained here).
But now there's a check for interactive elements! So this fix can be filtered down to non-interactive elements, significantly reducing listener attachments.
I've summarized this, and included Chrome JS Profiler snapshots here.
More background:
A while back I submitted a fix for disabled inputs in IE (#6215). At the time, I believed that the fix could happen closer to the event system, but lacked familiarity with it.
#7616 yielded a video deep dive on the event system, which was super helpful in understanding this part of React. Thanks kentcdodds, gaearon, and spicyj (not @mentioned for noise)!