Don't trigger action if its element is disabled

[rlivsey]

Fixes #1936

[jamesarosen]

+1

[rlivsey]

Should it still prevent-default if the element is disabled?

Thinking of links with href, if it's disabled then you'd click it and end up navigating to another page which probably isn't what should happen. Then again, technically disabled isn't valid on <a> IIRC.

[jamesarosen]

At first, I thought it should return false.

Then I reconsidered. You could conceivably have a jQuery plugin that shows a "why is this button disabled?" tooltip if you ever try to click something that's disabled. (Or, if you think that's silly, a click tracker on a feature that's not yet built that you use to figure out whether people are interested in the feature.)

Then I reconsidered again: there's already the bubbles property. How about obeying that?

[rlivsey]

@jamesarosen if event.preventDefault() is called it will still propagate so you could still have other listeners for those use-cases. If bubbles is false then you'd lose that - but you've got to have explicitly set that so if you want disabled links to propagate but non-disabled not to I think it's probably fine to require some custom coding.

I'm coming round to thinking that the disabled check should move down a few lines under the bubble check but will wait to see what others think before updating the pull request.

[wagenet]

I'm not sure that I really like the action helper checking for attributes. At the same time I understand the usefulness of this change. I'd like to hear a bit more discussion on this.

[jamesarosen]

@wagenet AFAICT, the only alternative would be to use a custom view class that includes TargetActionSupport and check the attribute there.

Is it possible that the action helper is doing too much? Is there something it could delegate to?

I guess the other option is to say that the disabled flag's state had to come from somewhere. That's likely the controller, so the it can use that information in the action callback to cancel the event.

[SirZach]

@jamesarosen We've implemented that exact solution of a small view mixing in the TargetActionSupport and on the click function...

click: function () {
    if (!this.get('disabled') && !Em.isEmpty(this.get('action'))) {
      this.triggerActionWithContext();
    }
  }

(We extened TargetActionSupport to include context)

[lukemelia]

I don't think {{action ...}} should look at the attributes of the element it is on. If support for disabled at this level is important (and I'm not yet convinced it isn't better handled elsewhere as others have mentioned), I think it would be better to see it as an option on the {{action ...}} helper.

[jamesarosen]

This leaves Ember without a default way of building a button that can be disabled. Yes, you can always build a view with TargetActionSupport, but that's what Em.Button was and that got removed, which indicates that it's not recommended.

[lukemelia]

@jamesarosen what do you think about the approach of adding an option to the action helper?

[jamesarosen]

Totally decent :)

[lukemelia]

Maybe you or @rlivsey want to implement?

[krisselden]

@jamesarosen If you use a <button> for your action instead of an <a>, the disabled attribute will already suppress the click event and thus suppress the action. Does that not meet the goal of "a default way of building a button that can be disabled"?

[jamesarosen]

@kselden that might just be crazy enough to work.

[th0r]

As I understand this PR hasn't been merged yet?

[rstudner]

<button type="button" class="btn btn-primary test_id_button_save_metric_time_series_widget"
          {{action 'saveWidget'}} {{bind-attr disabled='isWidgetHasTitle'}}>Save changes</button>

if isWidgetHasTitle returns either true, or false, the disabled attribute is never even set, or respected, on my button.

view code:
isWidgetHasTitle: function() {
var w = this.get('widget');
var c = w.get('config');
var t = c.title;
var l = t.length;
var result = l == 0;
if (result) {
return "true"
}

    return "false";
}.observes('widget.config.title')

I've tried returning true and "true", false and "false" just in case.

Neither provides even setting the disabled property, much less anything else in this thread.

thoughts?

[wagenet]

@rstudner This should work and seems like a separate issue. Please create a JSFiddle and ask if someone on IRC in #emberjs can review.