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

[BUGFIX beta] fillIn test helper triggers input event. #11708

Merged
merged 1 commit into from
Jul 10, 2015

Conversation

cibernox
Copy link
Contributor

@cibernox cibernox commented Jul 9, 2015

The new <input type="text" oninput={{action "search"}}> is very fast
but at the moment is unusable bacause ember test helpers doesn't fire
that event.

Fixed.

@rwjblue
Copy link
Member

rwjblue commented Jul 9, 2015

Looks good to me.

@mixonic
Copy link
Member

mixonic commented Jul 9, 2015

I would learn toward [BUGFIX beta]. This does not seem high priority, and is a small change in behavior. It would be very unexpected in a patch release.

@cibernox
Copy link
Contributor Author

cibernox commented Jul 9, 2015

@mixonic I'd argue that this is a missbehavior of the current release and prevents to use one of the cool features that 1.13 brought in into the toolchain.

@rwjblue
Copy link
Member

rwjblue commented Jul 9, 2015

@cibernox - Unless a given bugfix is very high severity it should go into beta (not release).

Also, seems easy enough to polyfill (via your own helper) if this is blocking you for past versions.

@mixonic
Copy link
Member

mixonic commented Jul 9, 2015

@cibernox yeah, but we've also had people want key events for these. There is some dangerous line we walk on where these helpers are emulating user behavior mostly but not entirely (or over specifically). The balance is delicate, and thus I am +1 but feel it should go into a minor release and not patch.

@cibernox
Copy link
Contributor Author

Nah, not very blocking since I can fire the event myself at the end. Just wondering what was the criteria to consider something a bugfix of the current release or the next one.
I'll update the commit message

@cibernox cibernox force-pushed the fillIn-triggers-oninput-event branch from 949abb7 to e57d6be Compare July 10, 2015 00:02
@cibernox cibernox changed the title [BUGFIX release] fillIn test helper triggers input event. [BUGFIX beta] fillIn test helper triggers input event. Jul 10, 2015
@stefanpenner
Copy link
Member

tests appear sad.

@cibernox are there concerns with emitting both change + input (mostly just the order)

The new `<input type="text" oninput={{action "search"}}>` is very fast
but at the moment is unusable bacause ember test helpers doesn't fire
that event.

Fixed.
@cibernox cibernox force-pushed the fillIn-triggers-oninput-event branch from e57d6be to 14124e9 Compare July 10, 2015 06:40
@cibernox
Copy link
Contributor Author

@stefanpenner rebuilding.

The correct order of the events is keydown > keyup > keypress > input > change according to MDN

@mixonic
Copy link
Member

mixonic commented Jul 10, 2015

@stefanpenner are you +1 for this? I'd like to land it, please merge away if seems good to you.

@stefanpenner
Copy link
Member

The correct order of the events is keydown > keyup > keypress > input > change according to MDN

thanks for the research, as long as this is preserved :shipit:

@mixonic
Copy link
Member

mixonic commented Jul 10, 2015

Input is before change here.

mixonic added a commit that referenced this pull request Jul 10, 2015
[BUGFIX beta] fillIn test helper triggers `input` event.
@mixonic mixonic merged commit f4f78a9 into emberjs:master Jul 10, 2015
@cibernox
Copy link
Contributor Author

I added a test explicitly for checking that: https://github.com/emberjs/ember.js/pull/11708/files#diff-f834632dddeade3002a271a3e9fb68a3R613 (i'm not firing key events)

@cibernox cibernox deleted the fillIn-triggers-oninput-event branch July 10, 2015 14:59
@quantuminformation
Copy link

Nice work dude!

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.

6 participants