-
Notifications
You must be signed in to change notification settings - Fork 27.4k
fix(ngAria): Apply ARIA attrs correctly #13483
Conversation
Well done @marcysutton - thanks. We will get this in after 1.5.0 is released |
709b79c
to
82c6438
Compare
82c6438
to
de718c8
Compare
<h2 id="ngvaluechecked">ngValue and ngChecked</h2> | ||
|
||
To ease the transition between native inputs and custom controls, ngAria now supports | ||
[ngValue](https://docs.angularjs.org/api/ng/directive/ngValue) and [ngChecked](https://docs.angularjs.org/api/ng/directive/ngChecked). The original directives were created for native inputs only, so ngAria extends |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These links should be {@link ng/directive/ngValue}
and {@link ng/directive/ngChecked}
.
de718c8
to
9af2245
Compare
|
||
The boolean `required` attribute is only valid for native form controls such as `input` and | ||
`textarea`. To properly indicate custom element directives such as `<md-checkbox>` or `<custom-input>` | ||
as required, using ngAria with {@link ng/directive/ngrequired ngRequired} will also add |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if ng/directive/ngrequired
(instead of ng/directive/ngRequired
) works.
BREAKING CHANGE: Where appropriate, ngAria now applies ARIA to custom controls only, not native inputs. Because of this, support for `aria-multiline` on textareas has been removed. New support added for ngValue, ngChecked, and ngRequired, along with updated documentation. Closes angular#13078 Closes angular#11374 Closes angular#11830
9af2245
to
86e49e1
Compare
}); | ||
|
||
it('should attach aria-required to textarea', function() { | ||
compileElement('<textarea ng-model="val" required></textarea>'); | ||
it('should attach to custom controls with ngModel and required', function() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excess leading whitespace :)
LGTM (the whitespace can be taken care of upon merging) |
@marcysutton, (a little too late) but I have some questions regarding dropping
|
} | ||
|
||
function shouldAttachRole(role, elem) { | ||
// if element does not have role attribute | ||
// AND element type is equal to role (if custom element has a type equaling shape) <-- remove? | ||
// AND element is not INPUT | ||
return !elem.attr('role') && (elem.attr('type') === role) && (elem[0].nodeName !== 'INPUT'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@marcysutton, why are we only checking against input
? Shouldn't all native controls (e.g. textarea
, select
, button
) be excluded ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that line should be replaced with isNodeOneOf(elem, nodeBlackList))
. It has already gone through the getShape
function but that only checks type
and role
.
BREAKING CHANGE: Where appropriate, ngAria now applies ARIA to custom controls only, not native inputs. Because of this, support for
aria-multiline
on textareas has been removed.New support added for ngValue, ngChecked, and ngRequired, along with updated documentation. Also supports ngTrueValue and ngFalseValue as part of ngModel.
Closes #13078
Closes #11374
Closes #11830
cc @petebacondarwin (I got this done finally! 😄)