Skip to content

Accessibility [FastPass]: Fix settings general missing aria input labels #3542

Merged
k8s-ci-robot merged 5 commits intokubernetes-sigs:mainfrom
vyncent-t:settings-aria-input-label-fix
Jul 29, 2025
Merged

Accessibility [FastPass]: Fix settings general missing aria input labels #3542
k8s-ci-robot merged 5 commits intokubernetes-sigs:mainfrom
vyncent-t:settings-aria-input-label-fix

Conversation

@vyncent-t
Copy link
Contributor

@vyncent-t vyncent-t commented Jun 27, 2025

Summary

This PR fixes accessibility related issues for the settings page, mainly just the issues found by FastPass on the General tab.

These changes are part of a larger accessibility compliance goal from ADO (not sure if linking is allowed).

Issues are found using FastPass automated checks, feel free to download the tool and doublecheck if interested.

Changes

These aria labels are now added to the settings view

  • Add aria labelled by for Sort sidebar items alphabetically menu switch
  • Add aria labelled by for Use evict for pod deletion menu switch
  • Add aria label for Number of rows for tables source component
  • Add aria label for Timezone to display for dates source component
  • Add aria label for drawer mode switch
  • Updates Language to use typography (was plain text before)

Note

I have ran into issues with applying some aria attributes to the MUI components directly. I found that just attaching aira-label / aira-labelledby etc would only attach to the wrapper for the MUI component and not the input element itself. By adding the inputProps field we can attach aria labels to the actual input element.

FastPass checks this solves

Issue for Number of rows for tables

image image

Issue for Timezone to display for dates

image image

issue for Sort sidebar items alphabetically

image image

issue for Use evict for pod deletion

image image

@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 27, 2025
@k8s-ci-robot k8s-ci-robot requested review from ashu8912 and skoeva June 27, 2025 20:26
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 27, 2025
@vyncent-t vyncent-t self-assigned this Jun 30, 2025
@vyncent-t vyncent-t force-pushed the settings-aria-input-label-fix branch 2 times, most recently from acf7f44 to 0fc8a97 Compare July 7, 2025 15:49
@vyncent-t vyncent-t changed the title Settings aria input label fix Frontend Accessibility Tasks - Fix missing aria input labels Jul 7, 2025
@vyncent-t vyncent-t force-pushed the settings-aria-input-label-fix branch from 0fc8a97 to 714d7ca Compare July 7, 2025 15:58
@vyncent-t vyncent-t marked this pull request as ready for review July 7, 2025 16:16
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 7, 2025
@k8s-ci-robot k8s-ci-robot requested review from illume and sniok July 7, 2025 16:16
@vyncent-t vyncent-t added the a11y Accessibility related issues label Jul 7, 2025
@vyncent-t vyncent-t changed the title Frontend Accessibility Tasks - Fix missing aria input labels Frontend Accessibility Tasks - Fix settings general missing aria input labels Jul 7, 2025
Comment on lines 160 to 162
inputProps={{
'aria-label': `option ${selectedValue}`,
}}
Copy link
Contributor

Choose a reason for hiding this comment

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

I am surprised this is not set by default. Can you elaborate what is the test failing here?
In any case, a solution would need the aria-label value to be translatable. Not sure if "option" is a good idea here either. AFAIU, screen readers already should understand this is part of a select.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The FastPass error check for the NumRowsInput:

image

This error says it is missing an aria-label or aria-labelledby.

Also tried adding an aria-label prop here, still returns that same error above and additionally introduces need for "role" semantics

image

since adding them as props normally seems to just add to a MUI wrapper and not the input element itself I use inputProps

Copy link
Contributor Author

@vyncent-t vyncent-t Jul 7, 2025

Choose a reason for hiding this comment

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

Not sure if "option" is a good idea here either.

I agree, could use some ideas for a more fitting word than 'option'.

Option just seemed to be the a fitting enough catch all word for this select of preset numbers or a custom input (was between using "option" or "choice")

maybe something like

          inputProps={{
            'aria-label': t('translation|{{ value }} number of rows', { value: selectedValue }),
          }}

AFAIU, screen readers already should understand this is part of a select.

An alternative method would be to try to wrap <MenuItem> inside some label and link it via an id= and aria-labelledby.

let me know if that is a good fit

Copy link
Contributor Author

Choose a reason for hiding this comment

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

^^^

update: I was able to use the same method of adding an id to the name label that I am using elsewhere for the settings table. This now links and references the name label directly without need for "options" keyword or the like. These changes also pass the check

variant="outlined"
inputProps={{
...params.inputProps,
'aria-label': t('Timezone'),
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment as to why this is needed. Can you double check if MUI is not setting this and we're just missing some param?

Copy link
Contributor Author

@vyncent-t vyncent-t Jul 7, 2025

Choose a reason for hiding this comment

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

For the timezone select component the FastPass error prints as follows:

image

It does not look like MUI has any aria labels or labelled by here.

I have tried adding just the aira-label prop to <Autocomplete> and <TextField> individually, which still returns the same check error

I have also tried using aria-labelledby="cluster-selector-autocomplete" within <TextField>, since <Autocomplete> wraps it via renderInput prop, this also returns the same check error.

I opted for adding the aria label to the spread within renderInput method as per the docs https://mui.com/material-ui/api/autocomplete/#props

@vyncent-t vyncent-t force-pushed the settings-aria-input-label-fix branch 3 times, most recently from 7751a21 to bef08fb Compare July 7, 2025 19:36
@vyncent-t vyncent-t marked this pull request as draft July 7, 2025 19:39
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 7, 2025
@vyncent-t vyncent-t force-pushed the settings-aria-input-label-fix branch 3 times, most recently from d5e7d8c to 5d3b694 Compare July 8, 2025 13:00
@vyncent-t vyncent-t marked this pull request as ready for review July 8, 2025 13:35
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 8, 2025
@vyncent-t vyncent-t force-pushed the settings-aria-input-label-fix branch from 5d3b694 to 16fa8b1 Compare July 8, 2025 17:01
@illume illume removed the request for review from sniok July 8, 2025 21:06
@vyncent-t vyncent-t force-pushed the settings-aria-input-label-fix branch 4 times, most recently from a991647 to cb41779 Compare July 23, 2025 18:08
@vyncent-t vyncent-t changed the title Frontend Accessibility Tasks - Fix settings general missing aria input labels Accessibility [FastPass]: Fix settings general missing aria input labels Jul 24, 2025
@vyncent-t vyncent-t force-pushed the settings-aria-input-label-fix branch from cb41779 to afa4c93 Compare July 24, 2025 16:18
@vyncent-t
Copy link
Contributor Author

@joaquimrocha

if the changes for NameValueTable are good here I can remove the duplicate work in #3598 and it should be ready for final review

@sniok sniok requested a review from Copilot July 25, 2025 08:00
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR addresses accessibility issues by adding proper ARIA labels and using MUI Typography components for better screen reader support. The changes are focused on improving the accessibility compliance of various UI components, particularly for form controls and data display elements.

  • Update MUI component patterns to use Typography wrapper for better semantic structure
  • Add proper ARIA labels to form controls and input elements for screen reader accessibility
  • Standardize label presentation across the application with consistent Typography components

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 28, 2025
@vyncent-t vyncent-t force-pushed the settings-aria-input-label-fix branch from afa4c93 to 035ae85 Compare July 28, 2025 18:32
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 28, 2025
@vyncent-t vyncent-t force-pushed the settings-aria-input-label-fix branch from 035ae85 to 4d1b87c Compare July 29, 2025 15:56
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jul 29, 2025
@vyncent-t vyncent-t force-pushed the settings-aria-input-label-fix branch from 4d1b87c to 14f6624 Compare July 29, 2025 16:18
@vyncent-t vyncent-t force-pushed the settings-aria-input-label-fix branch from 14f6624 to 5c14630 Compare July 29, 2025 16:44
Copy link
Contributor

@sniok sniok left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 29, 2025
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sniok, vyncent-t

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 29, 2025
@k8s-ci-robot k8s-ci-robot merged commit d4cc0f9 into kubernetes-sigs:main Jul 29, 2025
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

a11y Accessibility related issues approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants