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

Attach empty onclick listener to each node #1536

Merged
merged 1 commit into from
May 27, 2014

Conversation

sophiebits
Copy link
Collaborator

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

@yungsters
Copy link
Contributor

Very nice... I like this a lot more.

@@ -412,6 +418,23 @@ var SimpleEventPlugin = {
);
EventPropagators.accumulateTwoPhaseDispatches(event);
return event;
},

putListener: function(id, registrationName, listener) {
Copy link
Contributor

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.

Copy link
Collaborator Author

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. :)

Copy link
Collaborator Author

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.

@facebook-github-bot
Copy link

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@syranide
Copy link
Contributor

Perhaps wait for decision on #1550, which could allow for it to be done very cheaply.

@sophiebits
Copy link
Collaborator Author

Didn't your analysis conclude that walking the tree is already fast and so this should be fine as is?

@syranide
Copy link
Contributor

@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.

@sophiebits
Copy link
Collaborator Author

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');
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not used.

Copy link
Collaborator Author

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.
yungsters added a commit that referenced this pull request May 27, 2014
Attach empty onclick listener to each node
@yungsters yungsters merged commit bca1f0e into facebook:master May 27, 2014
@syranide
Copy link
Contributor

Accidentally posted it in the wrong PR (this is the right one):

http://www.quirksmode.org/blog/archives/2010/09/click_event_del.html

Apart from its ugliness, I see one problem with this approach: if my guess is right and
the Apple engineers have in fact disabled click event delegation because of memory
considerations, this workaround will create new memory problems. Lots of them if you
have lots of divs that must become clickable.

But that can’t be helped. Event delegation simply must work for the click event, and if
we have to brute-force it at the expense of memory that’s just too bad for Safari iPhone.

An alternative is to force cursor: pointer on Mobile Safari, since you can't see the cursor no one should care, it only has to exist for that element with a click-handler and it should keep us away from the issue above? I think? (It does have to play nice with style though, which might not be trivial?)

@sophiebits
Copy link
Collaborator Author

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 cursor: pointer would have the same overhead and I prefer this solution as it doesn't require browser sniffing or otherwise messing with CSS.

@syranide
Copy link
Contributor

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, cursor: pointer would still have to traverse the DOM and I was under the impression that onclick="" would need to be added to all elements on the path, which it doesn't seem like it does?

@sophiebits
Copy link
Collaborator Author

No, you only want to add onclick="" to the element that has the handler.

yungsters added a commit that referenced this pull request Jun 16, 2014
It's causing issues in product code. Reverting until it can be
investigated further.
sophiebits added a commit to sophiebits/react that referenced this pull request Feb 23, 2015
Formerly "Attach empty onclick listener to each node". This reverts commit 431155d.
sophiebits added a commit that referenced this pull request Mar 10, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants