feat(ui5-li-custom): improve accessibility announcements#12696
feat(ui5-li-custom): improve accessibility announcements#12696
Conversation
|
🧹 Preview deployment cleaned up: https://pr-12696--ui5-webcomponents.netlify.app |
IlianaB
changed the title
|
why don't we reuse this? We could externalize it from the table. |
We discussed with the team: we could use an externalized logic for extracting accessibility descriptions. The only concern - currently there is a specific table-related logic. Can it be separated from the externalized method for processing the accessibility info? |
|
Actually, I do not see any table-specific logic here. But if such logic exists, it should go into the options (we should probably use the second parameter of
As mentioned I do not see those as table-specific. |
Thanks for explanations @aborjinik . Let us know when it’s ready for use. We’ll try to adapt. If any questions or difficulties arise, we’ll contact you. :) |
I am a little confused. I am not fully aware of all your requirements. Are you expecting us to provide a utility that can be reused by other components? |
Please see #12976 |
| _onfocusin(e: FocusEvent) { | ||
| super._onfocusin(e); | ||
| // Skip updating invisible text during drag operations | ||
| if (!this._isDragging()) { |
There was a problem hiding this comment.
how the component is focused during dragging? that would be nice to know for the table
There was a problem hiding this comment.
This is a safety check due to consistently failing tests in TabContainerDragAndDrop.cy.tsx and TabContainerDragAndDropShadowDom.cy.tsx - it seems like rendering (changing text of invisible span) steals the focus in test environment and check for currently focused element fails. I wasn't able to reproduce any actual problem when trying to drag and drop list items in test pages.
| super._onfocusin(e); | ||
| // Skip updating invisible text during drag operations | ||
| if (!this._isDragging()) { | ||
| this._updateInvisibleTextContent(); |
There was a problem hiding this comment.
isn't this event bubble? from an input that is in the list item for example? for this case you do not need the custom announcement.
| onAfterRendering() { | ||
| // This will run after the component is rendered | ||
| if (this.shadowRoot && !this.shadowRoot.querySelector(`#${this._id}-invisibleTextContent`)) { | ||
| const span = document.createElement("span"); | ||
| span.id = `${this._id}-invisibleTextContent`; | ||
| span.className = "ui5-hidden-text"; | ||
| // Empty content as requested | ||
| this.shadowRoot.appendChild(span); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Returns the invisible text span element used for accessibility announcements | ||
| * @returns {HTMLElement | null} The HTMLElement representing the invisible text span used for accessibility announcements, or null if the element is not found in the shadow DOM | ||
| * @private | ||
| */ | ||
| private get _invisibleTextSpan(): HTMLElement | null { | ||
| return this.shadowRoot?.querySelector(`#${this._id}-invisibleTextContent`) as HTMLElement; | ||
| } |
There was a problem hiding this comment.
why don't you just use applyCustomAnnouncement ? You cannot focus two elements at the same time. It is not necessary to create a similar node for every list
There was a problem hiding this comment.
@aborjinik , when I first started working on this task I checked the table logic. Reusing just one span for every announcement was the solution I liked the most. Unfortunately, with the already existing implementation of all ListItems, following this logic would require refactoring on all of them, which we decided would unnecessary expand the scope of this task.
If we decide to create a refactoring task - this suggestion would be part of it.
There was a problem hiding this comment.
Unfortunately, with the already existing implementation of all ListItems, following this logic would require refactoring on all of them
Ok then I guess I do not understand what this custom announcement is good for and how it does not mix up with the original announcement.
| if (defaultSlot) { | ||
| const assignedNodes = (defaultSlot as HTMLSlotElement).assignedNodes({ flatten: true }); | ||
| assignedNodes.forEach(child => { | ||
| const text = getCustomAnnouncement(child, { lessDetails: false }, false); |
There was a problem hiding this comment.
please do not call with default values and the 3rd parameter is private
| // Return the built-in delete button from the shadow DOM if it exists | ||
| const deleteButton = this.shadowRoot?.querySelector(`#${this._id}-deleteSelectionElement`); | ||
| return deleteButton ? [deleteButton] : []; | ||
| } |
There was a problem hiding this comment.
Instead, I think you should just implement the accessibilityInfo, if any customization necessary (seems like something special with the delete button nodes) then only in this case this should be reflected in the children array accordingly. The type property of the accessibilityInfo should be taken from the LISTITEMCUSTOM_TYPE_TEXT and then you should just ask the `getCustomAnnouncement(this). You do not need to build your own custom announcement again.
There was a problem hiding this comment.
I have tried with accessibilityInfo implementation on the ListItemCustom itself. It's not the deleteButtons, which bothered me. I have just included them in the children array.
We decided we do not these logic for rootElement:
if (_isRootElement) {
const hasDescription = descriptions.length > 0;
if (!hasDescription || !lessDetails) {
const tabbables = getTabbableElements(element);
const bundleKey = [
hasDescription ? "" : ACC_STATE_EMPTY,
ACC_STATE_SINGLE_CONTROL,
ACC_STATE_MULTIPLE_CONTROLS,
][Math.min(tabbables.length, 2)];
if (bundleKey) {
hasDescription && descriptions.push(".");
descriptions.push(getBundle().getText(bundleKey));
}
}
}
Single control or Multiple controls - checked with ACC experts - we do not have such requirement for now. But because of the check || !lessDetails - we cannot omit this additional information (as we need more details on the children level - required, disabled, etc states).
To be honest I do not completely understand the check here - how the lessDetails are related to this additional information for single/multiple controls or empty state and the same time with the required/disabled/readonly states. Maybe if there are two different properties of the CustomAnnouncementOptions? lessDetails and accStates?
Empty state - seemed nice but we can never pass the conditions here as we will always have a description (coming from the type property of ListItemCustom's acc info).
There was a problem hiding this comment.
Single control or Multiple controls - checked with ACC experts - we do not have such requirement for now.
In a custom list item put a single native <input>, or <ui5-input> which did not implement the accessibilityInfo yet. When the focus set on that custom list item what should be the announcement?
Or just put nothing inside, and you focus on this CLI then what should be announced?
This is what that logic handles.
Besides that if one day ui5-input implements the accessibilityInfo then it is also true that you will hear "Includes Element" after the input announcement. This is something to notify users that there is something interactive inside this component so they can press F2 to jump in and get more details. But if you do not want that then you can add an option to disable that feature.
Empty state - seemed nice but we can never pass the conditions here as we will always have a description (coming from the type property of ListItemCustom's acc info).
True and can be fixed in getCustomAnnouncement but you can also consider to use aria-roledescription and avoid the type from the custom announcement.
4 participants
related to #7729