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

Make acceptance helpers fire native events instead of jQuery ones. #12575

Merged

Conversation

cibernox
Copy link
Contributor

@cibernox cibernox commented Nov 9, 2015

Following issue #12569

Goal: To fire events in relatively accurate and cross-browser way.

Simulate real events in pure javascript accurately is nearly impossible, but
this is a best effort. This fires 3 different kind of events:

  • MouseEvents: click, dbclick, mousedown, mouseup, mouseenter, mouseleave, ...
  • KeyEvents: keydown, keypress and keyup
  • Events: Anything other than mouse or keyboard events falls in this generic kind of event.

This should probably cover 99% of use cases.

Caveats found:

I've only encountered 2 failing tests after this refactor, both related to the same problem:

triggerEvent('.input', 'keydown', { keyCode: 13 });
// ...
equal(event.keyCode, 13, 'options were passed');

It turns that keyCode is a reserved property that only works with keyboardEvents, so although the event has this property mixed, the wrapped jquery event does not copy that property if the original event is not a keyboard event.
Seems very reasonable and probably that test was a bad example.

@cibernox cibernox force-pushed the make_acceptance_helpers_fire_native_evets branch from 6c9ca86 to ccadf06 Compare November 10, 2015 00:41
@cibernox
Copy link
Contributor Author

There is some funkiness going on with the focusin event in phantom and in any other browser the window doesn't has the focus.

Wut?

@rwjblue
Copy link
Member

rwjblue commented Nov 10, 2015

Wut?

Yep.

@cibernox
Copy link
Contributor Author

:trollface:

@cibernox cibernox force-pushed the make_acceptance_helpers_fire_native_evets branch 2 times, most recently from b10a790 to 565d169 Compare November 23, 2015 21:59
@cibernox
Copy link
Contributor Author

@rwjblue I managed to remove al reliance in jquery except this line: https://github.com/cibernox/ember.js/blob/make_acceptance_helpers_fire_native_evets/packages/ember-testing/lib/helpers.js#L108

I tried everything but it couldn't find a way of firing a focusin event if the browser doesn't has the focus. I think that is the browser itself the one that is swallowing any event whose type is focusin.
Any idea?

@cibernox cibernox force-pushed the make_acceptance_helpers_fire_native_evets branch from 58c9400 to 51c3de0 Compare November 27, 2015 10:27
@cibernox cibernox force-pushed the make_acceptance_helpers_fire_native_evets branch 4 times, most recently from f2917fd to 9c5f719 Compare January 20, 2016 09:56
@cibernox
Copy link
Contributor Author

I've rebased this. The only jquery event I wasn't able to replace was triggering focusin in firefox when the window is not focused. Everything I tried failed.

@stefanpenner
Copy link
Member

The only jquery event I wasn't able to replace was triggering focusin in firefox when the window is not focused. Everything I tried failed.

We have had lots of issues with this event on FF...

@stefanpenner
Copy link
Member

Do we want to treat this as a bugfix or a new feature?

@cibernox
Copy link
Contributor Author

Not sure. I'd say bugfix because firing jquery events instead of real events is being "less real" than we should be.

That said, I fear that this can be backwards incompatible in some weird edge case I haven't predicted, but it's just me being pessimistic. I'd try and see if someone reports.

@stefanpenner
Copy link
Member

That said, I fear that this can be backwards incompatible in some weird edge case I haven't predicted, but it's just me being pessimistic. I'd try and see if someone reports.

I like this PR alot, but I share this concern. Which made me think, what if it was treated as a feature instead. It is possible that [bugfix canary] is sufficient, but lets ask señor @rwjblue

@rwjblue
Copy link
Member

rwjblue commented Jan 21, 2016

I like the idea of it being in a feature flag (and therefore easier to back out if we have to), but I would like to land it and then subsequently enable the feature nearly immediately (to let folks bang on it for a full canary and beta cycle).

@cibernox - Do you mind feature flagging it?

@stefanpenner
Copy link
Member

I like the idea of it being in a feature flag (and therefore easier to back out if we have to),

ya good point.

@cibernox cibernox force-pushed the make_acceptance_helpers_fire_native_evets branch from 9c5f719 to a706765 Compare January 21, 2016 12:26
@cibernox
Copy link
Contributor Author

Added a ember-test-helpers-fire-native-events feature flag pre-enabled to true.
PR/commit has been labeled as [FEATURE canary]

@stefanpenner
Copy link
Member

@cibernox nice!

@cibernox cibernox force-pushed the make_acceptance_helpers_fire_native_evets branch from a706765 to 835041e Compare January 23, 2016 12:01
@cibernox
Copy link
Contributor Author

Any idea why the TEST_SUITE=each-package-test tests fail?

@rwjblue
Copy link
Member

rwjblue commented Jan 23, 2016 via email

@cibernox cibernox force-pushed the make_acceptance_helpers_fire_native_evets branch from 835041e to db9bebb Compare January 23, 2016 20:48
@cibernox
Copy link
Contributor Author

Seems to fail consistently, but the output doesn't give me any clue of why

@cibernox cibernox force-pushed the make_acceptance_helpers_fire_native_evets branch 2 times, most recently from 6700fd0 to 87d0e28 Compare February 2, 2016 19:24
@cibernox
Copy link
Contributor Author

cibernox commented Feb 2, 2016

Build is green now, but don merge yet, I will implement some suggestion by @runspired in slack tonight/tomorrow

@cibernox cibernox force-pushed the make_acceptance_helpers_fire_native_evets branch 3 times, most recently from c09d649 to 14df566 Compare February 3, 2016 01:24
@cibernox
Copy link
Contributor Author

cibernox commented Feb 3, 2016

Ready to merge if build turns out green again.

@cibernox cibernox force-pushed the make_acceptance_helpers_fire_native_evets branch 5 times, most recently from 2c146b1 to d2d7983 Compare February 3, 2016 19:39
Goal: To fire events in relatively accurate and cross-browser way.

Simulate real events in pure javascript accurately is nearly impossible, but
this is a best effort. This fires 3 different kind of events:

- MouseEvents: click, dbclick, mousedown, mouseup, mouseenter, mouseleave, ...
- KeyEvents: keydown, keypress and keyup
- Events: Anything else.

This should probably cover 99% of use cases.
@cibernox cibernox force-pushed the make_acceptance_helpers_fire_native_evets branch from d2d7983 to 3fbdcb7 Compare February 3, 2016 22:55
@stefanpenner stefanpenner changed the title Make acceptance helpers fire native evets instead of jQuery ones. Make acceptance helpers fire native events instead of jQuery ones. Feb 5, 2016
rwjblue added a commit that referenced this pull request Feb 5, 2016
…native_evets

Make acceptance helpers fire native evets instead of jQuery ones.
@rwjblue rwjblue merged commit 9875048 into emberjs:master Feb 5, 2016
@cibernox cibernox deleted the make_acceptance_helpers_fire_native_evets branch February 5, 2016 22:51
jgwhite added a commit to adopted-ember-addons/ember-sortable that referenced this pull request Mar 10, 2016
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

Successfully merging this pull request may close these issues.

3 participants