5

I'm dealing with a situation where I need to bind jQuery events to a page to handle UI updates that is being generated via JSF. Alas, JSF sucks and it sticks onclick events on everything, which pre-empt any of my jQuery work.

Thanks to the fine folks here on SO, I found a solution for that:

mixing my jQuery click events with existing object's onclick attribute

The logic is:

  • on page load, grab all of the onclick attributes and store them in a variable.
  • bind my jQuery events
  • after my own jQuery events, I can then eval the original onclick: eval(onclickValueVariable)

This worked find in all my dummy onclick event testing.

But then it failed on that live JSF page. The issue is that all of the onclick's end with a 'return false' which leads to this error:

return not in function

For example:

<div class="uglyJSFcreatedTag" onclick="console.log('hey');return false">

And the jQuery that would fire it:

var $jsfTag = $('.uglyJSFcreatedTag');
var cacheOnclick = $jsfTag.attr(onclick);
$jsfTag.removeAttr('onclick');

...bunch of logic here to decide if the inline onclick should fire and, if so...

eval(cacheOnclick)

The problem is the return false. It looks like nothing can be returned when firing a eval.

Is there a solution for this? I imagine I could take the value as a string and do string parsing to remove all return false, then call that from the script itself. But that sounds a bit messy. Is there a more elegant solution?

If not, what particular JS statements should I be looking for to filter out before calling eval?

Community
  • 1
  • 1
DA.
  • 39,848
  • 49
  • 150
  • 213
  • I honestly have no clue what you're trying to do, but I think I can help. – mowwwalker Oct 04 '11 at 04:56
  • Why aren't you adding all your listeners the same way? i.e. either all using JSF or all using jQuery. – RobG Oct 04 '11 at 05:02
  • I'm not writing the JSF. And I can't see how adding more UI logic via the backwards JSF way would be a step in the right direction. My job is to make the UI work as best I can given what the JSF team gives me. It harkens back to the days of SharePoint work...hacks on top of hacks. ;) – DA. Oct 04 '11 at 05:13

2 Answers2

6

Two options:

1. Using your current approach, just wrap the onclick string with a function before you eval it. Store the result, and call it as a function. Make sure you call it with the appropriate this context:

var f = eval("(function(){ "+cacheOnclick+"})");
f.call($jsfTag[0]);

Note: the parenthesis () around the function declaration, are required within the eval. This makes the declaration into an expression (syntactically speaking), thus making it legal in in the eval.

2. instead of grabbing the onclick attribute, grab the actual function from the dom element itself. Also, unless you need to do something special with the jsf handler function from your code, I would suggest that you just add the JSF function as a jquery click handler directly, rather than calling it explicitly from your code:

var $jsfTag = $('.uglyJSFcreatedTag');
$jsfTag.bind('click', $jsfTag[0].onclick);
$jsfTag.removeAttr('onclick');

Personally, I would go for approach #2, but either one should work.


Update: Here's a litte additional explanation for Option #2:

var $jsfTag = $('.uglyJSFcreatedTag');

That's from your example -- we're using jquery to retrieve a set containing all elements with the classname 'uglyJSFcreatedTag'.

$jsfTag.bind('click', $jsfTag[0].onclick);

This fetches the first element from the set ($jsfTag[0]), and gets the onclick property of that element object. .onclick is a javascript property that contains a reference to the compiled function that the browser generated from the "onclick" attribute's string content. Now, since we have the handler function, we can bind it directly to the jquery click event, using the jquery bind() function.

$jsfTag.removeAttr('onclick');

Finally, we just remove the attribute. This is ok, because we've already bound the function via jquery. If we don't remove it from the "old-style" onclick, then it'll get called twice.

Note: You may have noticed that the above code only works on the first element in the selected set. For the very likely case that your set contains multiple elements, you'll want to loop through the set and handle each element separately. Here's how you would do that:

var $jsfTag = $('.uglyJSFcreatedTag');
$jsfTag.each( function(idx, element) {
    $(element).bind('click', element.onclick).removeAttr("onclick");
});

If you want to skip the jsf handler for certain elements, then insert logic within the "each loop" to test for this, and skip the call to bind accordingly.


If you must continue to call the jsf handler from within your click handler, then at least consider using element.onclick instead of $(element).attr('onclick'). The latter requires eval, while the former does not.

Lee
  • 13,462
  • 1
  • 32
  • 45
  • The first one seems to make sense to me. However, that gives a different error: `function statement requires a name`. I'm not sure the second option will work for us, though I have to admit I'm not entirely following what the second one is doing. I should mention that we don't always want the onclick to eval (we have some logic that decides if it fires or not). – DA. Oct 04 '11 at 05:05
  • #2 is preferred, #1 is just plain ugly. – RobG Oct 04 '11 at 05:07
  • You should not get that error if you included the parenthesis around the function. I added those (and the corresponding note) in an edit immediately after I made the initial post - so maybe you missed them. That being said, #2 really is a much cleaner approach, and it doesn't involve using `eval()` (which you should generally try to avoid unless you really need it). – Lee Oct 04 '11 at 05:15
  • Ah! I think I get option 2 now. Is this correct: line 2 = binding a new event via jQuery (that happens to be the onclick attribute). Line 3 = remove the onclick event attribute. So, in that situation, if I want to bind my own stuff and have that fire before the original onclick, I just need to bind that prior to line 2, correct? – DA. Oct 04 '11 at 05:16
  • The only issue with #2 is that I don't always want the onclick to fire...and if it's already bound, that might make the if/else logic a bit messier... – DA. Oct 04 '11 at 05:17
  • I've added some additional explanation of option #2. I've also added an important note (with another example) that applies to dealing with multiple items in your jquery selection. Finally, I tried to address your concerns about the delegation needing to be conditional. Hope this helps. – Lee Oct 04 '11 at 05:44
  • It helps a lot. Thanks for such a detailed answer! One last question, if I may: what are the main reasons for wanting to avoiding eval? Is it dangerous in certain situations? – DA. Oct 04 '11 at 06:35
  • [Some will claim](http://javascript.crockford.com/code.html) that "eval is Evil". Truth is that `eval()` can be *used* for evil, but it is not, itself, evil. That said, it should be avoided when possible because: **1.** it is fairly process-intensive (i.e. can reduce your program's performance); and **2.** it is a potential hole through which untrusted/malicious code can get included into your program. Here, #1 is your main concern: the `onclick` attrs are automatically compiled into functions during page-load, so it's better to just use the pre-compiled function, than to eval it again. – Lee Oct 04 '11 at 15:50
4

I think wrapping the code in an anonymous function and invoke it right away should be easier than filtering out return statements.

evalCode = "function() {" + evalCode + "}()";
Jonas Høgh
  • 10,358
  • 1
  • 26
  • 46
  • Close, but when the (formerly) inline handler is called, its *this* keyword won't reference the element, so one more issue to deal with. – RobG Oct 04 '11 at 05:00