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

[MWPW-165791] Table select aria-label and column cell role added #3513

Open
wants to merge 9 commits into
base: stage
Choose a base branch
from
28 changes: 24 additions & 4 deletions libs/blocks/table/table.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
/* eslint-disable no-plusplus */
import { createTag, MILO_EVENTS } from '../../utils/utils.js';
import { createTag, getConfig, MILO_EVENTS } from '../../utils/utils.js';
import { decorateButtons } from '../../utils/decorate.js';
import { debounce } from '../../utils/action.js';
import { replaceKey } from '../../features/placeholders.js';

const DESKTOP_SIZE = 900;
const MOBILE_SIZE = 768;
Expand Down Expand Up @@ -90,10 +91,9 @@ function handleHeading(table, headingCols) {
const describedBy = `${headerBody?.id ?? ''} ${headerPricing?.id ?? ''}`.trim();
trackingHeader.setAttribute('aria-describedby', describedBy);

col.removeAttribute('role');
col.setAttribute('role', 'columnheader');
}

nodeToApplyRoleScope.setAttribute('role', 'columnheader');
nodeToApplyRoleScope.setAttribute('scope', 'col');
});
}
Expand Down Expand Up @@ -153,6 +153,15 @@ function handleAddOnContent(table) {
table.addEventListener('mas:resolved', debounce(() => { handleEqualHeight(table, '.row-heading'); }));
}

async function setAriaLabelsForExpandableIcons() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we have a unit test for this as well?

const config = getConfig();
const ariaLabel = await replaceKey('toggle-row', config);
const icons = document.querySelectorAll('.icon.expand[role="button"]');
Copy link
Contributor

Choose a reason for hiding this comment

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

Although I agree that all expand icons should get the aria-label attribute, I'm not sure this is the place to do it. I'd either apply the behavior in a more general icon file or scope these changes to just the table we're decorating.

icons.forEach((icon) => {
icon.setAttribute('aria-label', ariaLabel);
});
}

function handleHighlight(table) {
const isHighlightTable = table.classList.contains('highlight');
const firstRow = table.querySelector('.row-1');
Expand Down Expand Up @@ -255,7 +264,7 @@ function handleSection(sectionParams) {
}

if (isCollapseTable) {
const iconTag = createTag('span', { class: 'icon expand' });
const iconTag = createTag('span', { class: 'icon expand', role: 'button' });
sectionHeadTitle.appendChild(iconTag);

if (expandSection) {
Expand Down Expand Up @@ -448,6 +457,15 @@ function applyStylesBasedOnScreenSize(table, originTable) {
if ((!isMerch && !table.querySelector('.col-3'))
|| (isMerch && !table.querySelector('.col-2'))) return;

async function setAriaLabelsForElements(selectArray) {
const config = getConfig();
const ariaLabel = await replaceKey('choose-table-column', config);

selectArray.forEach((item) => {
item.setAttribute('aria-label', ariaLabel);
});
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This logic and the one from setAriaLabelsForExpandableIcons, are quite similar, I'd suggest we combine them into a more general method that we can call. Another tip: there is a replaceKeyArray with which you could get all the placeholders in one go, if that's helpful for the generic method.


const filterChangeEvent = () => {
table.innerHTML = originTable.innerHTML;
reAssignEvents(table);
Expand Down Expand Up @@ -508,6 +526,7 @@ function applyStylesBasedOnScreenSize(table, originTable) {
table.parentElement.insertBefore(filters, table);
table.parentElement.classList.add(`table-${table.classList.contains('merch') ? 'merch-' : ''}section`);
filterChangeEvent();
setAriaLabelsForElements([colSelect0, colSelect1]);
}
};

Expand Down Expand Up @@ -566,6 +585,7 @@ export default function init(el) {
});

handleHighlight(el);
setAriaLabelsForExpandableIcons();
if (isMerch) formatMerchTable(el);

let isDecorated = false;
Expand Down
8 changes: 8 additions & 0 deletions test/blocks/table/mocks/placeholders.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
{
"data": [
{
"key": "choose-table-column",
"value": "choose table column"
}
]
}
Copy link
Contributor

@mokimo mokimo Jan 22, 2025

Choose a reason for hiding this comment

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

Nit: Missing end of file

18 changes: 17 additions & 1 deletion test/blocks/table/table.test.js
Original file line number Diff line number Diff line change
@@ -1,10 +1,15 @@
import { readFile, sendMouse, sendKeys, resetMouse } from '@web/test-runner-commands';
import { expect } from 'chai';
import { MILO_EVENTS } from '../../../libs/utils/utils.js';
import { getConfig, MILO_EVENTS, setConfig } from '../../../libs/utils/utils.js';
import { delay, waitForElement } from '../../helpers/waitfor.js';
import { replaceKey } from '../../../libs/features/placeholders.js';

document.body.innerHTML = await readFile({ path: './mocks/body.html' });
const { default: init } = await import('../../../libs/blocks/table/table.js');
const locales = { '': { ietf: 'en-US', tk: 'hah7vzn.css' } };
const conf = { locales, contentRoot: '/test/blocks/table/mocks' };
setConfig(conf);
const config = getConfig();
Comment on lines +9 to +12
Copy link
Contributor

Choose a reason for hiding this comment

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

These seem to be relevant to a single test, should we scope them to that particular test? I'm not sure how we treat this in other unit tests, might be worth a look.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This seems to not be possible since in table.test.js we are initializing table.js and the calls for aria label get executed immediately and not just for that test.


describe('table and tablemetadata', () => {
beforeEach(() => {
Expand Down Expand Up @@ -111,5 +116,16 @@ describe('table and tablemetadata', () => {
expect(tooltipHeading.childNodes.length).to.equal(2);
expect(tooltipHeading.querySelector('.milo-tooltip, .icon-tooltip')).to.exist;
});

it('should apply aria-label to all selects within .filters on mobile', async () => {
window.innerWidth = 375;
window.dispatchEvent(new Event('resize'));
const ariaLabel = await replaceKey('choose-table-column', config);
const selectElements = document.querySelectorAll('.filters select');

selectElements.forEach((selectElement) => {
expect(selectElement.getAttribute('aria-label')).to.equal(ariaLabel);
});
});
});
});
Loading