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

Radio Group: Rating Example #1870

Merged
merged 26 commits into from
Nov 10, 2021
Merged

Radio Group: Rating Example #1870

merged 26 commits into from
Nov 10, 2021

Conversation

jongund
Copy link
Contributor

@jongund jongund commented Apr 23, 2021

I have created a new rating rating group example based on the discussion at the 6 April 2021 meeting.

Features:

  • Uses SVG graphics elements
  • Has high contrast support
  • Code to use APG coding practices
  • Uses role=none on SVG element
  • Sets the CSS property forced-color-adjust to auto on the SVG element.
  • Using stroke-opacity and fill-opacity instead of transparent values for setting stroke and fill colors for the SVG rect used for focus ring for high contrast mode.

Preview rating radio group example

Review checklist


Preview | Diff

@jongund jongund changed the title Radio Group Rating Example Radio Group: Rating Example May 7, 2021
@jongund
Copy link
Contributor Author

jongund commented Jun 22, 2021

For iOS it works, but does not read the radiogroup label when navigating with forms.

@jesdaigle
Copy link
Contributor

@jongund this PR has conflicts.

@jesdaigle
Copy link
Contributor

@jongund there are some pending conflicts.

@jongund
Copy link
Contributor Author

jongund commented Jul 15, 2021

@jesdaigle
I think there is a problem with the commit process with examples/index.html, I don't touch examples/index.html, something in the commit process is changing it and it should be fixed. I tried checking the index.html file out from the main branch, but it still is changing it and causing a conflict. People working on examples, shouldn't need to worry about the examples/index.html page blocking their pull requests then they are not even touching it.

Copy link
Contributor

@a11ydoer a11ydoer left a comment

Choose a reason for hiding this comment

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

@jongund Thanks for reflecting and implementing feedback from APG meeting.

@a11ydoer
Copy link
Contributor

@shirsha would you mind finishing accessibilty review for this example?

@mcking65 mcking65 added the Example Page Related to a page containing an example implementation of a pattern label Oct 18, 2021
@mcking65 mcking65 added this to the 1.2 Release 1 milestone Oct 18, 2021
@a11ydoer a11ydoer self-assigned this Nov 2, 2021
@a11ydoer a11ydoer removed the request for review from shirsha November 5, 2021 17:56
<h2>Accessibility Features</h2>
<ul>
<li>
To highlight the interactive nature of the rating stars a focus ring appears around the star icon that has focus. When a pointer hovers over a star the cursor changes to a pointer and the number of filled stars is updated to include the hovered star. When a pointer leaves a star, the filled stars are restored to indicate the currently checked star.
Copy link
Contributor

Choose a reason for hiding this comment

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

To highlight the interactive nature of the rating stars a focus ring appears around the star icon that has focus.

I don't understand the purpose of this sentence. It sounds like a focus ring appears on the item with focus. That goes without saying; it wouldn't be a special feature to provide a focus indicator.

When a pointer hovers over a star the cursor changes to a pointer and the number of filled stars is updated to include the hovered star. When a pointer leaves a star, the filled stars are restored to indicate the currently checked star.

This sounds like a potential problem because the user would not be able to distinguish between hover and selection. Someone using a mouse to drive magnification might think five stars are checked just because they look checked when the mouse puts their mag view over the fifth star. Do I understand this correctly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the way other rating stars work that I have found, basically hover fills in stars as if they were the current state. Not sure if there is any other good way to distinguish between focus and hover state that does not get visually confusing and also works in high contrast mode. This is similar tot he issue Siri brought up related to hover and focus styling that recently came up for the disclosure menu example.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mcking65 I have updated the description to talk about how changing hover and changing focus update the number of stars that are filled. So if focus is moving then the hover pointer is essentially being ignored. If hover is changing the number of stars filled changes based on the which star is being hovered. Selection of the radio is done through a click or a tap. If the hover moves off the stars it reverts to filling the stars based on the radio that is checked. I think this makes the most sense, but I am open to any other ideas people have.

examples/radio/radio-rating.html Outdated Show resolved Hide resolved
examples/radio/radio-rating.html Outdated Show resolved Hide resolved
@a11ydoer
Copy link
Contributor

@mcking65 High contrast(white/dark theme) on PC and Mac with Firefox, Chrome works well.

@mcking65
Copy link
Contributor

@a11ydoer
Do you think this is ready for me to merge then? There is still one windows accessibility review not checked off in the review list, but thinking you have done that now?

@a11ydoer
Copy link
Contributor

@mcking65 I also checked rating example with voiceover and NVDA. The example works well.

@a11ydoer
Copy link
Contributor

yes, @mcking65 I checked the example on windows too.

@mcking65 mcking65 merged commit fc85cba into main Nov 10, 2021
@mcking65 mcking65 deleted the radio-rating branch November 10, 2021 19:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Example Page Related to a page containing an example implementation of a pattern
Development

Successfully merging this pull request may close these issues.

4 participants