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

Events are fired twice for onxxxx type of handlers #12569

Closed
cibernox opened this issue Nov 6, 2015 · 8 comments
Closed

Events are fired twice for onxxxx type of handlers #12569

cibernox opened this issue Nov 6, 2015 · 8 comments

Comments

@cibernox
Copy link
Contributor

cibernox commented Nov 6, 2015

I was writing an acceptance test for something that looks like this:

<div class="foo" onclick={{action "foo"}}>Click me</div>

Bad news came when I realized that click('.foo') was triggering the foo action twice, first with a jquery.event with type click and a second time shortly after with a native MouseEvent.

Digging into the helper first and the addon later I've discover that the cause is that jquery's trigger triggers a jquery.event first, and if preventDefault is not called on that event, it fires a native event just afterwards, leading to unexpected behaviour (in my case, toggling a property twice and therefore letting it unchanged).

In real life the same code works perfectly because native events are fired instead.

I don't know if kebab actions, that will also receive native events, can be affected by this too.

Solutions I see:

A) Fire real events instead. Would it be backwards incompatible? I guess that the dispatcher would catch them and turn them into jquery events as it does with regular events in real life, and also mimics real behaviour better.

B) Accept a second argument in helpers click('.css-selector', { native: true }). Clearly backwards compatible, but leaves us with 2 different paths for firing events.

Thoughts?

@cibernox
Copy link
Contributor Author

cibernox commented Nov 7, 2015

Quick update.

I created myself a nativeClick async helper, that is basically a 1:1 copy of the current one but firing native MouseEvents and focus.

I replaced every invocation of click by nativeClick and the test suite run without any problem, which makes me think that firing native elements (option A) can be retrocompatible.

@stefanpenner
Copy link
Member

@cibernox yes, our test helpers should trigger via raw DOM not at the jQuery level. There are many cases where users libraries do no use jQuery, so triggering them in the jQuery world causes much grief. And a solution like yours is required.

I believe originally, supporting IE6-8 was a motivator. I am unsure, but I suspect with IE9+ we might be fine with your approach, atleast for some things? Is it possible for you to investigate compat further, and hopefully that can dictate when we can land an improved handler.

@cibernox
Copy link
Contributor Author

cibernox commented Nov 9, 2015

I don't believe it would take much time, because it would be just a 1:1 translation to jquery's trigger to a native trigger. I do remember that there is some inconsistencies between browsers when triggering keyboard events, but there is workarounds for that).

However there also another task that we might want to tackle first. Why are this acceptance test helpers part of ember's core? It seems to me that they should be part of ember-test-helpers or perhaps another addon of their own.

I see no reason to tie them together, and people shouldn't need to wait for ember 2.X to get fixes like this or #11708

How do you (core team) feel about breaking them apart?

@stefanpenner
Copy link
Member

I see no reason to tie them together, and people shouldn't need to wait for ember 2.X to get fixes like this or #11708

I think a supplementary addon could be created, but for 3.0 we can decide to remove/add the helpers.

The reality is, it was added to core because we lacked a good addon, system. So it might be something that leaves core, and goes to an addon for 3.0. But until then, and alternative set of helpers could be provided via addon.

@cibernox
Copy link
Contributor Author

cibernox commented Nov 9, 2015

In that case i'd rather attempt to fix this in ember itself as long as it is backwards compatible.

If it proves to not be possible I'll create an addon with those helpers

@stefanpenner
Copy link
Member

In that case i'd rather attempt to fix this in ember itself as long as it is backwards compatible.

very much +1 (as long as its backwards compat)

@pixelhandler
Copy link
Contributor

+1 for native events, and I like the references to "in real life" :)

@cibernox
Copy link
Contributor Author

Closed by #12575

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