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.

Sign up for GitHub

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

Jump to bottom

No link-hints for click handlers, registered with jQuery.live #1404

Open
maximbaz opened this issue Dec 31, 2014 · 11 comments
Open

No link-hints for click handlers, registered with jQuery.live #1404

maximbaz opened this issue Dec 31, 2014 · 11 comments

Comments

@maximbaz
Copy link

Consider the following example:

<!DOCTYPE html>
<html lang="en">
  <head>
    <meta charset="utf-8">
    <title>Link-hints for jquery.live</title>
    <style>.spoiler_title { text-decoration: underline; cursor: pointer; }</style>
  </head>
  <body>
    <div class="html_format">
      <div class="spoiler">
        <b class="spoiler_title">Spoilers!</b>
        <div class="spoiler_text" style="display: none;">
          <span>Hidden text is here!</span>
        </div>
      </div>
    </div>

    <script src="https://code.jquery.com/jquery-1.8.3.min.js"></script>
    <script>
      $(".html_format .spoiler_title").live("click", function() {
        var spoiler = $(this).closest(".spoiler");
        $("> .spoiler_text", spoiler).slideToggle();
        spoiler.toggleClass("spoiler_open");
      });
    </script>
  </body>
</html>

Press f to show link-hints.
Expected: There is a link-hint to expand/collapse spoiler.
Actual: There is none.

Tested on both master and post-1.46.

The following code was extracted from the popular Russian-speaking collaborative blog habrahabr.ru, so the impacted number of people for this particular example is rather high.
@mrmr1993
Copy link
Contributor

JSFiddle here. Looking at the .live() documentation, it looks like the function is depreciated, so hopefully it will become less of an issue as time goes on.

We can get the registered event listeners from jQuery._data(document, "events") (an internal jQuery API), run in the page (not our content script) context. It would probably be better to hook the jQuery listener registration functions, in a similar style to we do with addEventListener in #1167.

Edit: To communicate the information to the content script, we'll probably want to use a custom event, since (at least in this case) we're dealing with a selector rather than any specific element.

@maximbaz
Copy link
Author

It is deprecated in favor of .on(), which also needs to be patched (see JSFiddle v2).

I like the idea to hook jQuery functions like #1167.

@mrmr1993
Copy link
Contributor

It is deprecated in favor of .on(), which also needs to be patched (see JSFiddle v2).

That version works fine for me using #1167. You need to gf to the frame.

Edit: inserted italic text

@maximbaz
Copy link
Author

OK, so let this particular issue be only about .live() then.

I can see it was deprecated since jQuery 1.7 and removed in 1.9, but as it still being used in such popular sites, we should support it.

@mrmr1993
Copy link
Contributor

... as it still being used in such popular sites, we should support it.

@z0rch can you have a go at implementing it?

@maximbaz
Copy link
Author

I'll have a look in the next year 😄
Thanks for #1405, by the way.

@maximbaz
Copy link
Author

Just occurred to me, that in general case

$(selector).live("click", handlerFn);

is translated into

$(document).on("click", selector, handlerFn);

rather than

$(selector).on("click", handlerFn);

I feel that's where is the difference, handler is assigned to document rather than to a particular element. I updated the JSFiddle (see v3) to reflect this thought.

Will have a more detailed look later on.

@mrmr1993
Copy link
Contributor

Ahh, nice catch. We should probably guard against

$(element).on("click", selector, handlerFn);

too then, by hooking the on method rather than querying event listeners on document.

Looking forward to seeing your work on this!

@maximbaz
Copy link
Author

maximbaz commented Jan 1, 2015

Hey @mrmr1993, I'd like to hear your suggestions on the approach to choose.

First of all, keep in mind that the syntax $(document).on("click", selector, handlerFn) is used to setup a handler for elements, that do not necessarily exist at the time of setting up the handler.

This means that we need some way to remember the selector, and find matching elements by ourselves when a user hits f, just like jQuery itself would do.

Now, in order to hook the .on() method, we need to place the hook after jQuery is loaded, but before the first use of .on() method.
In general case, these two things may exist in one js file (minimized and concatenated), so I'm not even sure how to intercept our hook in-between.

To give you a better insight, here is what roughly happens inside jQuery.on():

  • Link between selector and handlerFn is saved in private variable, let's name it privateHandlers
  • A general event listener for click is assigned to document, with no information about selector or handlerFn
  • When a click event occurs anywhere on the page and bubbles up to document, jQuery uses privateHandlers to call handlerFn only if the chain of clicked events contains an element, matching the selector.

As you can see, even if we switch for a moment to think about overloading document::addEventListener (just like you've been hooking Element::addEventListener), we cannot extract the information about selector from there.

Using this information, do you have any suggestions on how to retrieve those selectors? What kind of hook would be appropriate for us to set, so that we can capture them?
Maybe you can see an another idea, that I'm missing here?

@mrmr1993
Copy link
Contributor

mrmr1993 commented Jan 2, 2015

Now, in order to hook the .on() method, we need to place the hook after jQuery is loaded, but before the first use of .on() method.

I've updated the JSFiddle with my suggested way of hooking the jQuery function. It uses Object.defineProperty on window to overwrite the jQuery implementation of on as it's set, so we don't have to worry about the timing.

@maximbaz
Copy link
Author

maximbaz commented Jan 2, 2015

Properties... beautiful! Thanks for the hint 👍

maximbaz pushed a commit to maximbaz/vimium that referenced this issue Jan 3, 2015
created a test to verify that link hint is shown for elements that have
0de5b74 
delegated events philc#1404
maximbaz pushed a commit to maximbaz/vimium that referenced this issue Jan 3, 2015
show link hints for elements that have delegated onclick listener phi…
524d00d 
…lc#1404
maximbaz pushed a commit to maximbaz/vimium that referenced this issue Jan 3, 2015
created tests to verify that addEventListener is not capturing elemen…
29f0471 
…ts, that only have delegated events assigned philc#1404
maximbaz pushed a commit to maximbaz/vimium that referenced this issue Jan 3, 2015
added a rule to skip addEventListener hook only if element is used as…
dacf625 
… a container to hold delegated events for children philc#1404
maximbaz pushed a commit to maximbaz/vimium that referenced this issue Jan 3, 2015
created test to verify that hink hints are created for elements, that…
5911406 
… are listened with deprecated `.live()` function (jQuery 1.7+) philc#1404
@maximbaz maximbaz mentioned this issue Jan 3, 2015
Closed
2 tasks
maximbaz pushed a commit to maximbaz/vimium that referenced this issue Jan 3, 2015
show link hints for elements that have delegated onclick listener phi…
4aed55b 
…lc#1404
maximbaz pushed a commit to maximbaz/vimium that referenced this issue Jan 3, 2015
created tests to verify that addEventListener is not capturing elemen…
e22fa09 
…ts, that only have delegated events assigned philc#1404
maximbaz pushed a commit to maximbaz/vimium that referenced this issue Jan 3, 2015
added a rule to skip addEventListener hook only if element is used as…
778ee3c 
… a container to hold delegated events for children philc#1404
maximbaz pushed a commit to maximbaz/vimium that referenced this issue Jan 3, 2015
created test to verify that hink hints are created for elements, that…
4cd765a 
… are listened with deprecated `.live()` function (jQuery 1.7+) philc#1404
maximbaz pushed a commit to maximbaz/vimium that referenced this issue Jan 11, 2015
created a test to verify that link hint is shown for elements that have
011dfa0 
delegated events philc#1404
maximbaz pushed a commit to maximbaz/vimium that referenced this issue Jan 11, 2015
show link hints for elements that have delegated onclick listener phi…
a19a96d 
…lc#1404
maximbaz pushed a commit to maximbaz/vimium that referenced this issue Jan 11, 2015
created tests to verify that addEventListener is not capturing elemen…
b39ca48 
…ts, that only have delegated events assigned philc#1404
maximbaz pushed a commit to maximbaz/vimium that referenced this issue Jan 11, 2015
added a rule to skip addEventListener hook only if element is used as…
2ce408c 
… a container to hold delegated events for children philc#1404
maximbaz pushed a commit to maximbaz/vimium that referenced this issue Jan 11, 2015
created test to verify that hink hints are created for elements, that…
8b5558a 
… are listened with deprecated `.live()` function (jQuery 1.7+) philc#1404
maximbaz pushed a commit to maximbaz/vimium that referenced this issue Jan 15, 2015
created a test to verify that link hint is shown for elements that have
b560b7c 
delegated events philc#1404
maximbaz pushed a commit to maximbaz/vimium that referenced this issue Jan 15, 2015
show link hints for elements that have delegated onclick listener phi…
4b0403e 
…lc#1404
maximbaz pushed a commit to maximbaz/vimium that referenced this issue Jan 15, 2015
created tests to verify that addEventListener is not capturing elemen…
b9c60ed 
…ts, that only have delegated events assigned philc#1404
maximbaz pushed a commit to maximbaz/vimium that referenced this issue Jan 15, 2015
added a rule to skip addEventListener hook only if element is used as…
4288498 
… a container to hold delegated events for children philc#1404
maximbaz pushed a commit to maximbaz/vimium that referenced this issue Jan 15, 2015
created test to verify that hink hints are created for elements, that…
2540e52 
… are listened with deprecated `.live()` function (jQuery 1.7+) philc#1404
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@maximbaz @mrmr1993