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

Add support for objects with handleEvent in event handlers #1939

Closed
spacejack opened this issue Aug 19, 2017 · 9 comments
Closed

Add support for objects with handleEvent in event handlers #1939

spacejack opened this issue Aug 19, 2017 · 9 comments

Comments

@spacejack
Copy link
Contributor

Expected Behavior

When adding event handlers, Mithril should support objects with a handleEvent function property in addition to plain functions.

Current Behavior

Only functions will be used as event handlers

Possible Solution

Test if the object supplied to an on[event] attrs property has a handleEvent function property. Example:

{
    handleEvent(e) {/*...*/},
    view() {return m('button', {onclick: this}, 'Click')}
}

Steps to Reproduce (for bugs)

Demo of lack of support

Context

This makes it quite easy to avoid re-creating event handler functions every redraw, and doesn't require binding for this context. See this article (pointed out by @JAForbes.)

Your Environment

Affects all browsers.

@pygy
Copy link
Member

pygy commented Aug 19, 2017

We should probably support this, yes... I discovered that interface recently probably as a ripple of that article.

It should be noted that the interface only works with addEventListener, not legacy element.onevent setters... (at least not in Chrome and Firefox). So we'll have to take that into account.

@spacejack
Copy link
Contributor Author

spacejack commented Aug 19, 2017

I took an initial run at it here. Still need to add tests and see if this thing actually works. But a few things I'm unsure of:

  • Not sure how a null exception is avoided here. If it wasn't susceptible before then I don't think it should be now.

  • Not sure how element.onevent setters were used previously. There are checks to replace existing ones in an element but I don't see how they would've been set initially.

  • Also a bit hazy on how the onevent hook is used.

@pygy
Copy link
Member

pygy commented Aug 19, 2017

It looks like a good start... Regarding your questions:

  • The null check is not needed at that point because the callback/listener is only assigned when value isn't null.

  • key in element looks for the presence of onX getters/setters, whether or not they've been set previously.

  • onevent handles e.redraw = false when using mount and route

I'll add a few comments in line...

@spacejack
Copy link
Contributor Author

Commited some updates, I believe it's now handling element.onevent cases in addition to addEventListener with functions, objects. Hoping it's not adding too much overhead.

Have some working demos running in the browser.

Started adding some tests, however it's failing and I wonder if the domMock dispatchEvent supports EventListener objects?

@pygy
Copy link
Member

pygy commented Aug 20, 2017

I wonder if the domMock dispatchEvent supports EventListener objects?

Nope, it doesn't.

@spacejack
Copy link
Contributor Author

Added support for EventListener objects where you pointed out. Added tests which seem to be working now.

I also created a built branch so we can try it in the browser.

@spacejack
Copy link
Contributor Author

I did find a bug, but it looks like it is a pre-existing issue. Example - Clicking "Add" should increment once, then the event listener should be removed.

I can reproduce this with current Mithril in this example. You'll need to use your browser's mobile emulator (touchstart is an event that must be added with addEventListener). Count should only increment once and then stop handling touchstart events, however the listener persists.

Finally, using undefined instead of null also fails to remove an onclick callback: example

@pygy
Copy link
Member

pygy commented Aug 20, 2017

#1804 and related issues are in principle tackled in #1865, I should finish that PR...

@dead-claudia
Copy link
Member

Should be fixed now by #1949.

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

No branches or pull requests

3 participants