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
13 changes: 11 additions & 2 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 @@ -63,7 +64,7 @@ function handleHeading(table, headingCols) {
});

const headingContent = createTag('div', { class: 'heading-content' });
const headingButton = createTag('div', { class: 'heading-button' });
const headingButton = createTag('div', { class: 'heading-button', role: 'cell' });

[...elements].forEach((e) => {
if (e.classList.contains('pricing') && isPriceBottom) headingButton.appendChild(e);
Expand Down Expand Up @@ -448,6 +449,13 @@ function applyStylesBasedOnScreenSize(table, originTable) {
if ((!isMerch && !table.querySelector('.col-3'))
|| (isMerch && !table.querySelector('.col-2'))) return;

async function setAriaLabelsForElements(first, second) {
const config = getConfig();
const ariaLabel = await replaceKey('choose-table-column', config);
first.setAttribute('aria-label', ariaLabel);
second.setAttribute('aria-label', ariaLabel);
}

const filterChangeEvent = () => {
table.innerHTML = originTable.innerHTML;
reAssignEvents(table);
Expand Down Expand Up @@ -508,6 +516,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);
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd like to future-proof this a bit, in case we'll have another number of columns visible on different breakpoints. So let's send these in an Array and have a single argument for setAriaLabelsForElements that iterates over the array's elements to set the aria-label.

}
};

Expand Down
15 changes: 14 additions & 1 deletion test/blocks/table/table.test.js
Original file line number Diff line number Diff line change
@@ -1,10 +1,13 @@
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';

document.body.innerHTML = await readFile({ path: './mocks/body.html' });
const { default: init } = await import('../../../libs/blocks/table/table.js');
const config = getConfig();
config.env = { locale: { contentRoot: `${window.location.origin}` } };
setConfig(config);
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't this end up actually making the placeholders call during the unit tests? We generally avoid network requests, so this should be mocked. I suspect the palceholders tests should have a way of doing this already.


describe('table and tablemetadata', () => {
beforeEach(() => {
Expand Down Expand Up @@ -111,5 +114,15 @@ 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'));
await delay(500);
Copy link
Contributor

Choose a reason for hiding this comment

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

We try to avoid any wait time in our unit tests as to not delay the whole suite. You can look up await clock.runAllAsync(); in the codebase and that should provide a hint on how to wait for things to happen without impact on run time.

const selectElements = document.querySelectorAll('.filters select');
selectElements.forEach((selectElement) => {
expect(selectElement.getAttribute('aria-label')).to.equal('choose table column');
Copy link
Contributor

Choose a reason for hiding this comment

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

Again, we should probably mock the placeholders logic/values, so we might need to adapt the expected value here as well.

});
});
});
});
Loading