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.

Sign up for GitHub

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

Jump to bottom

fix(ASSETS-17274): Visual list is not marked up as list. #275

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

fix(ASSETS-17274): Visual list is not marked up as list. #275

wants to merge 3 commits into from

Conversation

ionelc-ensemble
Copy link
Contributor

https://jira.corp.adobe.com/browse/ASSETS-17274

Change help menu from

<div>
    <a></a>
    <a></a>
</div>

to

<ul>
    <li>
        <a></a>
    </li>
    <li>
        <a></a>
    </li>
</ul>

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.

@ionelc-ensemble
Copy link
Contributor Author

HTML with the current code changes:
Screen Shot 2023-01-20 at 11 04 16

@ionelc-ensemble
Copy link
Contributor Author

Ready for review but cannot add reviewers so tagging @majornista & @Pareesh, whoever has the chance to take a look.
Thank you!

@ionelc-ensemble
Copy link
Contributor Author

Changed apps list from container with role=listbox to role=list and item role=option to role=listitem :
Screen Shot 2023-01-23 at 16 16 41

majornista
majornista requested changes Jan 23, 2023
View reviewed changes
Copy link
Collaborator

@majornista majornista left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SelectList already implements the WAI-ARIA Listbox design pattern and changing the roles for it to behave as a static list would be a breaking change for the Select and ComboBox controls.

coral-component-list/src/scripts/SelectList.js
@@ -526,7 +526,7 @@ const SelectList = Decorator(class extends BaseComponent(HTMLElement) {

// adds the role to support accessibility
if (!this.hasAttribute('role')) {
this.setAttribute('role', 'listbox');
this.setAttribute('role', 'list');
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change should not be made to the SelectList control. The WAI-ARIA listbox role is a widget role, that will force a screen reader into forms mode, allowing the user to navigate using the arrow keys. A list is a static role defining content structure. Think about this as the difference between a <select> element and a ul or ol in HTML.

coral-component-list/src/scripts/SelectListItem.js
@@ -221,7 +221,7 @@ const SelectListItem = Decorator(class extends BaseComponent(HTMLElement) {
this.classList.add(CLASSNAME);

if (!this.hasAttribute('role')) {
this.setAttribute('role', 'option');
this.setAttribute('role', 'listitem');
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change should not be made to the SelectListItem control. The WAI-ARIA option role is a widget role, that will keep a screen reader in forms mode, allowing the user to navigate using the arrow keys. A listitem is a static role defining content structure. Think about this as the difference between an <option> element and a li in HTML.

coral-component-list/src/tests/test.SelectList.Item.js
@@ -23,7 +23,7 @@ describe('SelectList.Item', function () {
describe('Instantiation', function () {
function testDefaultInstance(el) {
expect(el.classList.contains('_coral-Menu-item')).to.be.true;
expect(el.getAttribute('role')).to.equal('option');
expect(el.getAttribute('role')).to.equal('listitem');
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test for option is correct.

coral-component-list/src/tests/test.SelectList.js
@@ -17,7 +17,7 @@ describe('SelectList', function () {
describe('Instantiation', function () {
function testDefaultInstance(el) {
expect(el.classList.contains('_coral-Menu')).to.be.true;
expect(el.getAttribute('role')).equal('listbox');
expect(el.getAttribute('role')).equal('list');
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test for listbox is correct.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@majornista majornista majornista requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ionelc-ensemble @majornista