fix(typeahead): use inline style for live element and localize message #4605
Conversation
There was a problem hiding this comment.
Hey @jeremychoisy,
thanks for the PR.
I think there should be 2 PRs (or at least commits) for this:
- feat → allow customizing live announcer message (this PR)
- fix → to fix the shadow DOM issue (separate PR)
I've left a couple of comments in the code as well + the build is failing.
src/util/accessibility/live.ts
Outdated
| @@ -16,7 +16,7 @@ function getLiveElement(document: any, lazyCreate = false): HTMLElement | null { | |||
| element.setAttribute('aria-live', 'polite'); | |||
| element.setAttribute('aria-atomic', 'true'); | |||
|
|
|||
| element.classList.add('visually-hidden'); | |||
| element.style.display = 'none'; | |||
There was a problem hiding this comment.
Hmm, I think there is a reason .visually-hidden and the likes of it exist (ex. .cdk-visually-hidden in material).
I don't think you should use display: none and visiblity: hidden if you want the the content to be read by the screen reader.
You should either inline all the styles necessary, or we should allow providing a Live service at a different level.
WDYT?
There was a problem hiding this comment.
You are 100% right, updated with the inline style taken from Bootstrap visually-hidden, noticed that the cdk-visually-hidden class might be a little more robust, let me know if you'd like to use this one instead.
Regarding inline style VS providing the Live service at a different level, was not really into the service providing based solution because imo it seemed a little bit too complex compared to the inline style one. But seeing the result, I'll give a try to see how it goes because maintaining this inline styling and not benefiting from what could be done on Bootstrap side is kind of sad.
src/typeahead/typeahead.ts
Outdated
| @@ -397,6 +397,18 @@ export class NgbTypeahead implements ControlValueAccessor, OnInit, OnChanges, On | |||
| this._nativeElement.value = toString(value); | |||
| } | |||
|
|
|||
| private getAnnounceLocalizedMessage(count: number): string { | |||
There was a problem hiding this comment.
Maybe it would be better to introduce an @Input() for these kind of translations as well? Will it be useful?
Like @Input() liveAnnouncerMessage: (count: number) => string;
With default being the function you're suggesting I guess.
There was a problem hiding this comment.
Also in this project private methods start with _ → something like _getLiveAnnouncerLocalizedMessage
There was a problem hiding this comment.
Regarding the @Input, I'm not really sure tbh, I'm not super familiar with the way @angular/localize works but from what I understood, people using this library will generate/maintain translation files based on what is flagged with i18n/$localize. Do we really want/need to provide an additional way to override the translations on top of this already existing mechanism ? Provided keys for this use case seem to cover every possibilities, WDYT ?
jeremychoisy
force-pushed
the
fix/use-inline-style-for-live-element-instead-of-b-class
branch
2 times, most recently
from
02c8b2f
to
42c8721
Compare
jeremychoisy
force-pushed
the
fix/use-inline-style-for-live-element-instead-of-b-class
branch
5 times, most recently
from
4f9dcc5
to
6429acf
Compare
Enable the override of the container for the live element, through a DI token, meaning that the Live service is not provided in root anymore and needs to be provided when used in a component. Fixes ng-bootstrap#4560
jeremychoisy
force-pushed
the
fix/use-inline-style-for-live-element-instead-of-b-class
branch
from
6429acf
to
b5421f3
Compare
Enable localization for the live element. Fixes ng-bootstrap#4560
jeremychoisy
force-pushed
the
fix/use-inline-style-for-live-element-instead-of-b-class
branch
from
b5421f3
to
9d94ae0
Compare
Use inline style instead of the bootstrap class for the live element to support the usage of the component in the shadow DOM and localize the message itself. Fixes #4560
Before submitting a pull request, please make sure you have at least performed the following: