By analyzing the blame information on this pull request, we identified @dmethvin, @timmywil and @markelog to be potential reviewers

This looks really good to me!

The main problems with this approach:
getter properties are slow (but unless you call them N+ times still faster then today's fix loop, and fixes #1746)

Definitely seems like a big net plus since forcing layout is much worse!

putting getter properties on the prototype means event.hasOwnProperty(eventProp) is false

It looks like that's the case for the native event object as well? http://jsbin.com/tiwebiguli/1/edit?html,js,console,output

Potential changes to this approach:
drop the setter part (normally faster, except when the getter is called X+ times)

What's the net benefit to keeping the setter?

create jQuery.MouseEvent and jQuery.KeyboardEvent so properties like shiftKey are only on keyboard events

I'm not bothered about the way you have it now, it would be unusual for someone to infer the event type from the presence of properties wouldn't it? At least, I'd want to see someone report a bug before we tried to address this.

only use getters for properties which might force a layout and keep the fix-loop for copying other properties (requires new getter-properties API)

That seems like another place where we could wait and add the extra complexity if it turns out there's existing code that does a lot of event property access already.

You're right, it looks like native events (at least in chrome, FF) already use getters on the prototype. So this would actually make jQuery Event more like native events in that sense...

What's the net benefit to keeping the setter?

I haven't had a chance to measure it yet - but I seem to recall trying it out (>2 years go now!?) and finding that invoking the getter < ~5 times was faster then invoking defineProperty again. It may also depend how expensive the propHook being invoked is.

it would be unusual for someone to infer the event type from the presence of properties wouldn't it?

Yes it would be weird. I guess I just felt it would be a bit "cleaner" if it only had the properties that it should, but I'm not sure if that is worth any extra code.

The only other thing I can think of is how currently in this PR the event propHooks must be written to handle all event types because they are put on the prototype of jQuery.Event. For example the which hook uses rkeyEvent and rmouseEvent to decide which fix to use.

Sort of related... the fixHooks[type].props are currently copied onto the prototype of jQuery.Event the exact same way as props which makes the type-specific one pointless, but I kept those for backwards compatibility. If we stick to only one jQuery.Event object maybe fixHooks[type].props should be deprecated?

Sort of related... the fixHooks[type].props are currently copied onto the prototype of jQuery.Event the exact same way as props which makes the type-specific one pointless, but I kept those for backwards compatibility. If we stick to only one jQuery.Event object maybe fixHooks[type].props should be deprecated?

This definitely is redundant now. Since we're putting this in 3.0 and it's semver-major, we could just remove the per-event-type hooks and have Migrate fill this part by moving them to fixHooks. Thoughts? Looks like there are users of these hooks but as long as we could detect and fill for it in Migrate I don't see a problem with removing it.

This definitely is redundant now. Since we're putting this in 3.0 and it's semver-major, we could just remove the per-event-type hooks and have Migrate fill this part by moving them to fixHooks. Thoughts?

Sounds like a good idea to me.

I've dropped support for fixHooks[event].props but fixHooks[event].filter still exists for now. Should that remain as is? Or it could be simplified to fixHooks[event] now but that would be another breaking change (but could be solved by a migrate?).

This looks good to me from a code/design standpoint. I'd really love to see some sort of benchmark before landing it, even if it's rather ad-hoc. @jbedard any ideas?

Here's a super quick example comparing 2.2 vs this branch (might have to modify the ../dist/jquery.js depending where you put this...):

<html>
<head>
  <script src="https://ajax.googleapis.com/ajax/libs/jquery/2.2.2/jquery.js"></script>
  <script>
    window.jQuery22 = $.noConflict(true);
  </script>

  <script src="../dist/jquery.js"></script>
  <script>
    window.jQuery30 = $.noConflict(true);
  </script>

  <style>
    section {
      display: inline-block;
      width: 160px;
    }
    section div {
      height: 150px;
      width: 150px;
      border: 1px solid black;
      text-align: center;
      display: block;
      overflow: hidden;
    }

    .over {
      background-color: red;
    }
  </style>
</head>
<body>
</body>

<script>
var PROPS = ("altKey bubbles cancelable ctrlKey detail eventPhase " +
    "metaKey shiftKey view which char charCode key keyCode " +
    "button buttons clientX clientY offsetX offsetY pageX pageY screenX screenY toElement").split(" ");

  function setup($, name) {
    var d = $("<section>").append( $("<h1>").text(name) ).appendTo("body");

    d.append(
      $("<div>").text("mousemove noop").on("mousemove", function(){})
    );

    d.append(
      $("<div>").text("enterleave-toggle").on("mouseenter mouseleave", function(e) {
        $(this).toggleClass("over", e.type === "mouseenter");
      })
    );

    d.append(
      $("<div>").text("props access").on("mousemove", function(e) {
        $(this).text(
          PROPS.map(function(p) { return p + ": " + e[p]; }).join(", ")
        );
      })
    );

    function addMousemoveListenerTest(n) {
      d.append(
        $("<div>").text("repeat page/offsetX access (" + n + " times)").on("mousemove", function(e) {
          for (var i=0; i<n; i++) {
            if (e.pageX === -1 || e.offsetX == -1) {
              return;
            }
          }
        })
      );
    }

    for (var n=1; n<20; n+=5) {
      addMousemoveListenerTest(n);
    }
  }

  setup(jQuery22, "2.2");
  setup(jQuery30, "3.0-eventfix");
</script>
</html>

With that snippet above... every case I've tried this PR is faster then 2.2. Even if you read all properties or read pageX 15+ times it still seems faster then the fix loop. For more complicated DOM structures where a rerender is prevented (or where calculating offsetX would be more expensive, etc...) the benefit will be even better.

Is it possible to provide a real-life example, like jquery/jquery.com#88 (comment) or http://jsperf.com/old-vs-new-show-hide/3.

Like i don't understand what this -

for (var n=1; n<20; n+=5) {
  d.append(
    $("<div>").text("repeat pageX access (" + n + " times)").on("mousemove", function(e) {
      for (var i=0; i<n; i++) {
        if (e.pageX === -1) {
          return;
        }
      }
    })
  );
}

supposed to show

That tests the old fix loop which would copy everything once (including e.pageX) vs the new e.pageX getter which will execute n times. Trying to see how many times the getter must be invoked before being slower then the fix loop...

I'll try to put a jsperf together. I guess that would test invoking jQuery.event.dispatch with different handlers similar to the snippet above?

That tests the old fix loop which would...

I meant how this reflect on perf overall in real app.

I guess that would test invoking jQuery.event.dispatch with different handlers similar to the snippet above?

Yeah, if that could be something that we could mention in the blog post when releasing new version that would be great.

Although testing the full functionality with jQuery#on by profiling it on the real app would be more honest.

Quoting @dmethvin from some time ago -

For example, when I was doing the event stuff for 1.7 I used nytimes.com/skimmer among others to see where some real-life bottlenecks were for delegated event improvements. The very same things at the top of the profiler's list were the things that were made faster in 1.7.

I would really like to land this for 3.0 rather than waiting for a later version. I don't think it is truly a breaking change but with something that changes the approach radically there is always a chance of some issues. I'd rather see those in 3.0 than 3.1.

Any objections? I think this needs a quick rebase and it's ready.

I'd say if this is a performance improvement, then it should have performance test.

@dmethvin could you do a quick profile test? I heard you are an expert :)

@dmethvin Just an observation mainly for the future; using jQuery.Event as a forwarding proxy to the native event using Proxy would be insanely awesome and get you away from having to maintain a list of properties at all while still getting all of the benefits of this PR.

The only reason I didn't even suggest that earlier was because you'd have divergent code paths, and the properties not explicitly listed in this file would only work in Proxy supporting browsers which I know historical the core team isn't cool with.

@dcherman Right, the divergence in behavior would be really hard to handle. If it were just possible to do something like ourEvent = Object.create(event) or overwrite the properties on the native event we would have most of what we need. Proxies are likely to be slower than what we have here I'd think?

@markelog I'm not sure I can do better than this, it's always kind of subjective as to whether a synthetic test reflects the real world.

It is not what you said three years ago :), okay, if you sure and want this then i don't have objections

@dmethvin For the moment, definitely seems so. Ran a JSPerf/Benchmark.js suite locally (your favorite!) and it looks like defining the get trap runs at about half the speed of defineProperty getters in Chrome. Didn't really look at other browsers.

This PR does have breaking changes to the event hooks API right now.

Previously there was props, fixHooks[type].props/filter, keyHooks.props/filter and mouseHooks.props/filter.

Currently this PR removes keyHooks and mouseHooks while adding propHooks. Since all props (of any event type) are all copied onto the same jQuery.Event.prototype it would be nice to drop fixHooks[type].props. All internal usage of fixHooks[type].filter is also gone so that could also be dropped.

Could we drop all this functionality but leave it in jquery-migrate? jquery-migrate could easily patch jQuery.event.fix to delegate the old event hooks API to the new API.

If we're making API changes should we go a step further? Are there better ways of exposing this prop/hooks API? Some random ideas...

• jQuery.event.addProperty(name[, getterMethod]) (74068a0)
• jQuery.event.props[type] = true | getterMethod
• jQuery.event.props + jQuery.events.propHooks (9ad80f7)

I've added commits showing my proposals. Each one reduces the gzip size a bit more and the last one (74068a0) brings it down below master (-7 gzip/minified).

9ad80f7 is the original commit which drops keyHooks/mouseHooks and adds propHooks

66704d5 + c24d604 drops fixHooks

ed7aa50 does what I proposed above in a comment. I think this is logical and it reduces size.

74068a0 completely changes the event hooks API to jQuery.event.addProp(name[, hook]). This also allows the getter to be simplified because we know if the hook exists when addProp is called instead of having to check each and every time the getter is invoked (big 👍 of this API change imo). This also removes the (potential) confusion of having both props and propHooks and potential cases like someone adding to propHooks and not realizing they must also add that property to props etc.

WDYT?

jQuery.event properties are not part of the public API, and fixHooks and props don't even do anything other than enumerate properties to copy (which, as you point out, are unnecessary given this solution that automatically exposes all properties). I'm 100% on board up to ed7aa50, and if 74068a0 is smaller then that one works for me too. @jquery/core?

Thanks for the awesome work, @jbedard! 👏

Soooo tempted to switch up the API, but I am thinking it's another big last-minute big change and we should wait until v4 to do that in order to give fair warning. To make it happen now there would need to be more work at api.jquery.com, the upgrade guide, perhaps Migrate as well, and it would need to happen quickly.

One middle ground would be to:

• Land the compatible part of this for 3.0 to get the performance boost
• Deprecate fixHooks in 3.0 but keep them for compatibility
• Define jQuery.event.addProp(name[, hook]) in 3.0 (or 3.1 for that matter) as the new preferred way
• Drop fixHooks in 4.0 in favor of the new API

If most of the team wants to do all of it now I'll go for it too, but we really should get the other pieces done pretty fast.

And @jbedard thanks for your work on this!

Note that this doesn't expose all properties. There is still a list of properties supported by default and any additional properties must be added manually. The good thing is that the number of properties no longer has a negative effect on performance at runtime because properties are just getters on the prototype.

One reason I don't like the props + propHooks[event] solution is that adding a propHooks[event] without adding the same event to props would have no effect. I think this is a weird API where it requires adding 2 things instead of 1.

How about switching to jQuery.event.addProp(name[, hook]) and making all the old API things delegate to this? The old ones could all be deprecated (and moved to migrate?), delegate to addProp, and be dropped in 4.0.

Here is a potential (untested) jquery-migrate addition to maintain support for props and fixHooks (but not key/mouseHooks):

var originalFix = jQuery.event.fix;

jQuery.event.props = [];
jQuery.event.fixHooks = {};

jQuery.event.fix = function(originalEvent) {
    var event,
        type = originalEvent.type,
        fixHook = this.fixHooks[ type ];

    if ( jQuery.event.props.length ) {
        jQuery.each( jQuery.event.props.splice( 0 ), function( i, prop ) {
            jQuery.event.addProp(prop);
        } );
    }

    if ( fixHook && fixHook.props && fixHook.props.length ) {
        jQuery.each( fixHook.props.splice( 0 ), function( i, prop ) {
            jQuery.event.addProp(prop);
        } );
    }

    event = originalFix.call(this, originalEvent);

    return fixHook && fixHook.filter ? fixHook.filter( event, originalEvent ) : event
};

Per yesterday's meeting this is good to land, all commits right?

Should probably squash the commits since there's some intermediate steps (the propHooks API proposal in the first commit, dropped in the last commit).

Let me know if there's anything else I can do to help.

Yep, I squashed them but having the individual commits here is handy since it matches the discusion.

After squashing I'm getting a test failure on a fixHooks test, should that test have been removed?

That specific test should probably just be removed. Might be worth adding some addProp tests though?

Yes, we definitely need some tests. I'll move the propHooks test to Migrate to be sure we're shimming that correctly.

I've removed the fixHooks test (fff5ab5) and replaced it with a similar addProp test (360c512).

I've added 0a213d2 which reduces the size a bit down to -13 min/gz