MWPW-162026 [Milo][Sev2][Catalog] Group of checkboxes is missing FIELDSET … #3518
Conversation
…DSET element - filter checkboxes
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## stage #3518 +/- ##
=======================================
Coverage 96.49% 96.49%
=======================================
Files 260 260
Lines 60763 60776 +13
=======================================
+ Hits 58632 58647 +15
+ Misses 2131 2129 -2 ☔ View full report in Codecov by Sentry. |
| const groupIdEl = createTag('div', { class: 'invisible-and-shrank', id }); | ||
| groupIdEl.textContent = this.sidenavCheckboxTitle; | ||
| this.append(groupIdEl); | ||
| this.querySelectorAll('sp-checkbox').forEach((checkboxEl) => { |
There was a problem hiding this comment.
i am not sure i like the idea of merch-sidenav-checkbox-group being bound to sp-checkbox.
can't we add this 'role' attribute when we create those 'sp-checkboxes'? https://github.com/adobecom/cc/blob/stage/creativecloud/blocks/sidenav/sidenav.js#L79
I understand it means we need to rely on some stable id, so not sure if its better or not. maybe a bit less confusing for devs
There was a problem hiding this comment.
I know the JIRA recommends a few options, but can we try the OOTB SWC approach with sp-field-group
https://opensource.adobe.com/spectrum-web-components/components/field-group/
Before implementing this solution, the accessibility tool can be run against checkboxes in the SWC documentation
if it doesn't complain we can follow.
There was a problem hiding this comment.
@yesil I tried your idea directly on catalog page
it doesn't work. :( Screen reader ignores the word "Types" although it is there twice, once in element "label" and once in attribute "label"
@3ch023 we had this in CC code before but then it was requested to move to Milo
@npeltier I can move that "types" element from shadow to light dom but again I need to iterate through checkboxes to set attributes 'aria-labelledby', and that is where Mariia complains about
So, option A : I do it in CC code in sidenav.js, I move the H3 element from shadow to light dom
option B : I do the same thing in milo code
For both options one reviewer will not be happy.
@3ch023 I could also try to iterate through direct children of merch-sidenav-checkbox-group, without bounding to sp-checkbox, in that case it would be OK to do it in Milo code, right? I think that is the only solution where everyone is happy. Let me try.
There was a problem hiding this comment.
I am fine with any solution you pick, its just the current one looked 'hacked' to me. and i wonder if there is a better way. (first sidenav block creates elements, then checkout-group modifies it's children and itself)
I've read @npeltier new suggestion a few times and didn't understand it still
There was a problem hiding this comment.
thanks @3ch023
looked at it again. I guess @bozojovicic it would be better to pull out the group headings of the shadow DOM to the light DOM and you should be good.
We should not duplicate content and hack around this.
| const groupIdEl = createTag('div', { class: 'invisible-and-shrank', id }); | ||
| groupIdEl.textContent = this.sidenavCheckboxTitle; | ||
| this.append(groupIdEl); | ||
| this.querySelectorAll('sp-checkbox').forEach((checkboxEl) => { |
There was a problem hiding this comment.
I know the JIRA recommends a few options, but can we try the OOTB SWC approach with sp-field-group
https://opensource.adobe.com/spectrum-web-components/components/field-group/
Before implementing this solution, the accessibility tool can be run against checkboxes in the SWC documentation
if it doesn't complain we can follow.
…DSET element - filter checkboxes
|
@yesil @npeltier @3ch023 I think finally here is the solution which covers it all and works.
|
There was a problem hiding this comment.
works for me.
if/when we move sidenav block to Milo we probably should move 'sidenav-checkbox-group-title' creation to sidenav block. and keep merch-sidenav-checkbox-group more simple
i think its about generat expectation that web component will create shadow dom, but an outer actor creates the light dom children
…DSET element - filter checkboxes
… element - filter checkboxes
A11Y fix - In
merch-sidenav-checkbox-groupFor Type Desktop/Mobile/Web checkbox group ARIA group/labelledby technique applied to announce the grouping as a part of the checkboxes. This does not work if we have aria-labelledby="elementId" and #elementId is in shadow DOM so I had to create new hidden DIV, outside shadow DOM, which will have that ID and contain the text "Types".Test page : /products/catalog?milolibs=mwpw162026a11y--milo--bozojovicic
Resolves: MWPW-162026
Test URLs: