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
| 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
| 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
7 times, most recently
from
b5421f3 to
9d94ae0
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
9d94ae0 to
f33f92d
Compare
jeremychoisy
force-pushed
the
fix/use-inline-style-for-live-element-instead-of-b-class
branch
from
f33f92d to
68d53eb
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
68d53eb to
18753e5
Compare
2 participants
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: