Skip to content
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(ui5-list): change role mappings so no interactive elements are ne… #3952

Merged
merged 3 commits into from
Sep 20, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/fiori/src/NotificationListGroupItem.hbs
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@
<span id="{{_id}}-invisibleText" class="ui5-hidden-text">{{accInvisibleText}}</span>
</div>

<ui5-list class="ui5-nli-group-items" accessible-role="list">
<ui5-list class="ui5-nli-group-items">
<slot></slot>
</ui5-list>

Expand Down
4 changes: 2 additions & 2 deletions packages/fiori/test/pages/NotificationListGroupItem.html
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ <h3>Events on ui5-list level</h3>
<li>itemToggle</li>
</ul>

<ui5-list id="notificationList" header-text="Notifications grouped" accessible-role="list">
<ui5-list id="notificationList" header-text="Notifications grouped">
<ui5-li-notification-group
show-close
show-counter
Expand Down Expand Up @@ -209,7 +209,7 @@ <h3>Events on ui5-list level</h3>
<ui5-toast id="wcToastBS" duration="2000"></ui5-toast>

<ui5-popover id="notificationsPopover" style="max-width: 400px" placement-type="Bottom" horizontal-align="Right">
<ui5-list id="notificationListTop" header-text="Notifications titleText and content 'truncates'" accessible-role="list">
<ui5-list id="notificationListTop" header-text="Notifications titleText and content 'truncates'">
<ui5-li-notification-group
show-close
show-counter
Expand Down
6 changes: 3 additions & 3 deletions packages/fiori/test/pages/NotificationListItem.html
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ <h3>Events on ui5-list level</h3>
<li>itemClose</li>
</ul>

<ui5-list id="notificationList" header-text="Notifications titleText and content 'truncates'" accessible-role="list">
<ui5-list id="notificationList" header-text="Notifications titleText and content 'truncates'">

<ui5-li-notification
busy
Expand Down Expand Up @@ -129,7 +129,7 @@ <h3>Events on ui5-list level</h3>

<br><br>

<ui5-list id="notificationList2" header-text="Notifications titleText and content 'wraps'" accessible-role="list">
<ui5-list id="notificationList2" header-text="Notifications titleText and content 'wraps'">

<ui5-li-notification
show-close
Expand Down Expand Up @@ -184,7 +184,7 @@ <h3>Events on ui5-list level</h3>
<ui5-toast id="wcToastBS" duration="2000"></ui5-toast>

<ui5-popover id="notificationsPopover" style="max-width: 400px" placement-type="Bottom" horizontal-align="Right">
<ui5-list id="notificationListTop" header-text="Notifications titleText and content 'truncates'" accessible-role="list">
<ui5-list id="notificationListTop" header-text="Notifications titleText and content 'truncates'">
<ui5-li-notification
show-close
title-text="New order (#2525) With a very long title - Lorem ipsum dolor sit amet, consectetur adipiscing elit. Praesent feugiat, turpis vel scelerisque pharetra, tellus odio vehicula dolor, nec elementum lectus turpis at nunc."
Expand Down
1 change: 0 additions & 1 deletion packages/main/src/List.hbs
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@
role="{{accessibleRole}}"
aria-label="{{ariaLabelТxt}}"
aria-labelledby="{{ariaLabelledBy}}"
aria-multiselectable="{{isMultiSelect}}"
>
<slot></slot>

Expand Down
6 changes: 3 additions & 3 deletions packages/main/src/List.js
Original file line number Diff line number Diff line change
Expand Up @@ -231,14 +231,14 @@ const metadata = {
* <b>Note:</b> If you use notification list items,
* it's recommended to set <code>accessible-role="list"</code> for better accessibility.
*
* @public
* @private
* @type {String}
* @defaultvalue "listbox"
* @defaultvalue "list"
* @since 1.0.0-rc.15
*/
accessibleRole: {
type: String,
defaultValue: "listbox",
defaultValue: "list",
},

/**
Expand Down
3 changes: 2 additions & 1 deletion packages/main/src/ListItem.hbs
Original file line number Diff line number Diff line change
Expand Up @@ -11,13 +11,13 @@
@touchstart="{{_ontouchstart}}"
@touchend="{{_ontouchend}}"
@click="{{_onclick}}"
aria-selected="{{ariaSelected}}"
role="{{_accInfo.role}}"
aria-expanded="{{_accInfo.ariaExpanded}}"
title="{{title}}"
aria-level="{{_accInfo.ariaLevel}}"
aria-posinset="{{_accInfo.posinset}}"
aria-setsize="{{_accInfo.setsize}}"
aria-describedby="{{_id}}-invisibleText-describedby"
aria-labelledby="{{_id}}-invisibleText {{_id}}-content"
aria-disabled="{{ariaDisabled}}"
style="list-style-type: none;"
Expand Down Expand Up @@ -50,6 +50,7 @@
{{/if}}

<span id="{{_id}}-invisibleText" class="ui5-hidden-text">{{_accInfo.listItemAriaLabel}} {{accessibleName}}</span>
<span id="{{_id}}-invisibleText-describedby" class="ui5-hidden-text">{{_accInfo.ariaSelectedText}}</span>
</li>

{{#*inline "listItemPreContent"}}{{/inline}}
Expand Down
21 changes: 18 additions & 3 deletions packages/main/src/ListItem.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import {
ARIA_LABEL_LIST_ITEM_CHECKBOX,
ARIA_LABEL_LIST_ITEM_RADIO_BUTTON,
LIST_ITEM_SELECTED,
LIST_ITEM_NOT_SELECTED,
} from "./generated/i18n/i18n-defaults.js";

// Styles
Expand Down Expand Up @@ -77,13 +78,13 @@ const metadata = {
*
* @private
* @type {String}
* @defaultvalue "option"
* @defaultvalue "listitem"
* @since 1.0.0-rc.9
*
*/
role: {
type: String,
defaultValue: "option",
defaultValue: "listitem",
},

_mode: {
Expand Down Expand Up @@ -335,6 +336,20 @@ class ListItem extends ListItemBase {
return undefined;
}

get ariaSelectedText() {
let ariaSelectedText;

// Selected state needs to be supported separately since now the role mapping is list -> listitem[]
// to avoid the issue of nesting interactive elements, ex. (option -> radio/checkbox);
// The text is added to aria-describedby because as part of the aria-labelledby
// the whole content of the item is readout when the aria-labelledby value is changed.
if (this.ariaSelected !== undefined) {
ariaSelectedText = this.ariaSelected ? this.i18nBundle.getText(LIST_ITEM_SELECTED) : this.i18nBundle.getText(LIST_ITEM_NOT_SELECTED);
}

return ariaSelectedText;
}

get deleteText() {
return this.i18nBundle.getText(DELETE);
}
Expand All @@ -346,7 +361,7 @@ class ListItem extends ListItemBase {
ariaLevel: undefined,
ariaLabel: this.i18nBundle.getText(ARIA_LABEL_LIST_ITEM_CHECKBOX),
ariaLabelRadioButton: this.i18nBundle.getText(ARIA_LABEL_LIST_ITEM_RADIO_BUTTON),
listItemAriaLabel: this.ariaSelected ? this.i18nBundle.getText(LIST_ITEM_SELECTED) : undefined,
ariaSelectedText: this.ariaSelectedText,
};
}

Expand Down
14 changes: 2 additions & 12 deletions packages/main/src/TreeListItem.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ import {
TREE_ITEM_ARIA_LABEL,
TREE_ITEM_EXPAND_NODE,
TREE_ITEM_COLLAPSE_NODE,
LIST_ITEM_SELECTED,
} from "./generated/i18n/i18n-defaults.js";

// Template
Expand Down Expand Up @@ -286,7 +285,8 @@ class TreeListItem extends ListItem {
ariaLevel: this.level,
posinset: this._posinset,
setsize: this._setsize,
listItemAriaLabel: this.ariaLabelText,
ariaSelectedText: this.ariaSelectedText,
listItemAriaLabel: this.i18nBundle.getText(TREE_ITEM_ARIA_LABEL),
};
}

Expand Down Expand Up @@ -315,16 +315,6 @@ class TreeListItem extends ListItem {
}
}

get ariaLabelText() {
let text = this.i18nBundle.getText(TREE_ITEM_ARIA_LABEL);

if (this.selected) {
text += ` ${this.i18nBundle.getText(LIST_ITEM_SELECTED)}`;
}

return text;
}

get iconAccessibleName() {
return this.expanded ? this.i18nBundle.getText(TREE_ITEM_COLLAPSE_NODE) : this.i18nBundle.getText(TREE_ITEM_EXPAND_NODE);
}
Expand Down
3 changes: 3 additions & 0 deletions packages/main/src/i18n/messagebundle.properties
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,9 @@ LIST_ITEM_POSITION=List item {0} of {1}
#XACT: ARIA announcement for the list item selection.
LIST_ITEM_SELECTED=Selected

#XACT: ARIA announcement for the list item selected=false state
LIST_ITEM_NOT_SELECTED=Not Selected

#XBUT: List Multi Selection Mode Checkbox aria-label text
ARIA_LABEL_LIST_ITEM_CHECKBOX = Multiple Selection mode.

Expand Down
3 changes: 3 additions & 0 deletions packages/main/test/pages/List_test_page.html
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,9 @@
<ui5-input id="selectionChangeResultPreviousItemsParameter" placeholder="selectionChange previousSelection result"></ui5-input>

<br><br><br>
<ui5-list id="justList">
<ui5-li id="justList-country">Argentina</ui5-li>
</ui5-list>
<ui5-list id="listSelectedItem" mode="MultiSelect">
<ui5-li-groupheader id="group-header">New Items</ui5-li-groupheader>
<ui5-li id="not-selected-country">Argentina</ui5-li>
Expand Down
14 changes: 9 additions & 5 deletions packages/main/test/specs/List.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -378,15 +378,19 @@ describe("List Tests", () => {
assert.strictEqual(nextInteractiveElement.isFocused(), true, "Focus is moved to next interactive element.");
});

it('should include selected state text in accInfo', () => {
it('should include selected state text', () => {
const item = $("#justList #justList-country");
const notSelectedItem = $("#listSelectedItem #not-selected-country");
const selectedItem = $("#listSelectedItem #selected-country");

let accInfo = notSelectedItem.getProperty("_accInfo")
assert.strictEqual(accInfo.listItemAriaLabel, null, "Item label is empty");
let ariaSelectedText = item.getProperty("ariaSelectedText");
assert.strictEqual(ariaSelectedText, null, "List is not in select mode, no selected state should be spoken");

accInfo = selectedItem.getProperty("_accInfo");
assert.strictEqual(accInfo.listItemAriaLabel, "Selected", "Selected text is part of the label");
ariaSelectedText = notSelectedItem.getProperty("ariaSelectedText");
assert.strictEqual(ariaSelectedText, "Not Selected", "Selected false state text is correct");

ariaSelectedText = selectedItem.getProperty("ariaSelectedText");
assert.strictEqual(ariaSelectedText, "Selected", "Selected state text is correct");
});

it('group headers should not be with role options', () => {
Expand Down