[MWPW-165791] Table select aria-label and column cell role added #3513
Conversation
DKos95
requested review from
overmyheadandbody,
mokimo,
zagi25 and
robert-bogos
|
Hello, I'm the AEM Code Sync Bot and I will run some actions to deploy your branch and validate page speed.
|
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## stage #3513 +/- ##
========================================
Coverage 96.46% 96.47%
========================================
Files 259 260 +1
Lines 60217 60601 +384
========================================
+ Hits 58090 58465 +375
- Misses 2127 2136 +9 ☔ View full report in Codecov by Sentry. |
DKos95
force-pushed
the
table-select-cell
branch
from
d562bf0 to
7ca0dd5
Compare
libs/blocks/table/table.js
Outdated
| @@ -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); | |||
There was a problem hiding this comment.
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.
test/blocks/table/table.test.js
Outdated
| config.env = { locale: { contentRoot: `${window.location.origin}` } }; | ||
| setConfig(config); |
There was a problem hiding this comment.
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.
test/blocks/table/table.test.js
Outdated
| it('should apply aria-label to all selects within .filters on mobile', async () => { | ||
| window.innerWidth = 375; | ||
| window.dispatchEvent(new Event('resize')); | ||
| await delay(500); |
There was a problem hiding this comment.
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.
test/blocks/table/table.test.js
Outdated
| await delay(500); | ||
| const selectElements = document.querySelectorAll('.filters select'); | ||
| selectElements.forEach((selectElement) => { | ||
| expect(selectElement.getAttribute('aria-label')).to.equal('choose table column'); |
There was a problem hiding this comment.
Again, we should probably mock the placeholders logic/values, so we might need to adapt the expected value here as well.
| "value": "choose table column" | ||
| } | ||
| ] | ||
| } |
libs/blocks/table/table.js
Outdated
| @@ -153,6 +153,15 @@ function handleAddOnContent(table) { | |||
| table.addEventListener('mas:resolved', debounce(() => { handleEqualHeight(table, '.row-heading'); })); | |||
| } | |||
|
|
|||
| async function setAriaLabelsForExpandableIcons() { | |||
There was a problem hiding this comment.
Should we have a unit test for this as well?
libs/blocks/table/table.js
Outdated
| async function setAriaLabelsForExpandableIcons() { | ||
| const config = getConfig(); | ||
| const ariaLabel = await replaceKey('toggle-row', config); | ||
| const icons = document.querySelectorAll('.icon.expand[role="button"]'); |
There was a problem hiding this comment.
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.
libs/blocks/table/table.js
Outdated
| async function setAriaLabelsForElements(selectArray) { | ||
| const config = getConfig(); | ||
| const ariaLabel = await replaceKey('choose-table-column', config); | ||
|
|
||
| selectArray.forEach((item) => { | ||
| item.setAttribute('aria-label', ariaLabel); | ||
| }); | ||
| } |
There was a problem hiding this comment.
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 locales = { '': { ietf: 'en-US', tk: 'hah7vzn.css' } }; | ||
| const conf = { locales, contentRoot: '/test/blocks/table/mocks' }; | ||
| setConfig(conf); | ||
| const config = getConfig(); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
This resolves the issues where a table row was missing an adequate role and select on mobile was missing a label causing accessibility issues.
Resolves: MWPW-165791
Test URLs: