-
Notifications
You must be signed in to change notification settings - Fork 47.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
Attach empty onclick listener to each node #1536
Conversation
Very nice... I like this a lot more. |
@@ -412,6 +418,23 @@ var SimpleEventPlugin = { | |||
); | |||
EventPropagators.accumulateTwoPhaseDispatches(event); | |||
return event; | |||
}, | |||
|
|||
putListener: function(id, registrationName, listener) { |
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.
What do you think about naming this didPutListener
? Haha, lifecycle methods for everyone.
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.
Fine with me; I was feeling uncreative. :)
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.
Renamed to didPutListener and willDeleteListener.
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
Perhaps wait for decision on #1550, which could allow for it to be done very cheaply. |
Didn't your analysis conclude that walking the tree is already fast and so this should be fine as is? |
@spicyj Seems so yes, I was thinking that it might be easier to fix it in ReactDOMComponent or something similar, seeing as it will have "zero-cost" access to the node. So it's possible that the event API will be trimmed/reworked slightly in the end, as we can now either attach directly to the DOM or query the ReactDOMComponents instead, cheaply. Will probably be a long time coming though, so perhaps its better to just go ahead with this. |
Yeah, we should merge this for now -- if we decide to change how event handlers are done in general later then we can do that. |
@@ -20,6 +20,7 @@ | |||
|
|||
var EventPluginRegistry = require('EventPluginRegistry'); | |||
var EventPluginUtils = require('EventPluginUtils'); | |||
var ExecutionEnvironment = require('ExecutionEnvironment'); |
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.
This is not used.
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.
Oops, fixed. We have really got to get linting properly working here…
Fixes facebook#1169. This is a more robust way of fixing what MobileSafariClickPlugin previously tried to. Now even if you don't want anything to do with touch events, click events still work properly. Test Plan: Added a click handler to an `<img />` element and triggered it in the iOS simulator -- it didn't execute before.
Attach empty onclick listener to each node
Accidentally posted it in the wrong PR (this is the right one): http://www.quirksmode.org/blog/archives/2010/09/click_event_del.html
An alternative is to force |
I believe that the reason this is required is simply so that Safari can show the active style on the right element; you want each "distinct" clickable element to highlight separately, but you don't want subelements to get highlighted by themselves if they go to the same place as their siblings. I can only assume that |
Hmm, true. The current implementation explicitly and immediately traverses the DOM to all the affected elements, which is something we want to avoid in the general case I would think. I'm not opposed to it, but if we're committed to keeping the node cache lazy, it seems counter-productive to have more and more elements explicitly traverse the DOM on insertion anyway (which requires more logic = slightly more expensive). Also, yeah, |
No, you only want to add |
Formerly "Attach empty onclick listener to each node". This reverts commit 431155d.
Fixes #1169, fixes #238.
This is a more robust way of fixing what MobileSafariClickPlugin previously tried to. Now even if you don't want anything to do with touch events, click events still work properly.
Test Plan: Added a click handler to an
<img />
element and triggered it in the iOS simulator -- it didn't execute before.cc @yungsters