Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

fix(ngAria): handle non-string model values #10274

Closed
wants to merge 1 commit into from
Closed

fix(ngAria): handle non-string model values #10274

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/ngAria/aria.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -228,13 +228,13 @@ ngAriaModule.directive('ngShow', ['$aria', function($aria) {
if (needsTabIndex) {
needsTabIndex = false;
return function ngAriaRadioReaction(newVal) {
var boolVal = newVal === attr.value;
var boolVal = (angular.isUndefined(newVal) ? newVal : newVal.toString()) === attr.value;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lots of things don't have toString methods --- Object.create(null), { toString: null }, null, to name a few. Safer bet is just '' + newVal

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, '' + newval will still throw in the Object.create(null) case, although that is a bit of a corner case.

Maybe val && val.toString ? val.toString() : val would work better.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestions: add a helper function like this:

function ToString(O) {
  return (O == null || !O.toString) ? O : O.toString();
}

probably the best bet.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or just do var boolVal = newVal == attr.value like ngModelController does...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regardless of the above discussion, don't you guys feel like the underlying / idea of the fix is very fragile??

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$render of inputTypeRadio checks if $viewValue === attr.value, not the with the modelValue. I checked, that works. We could also expose the check as ctrl.$isEmpty(). Then we could do:

var boolVal = !ngModel.$isEmpty(ngModel.$viewValue);

Still a bit silly that we need to pass the $viewValue, since we now again always use $isEmpty with the $viewValue everywhere (or should). We could change $isEmpty in 1.4 to not take an argument, so we'd have a canonical way of getting the empty state for ngAria and other plugins.

elem.attr('aria-checked', boolVal);
elem.attr('tabindex', 0 - !boolVal);
};
} else {
return function ngAriaRadioReaction(newVal) {
elem.attr('aria-checked', newVal === attr.value);
elem.attr('aria-checked', (angular.isUndefined(newVal) ? newVal : newVal.toString()) === attr.value);
};
}
}
Expand Down
13 changes: 13 additions & 0 deletions test/ngAria/ariaSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,19 @@ describe('$aria', function() {
expect(element.eq(1).attr('aria-checked')).toBe('true');
});

it('should handle non-string model values', function() {
var element = $compile('<input type="radio" ng-model="val" value="0">' +
'<input type="radio" ng-model="val" value="1">')(scope);

scope.$apply("val=0");
expect(element.eq(0).attr('aria-checked')).toBe('true');
expect(element.eq(1).attr('aria-checked')).toBe('false');

scope.$apply("val=1");
expect(element.eq(0).attr('aria-checked')).toBe('false');
expect(element.eq(1).attr('aria-checked')).toBe('true');
});

it('should attach itself to role="radio"', function() {
scope.$apply("val = 'one'");
compileInput('<div role="radio" ng-model="val" value="{{val}}"></div>');
Expand Down