fix: address accessibility for pagination and select#21942
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #21942 +/- ##
==========================================
- Coverage 94.89% 94.89% -0.01%
==========================================
Files 539 539
Lines 45015 45019 +4
Branches 6352 6351 -1
==========================================
+ Hits 42716 42719 +3
- Misses 2167 2168 +1
Partials 132 132
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
✅ Deploy Preview for v11-carbon-react ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for v11-carbon-web-components ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
testing is passing locally, but i'm dumb and forgot you have to rebuild before retesting to test the latest changes 😂 i will fix those tests ASAP |
|
The pull request template is incomplete. |
|
@adamalston updated
|
|
I've been thinking about the id concern. looking across the web-components package and there are numerous places where the id attribute is set as a prop but not set to be required, are given a default value I agree that this should be fixed and understand it may be a breaking change. If that's the case then this may be something we want to sort out in a separate issue and PR. For the sake of this PR, it might be easier to omit the changes associated with removing the default id value and address it separately. Thoughts? |
|
I think that sounds reasonable. Once an approach is finalized, maybe it could be standardized. |
|
changes made 👍 |
|
would love to get this merged in ASAP if someone can take a look. thanks! 👍 |
| */ | ||
| @property() | ||
| id = ''; | ||
| id = 'input'; |
There was a problem hiding this comment.
I think #21942 (comment) was addressed. However, some changes were later reverted, understandably, and the issue now appears to have resurfaced.
Does anything need to be updated? My understanding of the current implementation is that unless each consumer overrides this id, it may lead to duplicate values. That could cause problems with selectors and ARIA references.
There was a problem hiding this comment.
based on my understanding of how the shadow dom works, i think it's technically ok due to the encapsulation that web components provide. it's probably still best for users to implement their own select components with unique ids, but i don't think it will actually cause issue to use the same id in different shadow roots.
it's not flagged as an issue by the accessibility checker in the pagination component when both of the select ids are removed
since this is a preexisting implementation i think it would be better to discuss further in a separate issue 👍
sources:
| */ | ||
| @property({ attribute: 'items-per-page-text' }) | ||
| itemsPerPageText = 'Items per page:'; | ||
| itemsPerPageText = ''; |
There was a problem hiding this comment.
Should this prop be marked as required now that it's not set by default?
| expect(label?.textContent?.trim()).to.equal('éléments par page'); | ||
| }); | ||
|
|
||
| it('should respect items-per-page-text attribute when not supplied', async () => { |
There was a problem hiding this comment.
| it('should respect items-per-page-text attribute when not supplied', async () => { | |
| it('should default to an empty label when `items-per-page-text` is omitted', async () => { |
19e73c0
…ystem#21942) * fix: address accessibility for pagination and select * fix: update select wc story * fix: tests * fix: resolve input and select query * fix: minor label margin style * fix: add label slot back into pagination * fix: tests * fix: added missing select ids and testing * fix: remove only from test * fix: add default id back in select * fix: react tests * fix: react tests * fix: docs
Closes #21932
Resolves accessibility violations from the
cds-paginationweb-componentChangelog
New
label-styles-disableprop to thecds-selectweb-component to account for use-cases where the user doesn't want the label to use the defaultcds--labelclass & stylesChanged
cds-paginationthe default value foritemsPerPageTexthas been removedcds-paginationthelabelelements adjacent to thecds-selectcomponents were moved to use thelabel-textslots incds-selectcds-selecttheidprop now has no default value and is appropriately applied to the underlyinglabelandselectelements. prior to this it was just using the hardcoded value ofinputTesting / Reviewing
yarn testpassespaginationcomponent has no violations in the accessibility tab in storybookPR Checklist
As the author of this PR, before marking ready for review, confirm you:
More details can be found in the pull request guide