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

Event: Support EventListener interface objects. #5024

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

MarkMYoung
Copy link

@MarkMYoung MarkMYoung commented Mar 15, 2022

Summary

When an event handler implements the EventListener interface (by being an object and having a function property called handleEvent), the handler function used internally is replaced with that handleEvent function bound to the handler object.
http://www.w3.org/TR/DOM-Level-2-Events/events.html#Events-EventListener
Fixes gh-1735

Checklist

  • New tests have been added to show the fix or feature works
  • Grunt build and unit tests pass locally with these changes
  • If needed, a docs issue/PR was created at https://github.com/jquery/api.jquery.com

@linux-foundation-easycla
Copy link

CLA Missing ID CLA Not Signed

@mgol
Copy link
Member

mgol commented Mar 15, 2022

Thanks for the PR. Please sign our CLA before we review it.

@MarkMYoung
Copy link
Author

I have signed it twice today, but it still says that it is missing.

@dmethvin
Copy link
Member

dmethvin commented Mar 16, 2022

The discussion in gh-1735 is pretty old. In that ticket the reason I was referring to the EventListener interface as "low-level" was because I thought it might be useful to use it as a foundation for existing jQuery functionality without exposing EventListener directly in the API. If it's exposed as a top-level ability with .on() (as in, "an EventListener object can be passed instead of a function") then it would probably prevent us from using EventListener for the internal low-level implementation.

I think there may be some other cases and design issues that need to be considered here, especially since we're looking to support new features like passive listeners or the capture phase in jQuery 4. (Passive listeners in particular seem to be something that people want, most likely because Chrome's console complains about them incessantly.)

Assuming those issues are resolved or not relevant, the spec says:

When a Node is copied using the cloneNode method the EventListeners attached to the source Node are not attached to the copied Node. If the user wishes the same EventListeners to be added to the newly created copy the user must add them manually.

However, jQuery's .clone() method supports cloning with copying data and/or events. I'm not sure if this PR handles that and it would be good to have a test to ensure it does when the handler is an EventListener object.

@dmethvin
Copy link
Member

Also I just realized that the Mozilla docs seem to indicate the object with handleEvent is for compatibility, which isn't a good signal for adding new support:

Note: Due to the need for compatibility with legacy content, EventListener accepts both a function and an object with a handleEvent() property function. This is shown in the example below.

@MarkMYoung
Copy link
Author

it would probably prevent us from using EventListener for the internal low-level implementation

Please explain. This PR does not actually pass around and repeatedly accommodate EventListener objects, it supplants it with a bound handleEvent function.

we're looking to support new features like passive listeners or the capture phase in jQuery 4.

This solution only affects the second parameter to addEventListener. Implementating event capturing or passive listeners is not affected by this. I know this because I had to develop my own EventTarget before (taking into account phases, bubbling, capturing, etc.).

I'm not sure if this PR handles that and it would be good to have a test to ensure it does when the handler is an EventListener object.

Again, this solution is merely a function binding. I can look into adding a unit test to demonstrate .clone() with withDataAndEvents set to true, but I don't see how that necessary since a bound member function is just a function.

compatibility...isn't a good signal for adding new support

EventListener is not legacy, it is good design and forethought. EventListener being around for 22 years and being around 11 years before .on() was added to jQuery is a good signal for compliance. That example on Mozilla's website does not even attempt to demonstrate the advantages EventListeners have over callback functions (this PR's unit test does a better job of demonstrating how this object having contextual members). For starters, callback event handlers usually make (strong) references to outside identifiers (because they have no context of their own), but EventListeners encourage code reuse and facilitate testability (by explicitly encapsulating the event handlers behavior).

I rapid develop using .on() with callback event handlers, but after the event/behavior relationships are formalized into EventListener event delegates, I either have to drop .on() in favor of .addEventListener or use something like:

function bindEventListener( evtListener ) {
	return evtListener.handleEvent.bind( evtListener );
}

but why?

// Contrived example.
class VisitedControl {
	constructor( { displayElem } ) {
		this.visitCount = 0;
		this.displayElem = displayElem;
	}
	handleEvent( evt ) {
		++this.visitCount;
		jQuery( this.displayElem ).text( this.visitCount );
	}
}
// Works with the `.on( events [, selector ] [, data ], handler )` signature.
jQuery( "button" ).on( "click", bindEventListener( new VisitedControl( { displayElem: "#button-count" } ) ) );
// Also works with the `.on( events [, selector ] [, data ] )` signature.
jQuery( "input" ).on( "blur", { handler: bindEventListener( new VisitedControl( { displayElem: "#input-count" } ) ) } );

@mgol
Copy link
Member

mgol commented Mar 16, 2022

That example on Mozilla's website does not even attempt to demonstrate the advantages EventListeners have over callback functions (this PR's unit test does a better job of demonstrating how this object having contextual members). For starters, callback event handlers usually make (strong) references to outside identifiers

The way it's implemented it in this PR nullifies that advantage as there's a strong reference to the handler, it's the handler that's being saved. To be able to fully utilize this, we'd have to actually store the object, not the handler and this should be confirmed by unit tests checking if swapping handleEvent for another callback after registration is respected. We'd probably have to create our own EventListener wrappers when a function is provided as a handler to unify the way we store handlers.

This solution only affects the second parameter to addEventListener. Implementating event capturing or passive listeners is not affected by this.

We realize this but when you take above into account, there's a lot more complexity to it. Implementing support for passive events requires us to do a significant refactor of our event module while making sure the test suite passes with as few breaking changes as possible. This is just not a good moment to add new significant features like this one. When the refactor is done, we can return to the subject if it's not already handled by the refactor.

@mgol
Copy link
Member

mgol commented Mar 16, 2022

As for the CLA, I've heard the error might be caused by the email used for the commit in this PR not being attached to your GitHub account. Can you make sure that's done?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

Allow objects as event handlers
3 participants