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

Components using text-support mixin do not work native class actions correctly #18994

Closed
chriskrycho opened this issue May 28, 2020 · 2 comments · Fixed by #18997
Closed

Components using text-support mixin do not work native class actions correctly #18994

chriskrycho opened this issue May 28, 2020 · 2 comments · Fixed by #18997

Comments

@chriskrycho
Copy link
Contributor

A simple reproduction:

import Component from '@glimmer/component'; // could be @ember/component also
import { action } from '@ember/object';

export default class MyComponent extends Component {
  @action onFocus() {
    console.log('this never gets hit');
  }
}
<Textarea @focus-in={{this.onFocus}} />

It does work if you instead invoke—

  • with {{fn}}:

    <Textarea @focus-in={{fn this.onFocus}} />
  • with {{action}}:

    <Textarea @focus-in={{action this.onFocus}} />
  • with {{action}} and a string name:

    <Textarea @focus-in={{action "onFocus"}} />

The problem is here:

let actionName = get(view, `attrs.${eventName}`) || get(view, eventName);
let value = get(view, 'value');
if (SEND_ACTION && typeof actionName === 'string') {
let message = `Passing actions to components as strings (like \`<Input @${eventName}="${actionName}" />\`) is deprecated. Please use closure actions instead (\`<Input @${eventName}={{action "${actionName}"}} />\`).`;
deprecate(message, false, {
id: 'ember-component.send-action',
until: '4.0.0',
url: 'https://emberjs.com/deprecations/v3.x#toc_ember-component-send-action',
});
view.triggerAction({
action: actionName,
actionContext: [value, event],
});
} else if (typeof actionName === 'function') {
actionName(value, event);
}

The key is that we look up the value from attrs first, and attrs always ends up with arguments wrapped in MutableCell (to support two-way binding), so the check whether typeof the action is "function" always fails (it's "object" instead). There is special handling for {{action}} and {{fn}} so they don't have this, which is why using them works.

We need to make sure we explicitly handle the case where the item is a MutableCell, and having done so we'll be able to correctly "fallback" for normal method-style actions.

cc. @rwjblue. I'll try to have a PR up for this tomorrow morning, with a test for the failure case and a fix.

@rwjblue
Copy link
Member

rwjblue commented May 29, 2020

Thanks for reporting! When you work on the PR, can you add tests for both {{input and {{textarea (and their angle cousins)?

@chriskrycho
Copy link
Contributor Author

Will do!

kategengler pushed a commit that referenced this issue Jun 1, 2020
The introduction of the `attrs` API in Ember 3.13 included wrapping
items passed to components with `MutableCell`, to support two-way
binding. Although two-way binding is gone from much of Ember, the text
input components (`Input` and `Textarea`) continue to support it, via
the `TextSupport` mixins. The `sendAction` function used by the mixin
previously assumed that the only options were for an action to be a
string or a function -- not a function wrapped in a `MutableCell`.

The result was that this code, which would be expected to work, did not:
it would simply never be invoked.

    <Textarea @Focus-In={{this.didFocusIn}} />

Accordingly, add logic to `sendAction` in `text_support.js` to unwrap a
mutable cell if it is set, and otherwise to carry on with the logic as
it was previously.

Resolves #18994

(cherry picked from commit f16d174)
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 a pull request may close this issue.

2 participants