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

When the element is can-defined, not able to dispatch DOM events #15

Open
matthewp opened this issue Jan 22, 2017 · 11 comments · Fixed by #19
Open

When the element is can-defined, not able to dispatch DOM events #15

matthewp opened this issue Jan 22, 2017 · 11 comments · Fixed by #19
Labels

Comments

@matthewp
Copy link
Contributor

el.dispatchEvent(new Event("some-event"));

this should dispatch the event on the element, but since can-define sets up its own addEventListener, the DOM event doesn't work.

@matthewp matthewp added the bug label Jan 22, 2017
matthewp added a commit that referenced this issue Jan 22, 2017
This adds a test for DOM events that currently doesn't work. Skipping it
for now so that we can get it into master.

For #15
@matthewp
Copy link
Contributor Author

There's a test in master that is being skipped for this bug, DOM events work when can-defined

@matthewp
Copy link
Contributor Author

matthewp commented Jan 22, 2017

So to summarize, we need to support both defined property events and arbitrary DOM events.

@justinbmeyer One way to fix this would be to wrap canEvent.addEventListener and fallback to the Node.prototype.addEventListener if the eventName is not one of the defined properties. Does that sound reasonable to you?

Pseudo-code:

var canEvent = require("can-event");

var aEL = canEvent.addEventListener;
canEvent.addEventListener = function(eventName, fn){
  if((this instanceof HTMLElement) && !this._define.definitions[eventName]) {
    return Node.prototype.addEventListener.apply(this, arguments);
  }

  return aEL.apply(this, arguments);
};

@matthewp
Copy link
Contributor Author

Implemented the above in #19

matthewp added a commit that referenced this issue Jan 22, 2017
This fixes #15. Wraps canEvents.addEventListener to fallback to DOM
events if the event isn't for a defined property.
@justinbmeyer
Copy link

justinbmeyer commented Jan 23, 2017 via email

@justinbmeyer
Copy link

justinbmeyer commented Jan 23, 2017 via email

@matthewp
Copy link
Contributor Author

matthewp commented Feb 2, 2017

I'm not understanding how that could work. If someone listens to a defined property event it has to go through the can-event system, doesn't it? Otherwise that property isn't bound and won't fire events.

@justinbmeyer
Copy link

justinbmeyer commented Feb 3, 2017

It doesn't have to go through can-event. can-observation just calls obj.addEventListener("event", callbacks) here: https://github.com/canjs/can-observation/blob/master/can-observation.js#L154

I'm betting it can keep native DOM events and avoid using addEventListener ... the lack of batching MIGHT be an issue still. However, most VM props don't need to change at the same time ... their property values that are objects/lists, which would use can-event, would need batching and have it.

@justinbmeyer
Copy link

Regarding batches, you can probably add the event dispatching in the event queue, so it would be fired at the right time. You'll have to make sure it add the batchNum, but everything should be possible.

@matthewp
Copy link
Contributor Author

matthewp commented Feb 3, 2017

I don't think I'm following your suggestion here. can-define does all of this stuff: https://github.com/canjs/can-define/blob/master/can-define.js#L649

You are saying to remove this, correct? If we remove that, how will it know that a property is being observed?

@justinbmeyer
Copy link

" property is being observed" happens when a property is being read and isn't related to the code you shared.

However, that code is important. Could we provide our own addEventListener that calls the element's addEventListener as the base functionality instead of the can-event addEventListener?

@matthewp
Copy link
Contributor Author

matthewp commented Feb 3, 2017

Filed canjs/can-define#145

@matthewp matthewp reopened this Mar 6, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants