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

Rating, ToggleGroup: Improve accessibility #9058

Merged
merged 8 commits into from
May 26, 2024
Merged

Conversation

danielchalmers
Copy link
Contributor

@danielchalmers danielchalmers commented May 25, 2024

Description

MudRating

  • Add role="radiogroup"
  • Add aria-readonly="@ReadOnly"
  • Fix mud-disabled class not applied to root so :active style was still applied
  • Format razor & C#

MudRatingItem

  • Add role="radio"
  • Add aria-checked="@Checked" for readonly items
  • Add localized label ("{0} Rating")
  • Format razor & C#

MudToggleGroup

MudToggleItem

  • Add role="checkbox"
  • Add aria-checked="@Selected"

How Has This Been Tested?

visually

Type of Changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation (fix or improvement to the website or code docs)

Checklist

  • The PR is submitted to the correct branch (dev).
  • My code follows the code style of this project.
  • I've added relevant tests.

MudRating
-  Add `role="radiogroup"`
- Format razor

MudRatingItem
- Add `role="radio"`

MudToggleGroup
-  Add `role="group"`
- Format razor
- still needs proper keyboard support (Waiting for MudBlazor#9003)

MudToggleItem
- Add `role="checkbox"`
@github-actions github-actions bot added enhancement New feature or request PR: needs review labels May 25, 2024
Copy link

codecov bot commented May 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.64%. Comparing base (28bc599) to head (905adae).
Report is 236 commits behind head on dev.

Additional details and impacted files
@@            Coverage Diff             @@
##              dev    #9058      +/-   ##
==========================================
+ Coverage   89.82%   90.64%   +0.81%     
==========================================
  Files         412      398      -14     
  Lines       11878    12374     +496     
  Branches     2364     2403      +39     
==========================================
+ Hits        10670    11216     +546     
+ Misses        681      620      -61     
- Partials      527      538      +11     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@danielchalmers
Copy link
Contributor Author

cc @igotinfected 1 of 3

Copy link
Member

@igotinfected igotinfected left a comment

Choose a reason for hiding this comment

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

Added a few suggestions, as usual huge thanks for the work!

src/MudBlazor/Components/Rating/MudRating.razor Outdated Show resolved Hide resolved
src/MudBlazor/Components/Rating/MudRatingItem.razor Outdated Show resolved Hide resolved
src/MudBlazor/Components/Rating/MudRating.razor Outdated Show resolved Hide resolved
@danielchalmers danielchalmers marked this pull request as draft May 25, 2024 18:30
@danielchalmers

This comment was marked as outdated.

@danielchalmers
Copy link
Contributor Author

danielchalmers commented May 25, 2024

What do you think now @igotinfected? Should be a good compromise i think

Maybe the ToggleItem needs a localizable label like MudRatingItem?

Also getting ARIA attributes must conform to valid values: Invalid ARIA attribute value: aria-checked="" on the ToggleItem. Any idea why?

Is there a difference in aria attributes and regular ones?
Here's a snippet I found: aria-checked="@(BoolValue.ToString().ToLower())" checked="@BoolValue".
One formats and one doesn't.

@igotinfected
Copy link
Member

igotinfected commented May 25, 2024

LGTM!

Maybe the ToggleItem needs a localizable label like MudRatingItem?

Users can add aria-label to MudToggleItem directly, should be fine?

Also getting ARIA attributes must conform to valid values: Invalid ARIA attribute value: aria-checked="" on the ToggleItem. Any idea why?

Pretty sure I ran into the same problem in one of my accessibility PRs... IIRC it's because aria-checked (and a bunch of other aria attributes) only have two valid values: true or false. This implies that they cannot be an empty string, or be used like disabled, or readonly, or checked, which are implicitly == true when no value is provided.

I think Blazor outputs attribute="" when passed a boolean true, hence the need to pass it a string like the snippet you posted: aria-checked="@(BoolValue.ToString().ToLower())" checked="@BoolValue"

@danielchalmers
Copy link
Contributor Author

@henon Ready to review

@danielchalmers danielchalmers marked this pull request as ready for review May 25, 2024 22:41
@danielchalmers danielchalmers requested a review from henon May 25, 2024 22:41
@henon henon merged commit 945a237 into MudBlazor:dev May 26, 2024
4 checks passed
@henon
Copy link
Collaborator

henon commented May 26, 2024

Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants