NgbRadio disabled does not work.

[leekiernan]

Adding the disabled attribute to an input is removed on page load. Success in tests however it appears that the browser does two passes, removing this attribute on the second.

http://plnkr.co/edit/9udhxszOhPKZAEWcccmi

Versions:

Angular: 2.0.0, 2.1.1
NGbootstrap: 1.0.0-alpha.9, 1.0.0-alpha.14
Bootstrap: 4.0.0-alpha.5, 4.0.0-alpha.2

Stacktrace:

x	x
DomRenderer.setElementAttribute	@ platform-browser.umd.js:1458
DomRenderer.setBindingDebugInfo	@ platform-browser.umd.js:1477
DebugDomRenderer.setBindingDebugInfo	@ core.umd.js:8850
setBindingDebugInfo	@ core.umd.js:5433
Wrapper_NgbRadio.detectChangesInHostProps	@ wrapper.ngfactory.js:61
_View_CoverOptionsComponent0.detectChangesInternal	@ component.ngfactory.js:2006
AppView.detectChanges	@ core.umd.js:9305
DebugAppView.detectChanges	@ core.umd.js:9410
AppView.detectViewChildrenChanges	@ core.umd.js:9331
_View_CustomiseComponent2.detectChangesInternal	@ component.ngfactory.js:1058
AppView.detectChanges	@ core.umd.js:9305
DebugAppView.detectChanges	@ core.umd.js:9410
AppView.detectContentChildrenChanges	@ core.umd.js:9323
_View_NgbAccordion3.detectChangesInternal	@ component.ngfactory.js:255
AppView.detectChanges	@ core.umd.js:9305
DebugAppView.detectChanges	@ core.umd.js:9410
AppView.detectContentChildrenChanges	@ core.umd.js:9323
_View_NgbAccordion1.detectChangesInternal	@ component.ngfactory.js:157
AppView.detectChanges	@ core.umd.js:9305
DebugAppView.detectChanges	@ core.umd.js:9410
AppView.detectContentChildrenChanges	@ core.umd.js:9323
_View_NgbAccordion0.detectChangesInternal	@ component.ngfactory.js:58
AppView.detectChanges	@ core.umd.js:9305
DebugAppView.detectChanges	@ core.umd.js:9410
AppView.detectViewChildrenChanges	@ core.umd.js:9331
_View_CustomiseComponent0.detectChangesInternal	@ component.ngfactory.js:840
AppView.detectChanges	@ core.umd.js:9305
DebugAppView.detectChanges	@ core.umd.js:9410
AppView.detectViewChildrenChanges	@ core.umd.js:9331
_View_CustomiseComponent_Host0.detectChangesInternal	@ host.ngfactory.js:33
AppView.detectChanges	@ core.umd.js:9305
DebugAppView.detectChanges	@ core.umd.js:9410
ViewRef_.detectChanges	@ core.umd.js:7398
RouterOutlet.activate	@ router.umd.js:3458
ActivateRoutes.placeComponentIntoOutlet	@ router.umd.js:2955
ActivateRoutes.activateRoutes	@ router.umd.js:2933
(anonymous function)	@ router.umd.js:2902
ActivateRoutes.activateChildRoutes	@ router.umd.js:2901
ActivateRoutes.activate	@ router.umd.js:2896
(anonymous function)	@ router.umd.js:2646
(anonymous function)	@ Observable.js:108
SafeSubscriber.__tryOrSetError	@ Subscriber.js:232
SafeSubscriber.next	@ Subscriber.js:174
Subscriber._next	@ Subscriber.js:125
Subscriber.next	@ Subscriber.js:89
MergeMapSubscriber.notifyNext	@ mergeMap.js:133
InnerSubscriber._next	@ InnerSubscriber.js:23
Subscriber.next	@ Subscriber.js:89
MapSubscriber._next	@ map.js:83
Subscriber.next	@ Subscriber.js:89
ReduceSubscriber._complete	@ reduce.js:105
Subscriber.complete	@ Subscriber.js:114
MergeMapSubscriber._complete	@ mergeMap.js:125
Subscriber.complete	@ Subscriber.js:114
ArrayObservable._subscribe	@ ArrayObservable.js:116
Observable.subscribe	@ Observable.js:56
Observable._subscribe	@ Observable.js:114
MergeMapOperator.call	@ mergeMap.js:75
Observable.subscribe	@ Observable.js:53
Observable._subscribe	@ Observable.js:114
ReduceOperator.call	@ reduce.js:63
Observable.subscribe	@ Observable.js:53
Observable._subscribe	@ Observable.js:114
MapOperator.call	@ map.js:54
Observable.subscribe	@ Observable.js:53
subscribeToResult	@ subscribeToResult.js:21
MergeMapSubscriber._innerSub	@ mergeMap.js:120
MergeMapSubscriber._tryNext	@ mergeMap.js:117
MergeMapSubscriber._next	@ mergeMap.js:100
Subscriber.next	@ Subscriber.js:89
MergeMapSubscriber.notifyNext	@ mergeMap.js:133
InnerSubscriber._next	@ InnerSubscriber.js:23
Subscriber.next	@ Subscriber.js:89
EverySubscriber.notifyComplete	@ every.js:47
EverySubscriber._complete	@ every.js:64
Subscriber.complete	@ Subscriber.js:114
MergeAllSubscriber._complete	@ mergeAll.js:94
Subscriber.complete	@ Subscriber.js:114
Subscriber._complete	@ Subscriber.js:132
Subscriber.complete	@ Subscriber.js:114
ArrayObservable._subscribe	@ ArrayObservable.js:116
Observable.subscribe	@ Observable.js:56
Observable._subscribe	@ Observable.js:114
MapOperator.call	@ map.js:54
Observable.subscribe	@ Observable.js:53
Observable._subscribe	@ Observable.js:114
MergeAllOperator.call	@ mergeAll.js:63
Observable.subscribe	@ Observable.js:53
Observable._subscribe	@ Observable.js:114
EveryOperator.call	@ every.js:27
Observable.subscribe	@ Observable.js:53
subscribeToResult	@ subscribeToResult.js:21
MergeMapSubscriber._innerSub	@ mergeMap.js:120
MergeMapSubscriber._tryNext	@ mergeMap.js:117
MergeMapSubscriber._next	@ mergeMap.js:100
Subscriber.next	@ Subscriber.js:89
MapSubscriber._next	@ map.js:83
Subscriber.next	@ Subscriber.js:89
MapSubscriber._next	@ map.js:83
Subscriber.next	@ Subscriber.js:89
MapSubscriber._next	@ map.js:83
Subscriber.next	@ Subscriber.js:89
MergeMapSubscriber.notifyNext	@ mergeMap.js:133
InnerSubscriber._next	@ InnerSubscriber.js:23
Subscriber.next	@ Subscriber.js:89
subscribeToResult	@ subscribeToResult.js:16
MergeMapSubscriber._innerSub	@ mergeMap.js:120
MergeMapSubscriber._tryNext	@ mergeMap.js:117
MergeMapSubscriber._next	@ mergeMap.js:100
Subscriber.next	@ Subscriber.js:89
Subscriber._next	@ Subscriber.js:125
Subscriber.next	@ Subscriber.js:89
MapSubscriber._next	@ map.js:83
Subscriber.next	@ Subscriber.js:89
MapSubscriber._next	@ map.js:83
Subscriber.next	@ Subscriber.js:89
MapSubscriber._next	@ map.js:83
Subscriber.next	@ Subscriber.js:89
LastSubscriber._complete	@ last.js:109
Subscriber.complete	@ Subscriber.js:114
MergeAllSubscriber._complete	@ mergeAll.js:94
Subscriber.complete	@ Subscriber.js:114
ScalarObservable._subscribe	@ ScalarObservable.js:51
Observable.subscribe	@ Observable.js:56
Observable._subscribe	@ Observable.js:114
MergeAllOperator.call	@ mergeAll.js:63
Observable.subscribe	@ Observable.js:53
Observable._subscribe	@ Observable.js:114
LastOperator.call	@ last.js:38
Observable.subscribe	@ Observable.js:53
Observable._subscribe	@ Observable.js:114
MapOperator.call	@ map.js:54
Observable.subscribe	@ Observable.js:53
Observable._subscribe	@ Observable.js:114
MapOperator.call	@ map.js:54
Observable.subscribe	@ Observable.js:53
Observable._subscribe	@ Observable.js:114
MapOperator.call	@ map.js:54
Observable.subscribe	@ Observable.js:53
Observable._subscribe	@ Observable.js:114
CatchOperator.call	@ catch.js:30
Observable.subscribe	@ Observable.js:53
Observable._subscribe	@ Observable.js:114
MergeMapOperator.call	@ mergeMap.js:75
Observable.subscribe	@ Observable.js:53
Observable._subscribe	@ Observable.js:114
MapOperator.call	@ map.js:54
Observable.subscribe	@ Observable.js:53
Observable._subscribe	@ Observable.js:114
MapOperator.call	@ map.js:54
Observable.subscribe	@ Observable.js:53
Observable._subscribe	@ Observable.js:114
MapOperator.call	@ map.js:54
Observable.subscribe	@ Observable.js:53
Observable._subscribe	@ Observable.js:114
MergeMapOperator.call	@ mergeMap.js:75
Observable.subscribe	@ Observable.js:53
Observable._subscribe	@ Observable.js:114
MergeMapOperator.call	@ mergeMap.js:75
Observable.subscribe	@ Observable.js:53
(anonymous function)	@ Observable.js:87
ZoneAwarePromise	@ zone.js:518
Observable.forEach	@ Observable.js:86
(anonymous function)	@ router.umd.js:2630
ZoneAwarePromise	@ zone.js:518
Router.runNavigate	@ router.umd.js:2595
(anonymous function)	@ router.umd.js:2586
ZoneDelegate.invoke	@ zone.js:232
onInvoke	@ core.umd.js:6206
ZoneDelegate.invoke	@ zone.js:231
Zone.run	@ zone.js:114
(anonymous function)	@ zone.js:502
ZoneDelegate.invokeTask	@ zone.js:265
onInvokeTask	@ core.umd.js:6197
ZoneDelegate.invokeTask	@ zone.js:264
Zone.runTask	@ zone.js:154
drainMicroTaskQueue	@ zone.js:401

[alex321]

I've found the culprit. It is in here:

https://github.com/ng-bootstrap/ng-bootstrap/blob/master/src/buttons/radio.ts#L142

It should be:

this.disabled = isDisabled; instead of this._disabled = isDisabled
as we want to make sure that we respect the disabled attribute on the actual button itself

I am guessing it makes sense to also change
this._label.active = this.checked; this._label.active = this.disabled

The tests are failing because this seems to be caused by the ngbRadioGroup instead and, unfortunately, that has been missed.

@pkozlowski-opensource I will try to submit a PR today if this makes sense to you.

[pkozlowski-opensource]

@alex321 PRs are always welcomed. Just please make sure to add a test(s) so we don't introduce regressions in the future. Thnx!

[leekiernan]

There's a further point to this should you wish to incorporate into a single PR:

  @Input('disabled')
  set disabled(value: any) {
    this._disabled = this._element.nativeElement.hasAttribute('disabled') ? true : value;

does not reflect any passed in directive as value. Should include

    this._renderer.setElementProperty(this._element.nativeElement, 'disabled', this._disabled);

or similar

[pkozlowski-opensource]

@leekiernan I don't think that your last comment is valid since the disabled property is of type Boolean: https://developer.mozilla.org/en-US/docs/Web/API/HTMLButtonElement

I guess you were thinking of an attribute but we are setting a property here (and then an element might choose to reflect this property as an attribute).

[leekiernan]

Apologies I don't think I expressed myself well above. I looked at @alex321 PR and it appeared to implement the binding that I was trying to bring up.