Mouse enter and leave work consistently with disabled elements #5762
Conversation
|
@jquense updated the pull request. |
|
Ping. Anything I can do here? |
| React = require('React'); | ||
| ReactDOM = require('ReactDOM'); | ||
| ReactDOMComponentTree = require('ReactDOMComponentTree'); | ||
| it('should use native mouseenter is supported', function() { |
There was a problem hiding this comment.
should use native mouseenter if it is supported
|
anyone have any thoughts about this? ran into this bug again today. Perhaps its less controversial to add native event support separate from rewriting the polyfill? |
|
@jquense updated the pull request. |
ghost
mentioned this pull request
|
any process? |
|
ping~ |
|
@jquense I'll help see this through if you're still game, otherwise I'm happy to pick this up.
Does this mean the fork disappeared? Frustrating. Either way, this seems valuable. I'd even be happy to write some DOM fixtures for it. |
|
I don't know where the branch went! adding support for the native event should be fine I think still. I meant to split it out but haven't gotten to it. The polyfill was a lot more contentious, we ended up at "no one feels comfortable changing the logic here because its super complex and old and no one remembers why it's that way" |
Is that something you'd be willing to take on still (edit: splitting out the native event support)? That just involves plugging up the event registry key/value pairs, rough in here, right? In the mean time, @sebmarkbage do we want to polyfil mouse enter/leave? You've mentioned wanting to trim down ReactDOM, I'm not sure where you'd want to draw the line in the sand for where React polyfills inconsistent browser behavior. |
|
LGTM, any progress? I think it could be helpful if react can do this polyfill. |
|
@nhunzaker I'm ok with any fix that mimics what modern browsers do by default if we attach listeners directly to the nodes since that brings our behavior closer to what the minimal implementation would be. |
makes an attempt at fixing #4251.
First, I added support for native mouseenter and mouseleave, I am not sure why they weren't used before, there may have been a good reason I didn't realize :)
This drastically changes how enter|leave is implemented, modeled after the jQuery approach. I tried to keep with the current implementation but browsers just don't fire the necessary mouse events so it seemed that there was no way to "fix" the current approach.
The EnterLeave logic is pretty deeply integrated so I am not sure I added the various utils in the right spot to maintain correct encapsulation.
While fairly easy to implement with normal handlers the React event system made this sort of tough since you don't know interested nodes. One concern with this approach is that it needs to walk up the instance tree for every mouseout|over event fired to try and find a parent node listening for enter|leave. I thought this approach was more in keeping with how stuff works other places, but I wasn't sure if it represents an unacceptable philosophical or perf concern (my light testing didn't seem to indicate that).
Alternatively I could have just used the didPutListener hook to manually attach mouse over|out listeners.
There could also be a hook or accumulation pattern that does this already that I missed.
thanks for all the hard work ya'll and Happy new years!