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

Support onMouseOver #340

Closed
chenglou opened this issue Sep 11, 2013 · 10 comments · Fixed by #754
Closed

Support onMouseOver #340

chenglou opened this issue Sep 11, 2013 · 10 comments · Fixed by #754

Comments

@chenglou
Copy link
Contributor

https://github.com/facebook/react/blob/master/src/eventPlugins/SimpleEventPlugin.js#L153

Someone had the trouble in IRC about it.

Edit: changed the subject from "document it" to "support it", because the documentation issue is already at #171.

@sophiebits
Copy link
Collaborator

Better yet, support it!

@BenQuinn
Copy link

Yeah, is there a particular reason it's not supported?

@jordwalke
Copy link
Contributor

Curious if you've seen the EnterLeaveEventPlugin.js. It's automatically injected into React by default, so you can use it today. IMHO it's (much) better than onMouseOver. (I suppose we could support onMouseOver just for the sake of completeness, but I thought I'd just make sure you guys were aware of onMouseEnter/onMouseLeave.)

@sophiebits
Copy link
Collaborator

We discussed this for a bit on IRC -- @BenQuinn was just confused that onMouseOver silently didn't work whereas other events did -- perhaps adding more documentation (#171) will fix the problem too but it seems like we might as well add onMouseOver for completeness.

(I agree that enter/leave are much more useful and suggested that over/out are likely missing simply because no one has needed them!)

@jordwalke
Copy link
Contributor

Agree - I believe there are a few extra allocations (not-pooled) occuring in the event system already today. I'd love to track those down before adding more mouse-move event allocations. If you open the memory profiler in Chrome and move the mouse around, you'll see memory being allocated (and then properly freed of course) but if we use PooledClasses everywhere, we should be flat. That would make me much more comfortable with adding mouseMove to SimpleEventPlugin.js.

Edit: Just checked out SimpleEventPlugin.js - we already have onMouseMove which fires way more frequently than onMouseOver would so it's probably not such a big deal to add onMouseOver - though we really should get rid of all additional allocations in the event path.

@sophiebits
Copy link
Collaborator

Just started to look a little bit into the allocations here. Redefining trapBubbledEvent in ReactEventEmitter with:

function trapBubbledEvent(topLevelType, handlerBaseName, element) {
  EventListener.listen(
    element,
    handlerBaseName,
    function() {}
  );
}

still shows allocations in the Chrome memory timeline view, so I'm not convinced that there's anything we can do here? (Replacing trapBubbledEvent itself with a noop makes the allocations go away. EventListener.listen simply calls addEventListener.)

@petehunt
Copy link
Contributor

@jordwalke I am pretty sure we have done all we can here -- at least I think we've tracked down every callsite where PooledClass would have helped us. I updated our recast transforms to log every syntax construct that would cause an obvious allocation and couldn't find anything. I was thinking maybe there was an array slice or a bind() in there, but @spicyj's experiment shows that this is not the case.

@sophiebits
Copy link
Collaborator

(I didn't verify that we don't have more allocations than the empty-function case; I don't know of a good way to do so.)

@danielstocks
Copy link

I might be misinformed, but isn't there quite a difference in use cases between mouseenter and mouseover events?

Here's a theoretical (non-react) example working with some hover states for nested elements:
http://jsfiddle.net/aJM2S/3/

I tried to apply the above concept in react (with nested components) using onMouseEnter instead, but it obviously fails because it's only triggered once on the top-most element. Moving into a child element and back has no effect.

Maybe there's a better approach for this in react altogether?

sophiebits added a commit to sophiebits/react that referenced this issue Dec 31, 2013
Fixes facebook#340.

Test Plan:
Ported @danielstocks's jsfiddle (linked in the issue) to React and the hover effect worked properly.
@plievone
Copy link
Contributor

plievone commented Jan 1, 2014

Maybe there's a better approach for this in react altogether?

@danielstocks If you render the child hierarchy with React, you can listen to onMouseEnter/onMouseLeave on each child element you are interested in instead of using event bubbling. It is cheap.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants