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

Input component does not accept documented event names #18654

Closed
jenweber opened this issue Jan 5, 2020 · 23 comments
Closed

Input component does not accept documented event names #18654

jenweber opened this issue Jan 5, 2020 · 23 comments

Comments

@jenweber
Copy link
Contributor

jenweber commented Jan 5, 2020

Is this intended behavior, or a bug?

A contributor fixed an issue with our checkbox examples in the Guides, and in testing their work, I found that none of the Input types accept dasherized names, or enter, unless you use the action handler. However, it stated in the guides that they do, and the API docs suggest it as well.

In a fresh 3.15 app:

@action
doSomething() {
  alert('something')
}
This does not work:
<Input @type="text" @key-down={{this.doSomething}} />

This also does not work, even though there are no dashes:
<Input type="text" @enter={{this.doSomething}} />

This does not work:
<Input type="text" {{on "enter" this.doSomething}} />

This works:
<Input @type="text" @keyDown={{this.doSomething}} />

This works:
<Input @type="text" @enter={{action "doSomething"}} />

This works:
<Input @type="text" @key-down={{action "doSomething"}} />

This works:
<Input type="text" {{on "keydown" this.doSomething}} />

To do:

@jenweber
Copy link
Contributor Author

jenweber commented Jan 17, 2020

As discussed in the framework team meeting, we should show using camelCase event names for actual events, i.e. {{on "someEvent" this.doThing}}

There are some directly handled actions:

  • @enter
  • ????

Overall the docs on this should be scrapped & rewritten from scratch to describe these in an Octaney way

@edprats
Copy link

edprats commented Feb 7, 2020

thank you! i spent a pretty good amount of time losing my mind over this. the ember guides should 100% be updated to reflect this.

https://guides.emberjs.com/release/components/built-in-components/

The example given for action handling is incorrect and will never work:

<Input @value={{this.firstName}} @key-down={{this.updateFirstName}} id="firstname" />

@ianknauer
Copy link
Contributor

looks like focusing in and out requires everything to be fully lowercase, the camel case version doesn't work.

<Input id="fname" @placeholder="First Name" @value={{this.value}} {{on "focusin" this.increment}} {{on "focusout" this.decrement}} />

@pzuraq
Copy link
Contributor

pzuraq commented Feb 21, 2020

Using on always uses standard browser events, whose names are never camelcase, they’re lowercase. I think the outcome of the meeting was to show camelcase for argument events:

<Input @keyDown={{this.foo}}>

Arguments should also be preferred over using the on modifier, when they exist.

@gvocale
Copy link

gvocale commented Feb 27, 2020

I find also weird that if I have the following @keyUp fires when I press ENTER, but not @enter:

<Input @enter={{action 'onEnter'}} @keyUp={{this.onKeyUp}} />

If I remove the @keyup, then @Enter fires.

<Input @enter={{action 'onEnter'}} @keyUp={{this.onKeyUp}} />

Is it desired behavior?

@nightire
Copy link
Contributor

@gvocale If you change your code to:

<Input @enter={{action 'onEnter'}} @key-up={{this.onKeyUp}} />

then it would work as you expected.

And @key-up is exactly what the official document recommended way:

image

Please note what I highlighted in the screenshot.

The Additional work which mentioned in the above description is here:

https://github.com/emberjs/ember.js/blob/v3.16.3/packages/@ember/-internals/views/lib/mixins/text_support.js

and here is the reason why @key-up would trigger @enter as well:

https://github.com/emberjs/ember.js/blob/v3.16.3/packages/@ember/-internals/views/lib/mixins/text_support.js#L195-L198


I can walk through you what happened, but I can not explain why it is behaving like that. I believe this is one of a few "mysterious" things that often make people confused and ember.js should fix them ASAP.

At last, I highly recommend rewriting your code to:

{{! @enter is a "magical thing" that ember.js does for us, not a native event }}
<Input @enter={{fn this.onEnter}} {{on "keyup" (fn this.onKeyUp)}} />

and actions could be written as :

@action
onEnter(_value: string, event: KeyboardEvent): void {
  console.log('onEnter: ', event);
}

@action
onKeyUp(event: KeyboardEvent): void {
  console.log('onKeyUp: ', event);
}

Because this way should be last long enough, and I truly think it has a better consistency and easy to understand.

@jenweber
Copy link
Contributor Author

jenweber commented Feb 27, 2020

@MelSumner did you end up getting any more info on this issue?

Melanie and I created an app for demonstrating/testing the behavior and it was confusing enough that I didn’t know what to put in the Guides. The comments in this thread help!

Demo app: https://github.com/jenweber/octane-input-testing

@cah-brian-gantzler
Copy link

My understanding was that fn was there to create a function for currying additional parameters. It doesnt make sense to use fn to call a function that is already a function, yet I am seeing this more and more. What is fn doing here that it needs to be used?

@pzuraq
Copy link
Contributor

pzuraq commented Feb 28, 2020

That shouldn’t be needed, and if that is the case it definitely is a bug. I think some folks may be doing it because they are converting from (action) as well.

@kevinhinterlong
Copy link

@cah-brian-gantzler
Copy link

I would love to know the reason its needed to help my understanding of what fn is actually doing

@ef4
Copy link
Contributor

ef4 commented Mar 6, 2020

All the event names that always worked on the classic {{input }} (prior to 3.10, which is the first release that supports <Input />) should also work on <Input/> and if one doesn't it's a bug.

However, this is complicated by the fact that there are patterns that worked by accident. For example, key-down is the supported event name, but it's implemented internally with a keyDown method, so if you say keyDown= you're actually overwriting that internal method and it just happens to do the right thing. But overwriting internal methods is... not a good idea.

(This is a great illustration for why glimmer components use this.args instead of putting all the arguments directly onto the component instance. We learned from the mistake.)

@ef4
Copy link
Contributor

ef4 commented Mar 6, 2020

I would love to know the reason its needed to help my understanding of what fn is actually doing

fn does only one thing: partial application (which is also known as "currying").

This:

@logOne={{fn this.log 1}}

means the same thing as this would in javascript:

logOne = function(...args) {
  return this.log(1, ...args);
}

That is all fn is for. If you find that you need to wrap things in fn just to make event handling work, that is a bug and please file an issue about it.

@jenweber
Copy link
Contributor Author

jenweber commented Mar 6, 2020

We discussed this issue in today's Framework Team meeting and @locks will be working on solidifying what we want to teach people as well as which of this behavior is unexpected.

Godfrey shared some insights that the inconsistency is possibly due to clobbering events and actions in the Input component.

@nightire
Copy link
Contributor

nightire commented Mar 7, 2020

@cah-briangantzler @kevinhinterlong

I would love to know the reason it's needed to help my understanding of what fn is actually doing

Actually it has nothing to do with the fn helper, the problem is caused by this line:

} else if (typeof actionName === 'function') {

When passing actions to <Input /> without fn helper, the action is actually a "Mutable Cell Object" (correct me if I'm using a wrong term), it exists as view.attrs.enter, the view in here is the <Input /> component actually.

This line checks if a found "action" (view.attrs.enter) is actually a function, but apparently it isn't. The real function exists on the view.attrs.enter.value property and if you call view.attrs.enter directly, it will proxy this invocation to its value instead.

If we wrap the action with fn helper, this line will pass and the action will be invoked correctly. So fn doesn't has any problem here, the problem is in the old <Input /> component itself.

UPDATE:
My english is weak, in order to help you understand, I setup a similar demo like @kevinhinterlong did, and I set a breakpoint on the line I mentioned above:

图片

(passing an action w/o fn helper)


图片

(passing an action w fn helper)

@cah-brian-gantzler
Copy link

Thanks for the explanation.

Yes the action appears to be wrapping the function in a property. I dont know why. We found this when using sinon. To mock the action, you have to mock the getter as if it is a property instead of a function. It looks like that decision is also confusing input.

@liberlanco
Copy link

I've stuck on this too.

As a new ember developer I was really under impression that <Input @enter="this.method" /> should work. Even magic @enter="method" works (real magic!). Not sure is this is expected or unexpected way, but that was I tried at first guided by intuition after reading docs and completing tutorial.

Here are my tests:
https://ember-twiddle.com/f8d2d67811bae4f7f0a3dbdd91cf52eb?openFiles=templates.components.my-component%5C.hbs%2C

Thanks.

@chancancode
Copy link
Member

chancancode commented Sep 27, 2020

This should be fixed by #18997, which should be released in 3.21 and should be backported has been released in 3.21 and backported into LTS 3.20 and 3.16.

@chancancode
Copy link
Member

To be clear, the previously documented approach is correct – i.e. <Input @type="text" @key-down={{this.doSomething}} />. With modifiers now available, if you are listening to a native event, then the {{on}} modifier approach is probably preferable and more idiomatic.

The "camel case" approach is not correct as it "works" by "overriding" the private event handling hook as arguments are simply "splatted" onto the component instance.

And to be extra clear, there is no "magic" with the casing and they are not interchangeable – keyDown is hook inherited from Ember.Component intended for the implementation of the class to override, not for the consumer/invoker of the component. Since that name is taken (remember arguments and properties shares the same namespace in Ember.Component), the public API name for the callback arguments has to be different and key-down (etc) was chosen. It's a completely different property from keyDown. (In fact, the implementation of the keyDown hook is what calls/dispatches the key-down callback.)

As far as passing strings (@enter="method") it part of the "sendAction" semantics and is deprecated and will be removed in 4.0.

@chancancode
Copy link
Member

We can probably close this? However some of the confusion has since propagated into the learning materials, so we probably need to unroll them and track that effort somewhere (cc @emberjs/learning-team-managers). So sorry for the bug/confusion, and not being able to investigate earlier. Many thanks @chriskrycho for investigating/fixing the bug!

@jenweber
Copy link
Contributor Author

PRs for furture reference:
#18997
#19001

For the Guides followup work, I am moving this to ember-learn/guides-source#1543

@jenweber
Copy link
Contributor Author

Thanks everyone for the help, discussion, and fixes!

If anyone is interested to help with the guides writing/edits, please let me know!

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