[post-1.46] Enable link hints for all elements with click listeners + support jQuery delegated events, and drop some legacy link hint conditions

[maximbaz]

This PR is based on @mrmr1993's #1405, with added fix for #1404.

Currently addEventListener() and jQuery.on() are hooked to provide missing link hints.
.live() is based on .on() since jQuery v1.7 (which was released in Nov. 2011), so it is hooked as well.
I decided not to spend time on supporting older versions.

Ideally, it would be nice to hook removeEventListener() and jQuery.off() as well -- I'll look into this later.

• jQuery.off()
• removeEventListener()

UPDATE (done): There is a bug with number of link hints appearing when listeners for both "normal" and "delegated" events are assigned on the same element. Corresponding test is currently disabled.

I'll look more into that, but you are more than welcome to provide an early feedback for this PR.

UPDATE 2: Agreed not to touch removeEventListener() and jQuery.off() in this PR since we cannot properly handle all cases.

---

#1408 is also fixed as a part of this PR, as I used that functionality to create new tests. It appeared to be simply a missing setup data.

[mrmr1993]

This could be rewritten to avoid 2 DOM calls, remove some redundancy:

selectorsStr = element.getAttribute "vimium-jquery-delegated-events-selectors"
return unless selectorsStr?

[mrmr1993]

Ideally, it would be nice to hook removeEventListener() and jQuery.off() as well -- I'll look into this later.

I'd purposely left this out since it bloats the PR, and doesn't add much value. I can't think of many situations where an element will remain in the DOM and visible but have its click event listener removed. Maybe work on this in a separate branch, so we can consider the merits of the removeEventListener and jQuery.off hooks separately.

I've left a few comments on the code for you to take a look over.

[maximbaz]

Thanks for the comments, they have been addressed.

In regards of .off() and removeEventListener... well maybe you are right, not a big win. But omitting them just feels not good, it's like we are not cleaning up after ourselves.

And that's the reason I also don't want to create a separate PR for it -- if you look at those commits separately, you will not be convinced that it is an important change to merge. While here... well, it is sort of a part of the issue.

And the code amount is small, comparing to other changes: 10 lines of .off(), guarded by 2 little tests, and removeEventListener() is even smaller.

But if you believe this brings too much risk, I'm fine to delete these 3 commits.

[maximbaz]

I have been running this for a while, it is wrong to use querySelectorAll here. Selectors were extracted from jQuery's .on() function, and jQuery supports more kinds of selectors than querySelectorAll (it uses Sizzle in some cases).

An example I found on trello.com is the following selector: a[href=#].
jQuery returns some links, while querySelectorAll throws an exception.

Now, it sounds safe to use jQuery defined on a page — it is definitely defined as we reach this function, we don't bother if jQuery treats same selectors differently in different versions, etc.

But it seems like we cannot access jQuery, defined on the page, from the content script... Is that right, @mrmr1993?

If that's true, what would be the best way to handle this situation?
I can easily think of the following approaches:

• Catch querySelectorAll's exceptions and simply ignore those selectors.
• Introduce jQuery (or Sizzle) as a part of vimium distribution.

But I like neither of them 😄

[mrmr1993]

it seems like we cannot access jQuery, defined on the page, from the content script... Is that right, @mrmr1993?

Unfortunately it is. I would send a custom event from the content script and mark the elements we want when we catch it in the injected code. Obviously then we can resume code execution in link hints with another custom event.

Since a lot of the time we don't have to do this, we should also only send the event when we have a querySelector match on the delegated event selector.

[mrmr1993]

if you believe this brings too much risk, I'm fine to delete these 3 commits.

The code here unmarks elements which have

$(element).on("click", "selector", func1);
$(element).on("click", "selector", func2);
// later
$(element).off("click", "selector", func2);

(or similar) which still have a click listener. I'd rather err on the side of overreporting clickable elements, so maybe separating this out until the implementation is right would be better. For this, querying jQuery._data(element, "events") after each update means that we don't have to track event listeners ourselves.

Edit: I didn't notice you'd already done removeEventListener. That suffers from the same problem, but we can't cheat with jQuery._data, so we'd have to maintain a list of all registered listeners ourselves. IMHO, it's not worth the headache.

[mrmr1993]

Ping @z0rch, can you take a look at my comments here?

[maximbaz]

Sorry for the silence, I agree with both comments, just didn't have time to look into that last week. However I do have time this week 😄