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
fix(input): update aria-describedby to also include errors #6239
Conversation
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.
LGTM, a few nits.
src/lib/input/input-container.ts
Outdated
ids.push(endHint.id); | ||
} | ||
} else { | ||
if (this._errorChildren) { |
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.
This one can be moved up and turned into an else if
.
fixture.detectChanges(); | ||
|
||
let errorIds = fixture.debugElement.queryAll(By.css('.mat-input-error')) | ||
.map((el) => el.nativeElement.getAttribute('id')).join(' '); |
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.
nit: you can remove the parentheses around el
.
let hintId = fixture.debugElement.query(By.css('.mat-hint')).nativeElement.getAttribute('id'); | ||
let describedBy = inputEl.getAttribute('aria-describedby'); | ||
|
||
expect(hintId).not.toBeFalsy('hint should be shown'); |
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.
toBeTruthy
instead of .not.toBeFalsy
.
.map((el) => el.nativeElement.getAttribute('id')).join(' '); | ||
describedBy = inputEl.getAttribute('aria-describedby'); | ||
|
||
expect(errorIds).not.toBeFalsy('errors should be shown'); |
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.
Also toBeTruthy
.
src/lib/input/input-container.ts
Outdated
|
||
if (endHint) { | ||
ids.push(endHint.id); | ||
if (this._getDisplayedMessages() == 'hint') { |
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.
Triple equals for consistency.
@@ -100,10 +101,13 @@ export class MdHint { | |||
@Directive({ | |||
selector: 'md-error, mat-error', | |||
host: { | |||
'class': 'mat-input-error' | |||
'class': 'mat-input-error', |
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.
Why add a css class just for querying in a test?
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.
This class has always been here, it's used for styling to avoid having to target both element selectors
ids.push(endHint.id); | ||
} | ||
} else if (this._errorChildren) { | ||
ids = this._errorChildren.map(mdError => mdError.id); |
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.
It looks like this always clobbers any existing aria-describedby
. We want to augment an existing value if there already is one. @andrewseguin ran into the same thing w/ tooltip and was going to write some common code to handle this
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.
We could try to add to a static aria-describedby
, but if the user has set up some binding like [attr.aria-describedby]="myStuff"
I'm not sure there's much we can do. I'll sync with Andrew
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.
We'd probably have to update after every ngOnChanges
. This is good for now and we can revisit later
let startHint = this._hintChildren ? | ||
this._hintChildren.find(hint => hint.align === 'start') : null; | ||
let endHint = this._hintChildren ? | ||
this._hintChildren.find(hint => hint.align === 'end') : null; |
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.
You could simplify a lot of this to
const ids = [...this._hintChildren.toArray(), ...this._errorChildren.toArray()].map(c => c.id);
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.
The reason this is more complicated is because there's also a hintLabel
property that can be used to set the start hint. The extra logic is to make sure we put the start hint first (whether its from a md-hint
or hintLabel
, followed by the end hint)
Looks like some test failures on CI |
CI issues resolved |
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.
LGTM
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
fixes #6131