Skip to content
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

Merged
merged 8 commits into from
Sep 8, 2016

Conversation

nhunzaker
Copy link
Contributor

Eliminates DisabledInputUtils in favor of filtering out mouse events for disabled interactive elements in SimpleEventPlugin.

I've published the build of this branch, with a test page here: http://natehunzaker.com/disabled-react-inputs/

Drawback (and a question):

ReactTestUtils.Simulate now allows disabled inputs to receive mouse events. This made it necessary to use ReactTestUtils.SimulateNative when I updated the tests. I'm not sure what to do about this. It's as if React.TestUtils.Simulate is bypassing SimpleEventPlugin. 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)!

@@ -634,7 +656,7 @@ var SimpleEventPlugin = {
},

willDeleteListener: function(inst, registrationName) {
if (registrationName === 'onClick') {
if (registrationName === 'onClick' && isInteractive(inst._tag)) {
Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

Copy link
Collaborator

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?

Copy link
Contributor Author

@nhunzaker nhunzaker Sep 2, 2016

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.

@aweary
Copy link
Contributor

aweary commented Sep 2, 2016

I like the idea of removing a level of indirection here, but if it breaks ReactTestUtils.Simulate then we'd need to address that somehow.

@aweary
Copy link
Contributor

aweary commented Sep 2, 2016

cc @spicyj

@nhunzaker
Copy link
Contributor Author

@aweary Did a bit more research. I modified the ReactTestUtils.Simulate.* such that it uses the event hub. With the exception of change events, it works!

Checkout: nhunzaker/react@nh-disabled-mouse-extraction...nhunzaker:nh-disabled-mouse-extraction-simulation

As for change events...

The trouble is that, when EventHubPlugin calls out to ChangeEventPlugin, it absorbs the onChange event because inputValueTracking.js deems the value has not changed. I'm observing it in the following test (and others that are similar):

// 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, onChange is getting ignored because inputValueTracking overrides the value property and observes new assignments. So when the change event fires afterwards, technically nothing has changed, and ChangeEventPlugin ignores it.

So far I'm exploring 2 options:

  1. If EventPluginHub doesn't pick up any events, just dispatch a SyntheticEvent. It's already whitelisted to trusted event types.
  2. If the event is a change event, use the current Simulate behavior of always dispatching a SyntheticEvent (which circumvents the event hub and forces the onChange handler).

What do you think?

@ghost ghost added the CLA Signed label Sep 2, 2016
@sophiebits
Copy link
Collaborator

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 inst._currentElement null (in inst._currentElement &&)?

Can you reformat the isInteractive to be

return (
  tag === 'button' || tag === 'input' ||
  tag === 'select' || tag === 'textarea'
);

or a switch statement returning true/false?

@aweary
Copy link
Contributor

aweary commented Sep 3, 2016

@spicyj did you mean to mark as accepted? Seems like there's still potential changes to make

@ghost ghost added the CLA Signed label Sep 3, 2016
@nhunzaker
Copy link
Contributor Author

I might be okay with breaking Simulate in this case actually. Need to think more.

I'm starting to think this is an okay behavior. If I tap into EventPluginHub, I get other test failures related to the other event plugins. For example, mouse enter/leave fail. Simulate forcing an event, with SimulateNative producing the expected DOM behavior, is okay by me (for whatever my vote is worth).

This suppresses for contextmenu, mouseover, mouseout. Is that intentional? [...]

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 disabled, I'd be happy to look into it. Either way, I agree that it is important for a non-breaking release.

When is inst._currentElement null (in inst._currentElement &&)?

It otherwise fails the following test:

https://github.com/facebook/react/blob/master/src/renderers/dom/client/__tests__/ReactEventIndependence-test.js#L53-L68

Though I'm curious if this could just be an invariant in Simulate.

Can you reformat the isInteractive to be [...] or a switch statement returning true/false?

Yep. You know I wrote a switch twice before I settled on the || chain... I've done the reformatting.

@nhunzaker
Copy link
Contributor Author

Sort of unrelated, but when I was working through an attempt to update Simulate, I noticed that BeforeInputEventPlugin returns [null, null] a lot. This happens, I think, nearly every time EventPluginHub.extractEvents is invoked. Any interest in a PR that cuts the extra array allocation?

@ghost ghost added the CLA Signed label Sep 3, 2016
@coveralls
Copy link

Coverage Status

Coverage increased (+0.002%) to 87.235% when pulling f07a542 on nhunzaker:nh-disabled-mouse-extraction into 7b247f3 on facebook:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.002%) to 87.235% when pulling f07a542 on nhunzaker:nh-disabled-mouse-extraction into 7b247f3 on facebook:master.

@nhunzaker
Copy link
Contributor Author

@aweary I think I've addressed all of the comments. The only thing left is to figure out how the change to Simulate should be handled.

@ghost ghost added the CLA Signed label Sep 6, 2016
@aweary
Copy link
Contributor

aweary commented Sep 6, 2016

Thanks!

The only thing left is to figure out how the change to Simulate should be handled.

I'm going to defer to @spicyj on that

@ghost ghost added the CLA Signed label Sep 6, 2016
@@ -137,6 +137,23 @@ function getDictionaryKey(inst) {
return '.' + inst._rootNodeID;
}

function isInteractive(tag) {
return tag === 'button' || tag === 'input' ||
Copy link
Collaborator

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Copy link
Contributor Author

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.

@sophiebits sophiebits added this to the 15-next milestone Sep 8, 2016
@sophiebits
Copy link
Collaborator

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!

@ghost ghost added the CLA Signed label Sep 8, 2016
@nhunzaker
Copy link
Contributor Author

@spicyj Done.

@sophiebits sophiebits merged commit 73c50e7 into facebook:master Sep 8, 2016
@sophiebits
Copy link
Collaborator

Thank you!

acdlite pushed a commit to acdlite/react that referenced this pull request Sep 9, 2016
…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
@zpao zpao modified the milestones: 15-next, 15.4.0 Oct 4, 2016
zpao pushed a commit that referenced this pull request Oct 4, 2016
…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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants