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

Fix focus visibility for input components in site setup #6544

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

Abhishek-17h
Copy link
Contributor


If your pull request closes an open issue, include the exact text below, immediately followed by the issue number. When your pull request gets merged, then that issue will close automatically.
Fixed missing visible focus for multiple select inputs across components using the SelectStyling widget in Site Setup. The fix ensures proper focus indication for accessibility across all instances of the widget.
Update snapshots to match new UI behavior.

Screenshots after Fix :
solve1
solve2

Previous behavior is provided in #6348

Closes #6348

Copy link

netlify bot commented Dec 17, 2024

Deploy Preview for plone-components canceled.

Name Link
🔨 Latest commit e8956f9
🔍 Latest deploy log https://app.netlify.com/sites/plone-components/deploys/67659356b72e070008c9e57b

@Abhishek-17h Abhishek-17h force-pushed the bugfix/fix-focus-visibility-dropdown branch 2 times, most recently from c47d5f2 to 039bab3 Compare December 17, 2024 12:14
@Abhishek-17h
Copy link
Contributor Author

@stevepiercy @sneridagh can you please provide review on this?

@stevepiercy
Copy link
Collaborator

Refs: #6504

Copy link
Collaborator

@stevepiercy stevepiercy left a comment

Choose a reason for hiding this comment

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

News looks good, with a minor tweak.

This needs review from the @plone/volto-accessibility Team, so I assigned them as reviewers.

I also have a question about the source of the colors that you chose.

This looks like a nice improvement. Thank you!

packages/volto/news/6348.bugfix Outdated Show resolved Hide resolved
':active': {
backgroundColor: null,
backgroundColor: state.isFocused ? '#e5f4fb' : null,
Copy link
Collaborator

Choose a reason for hiding this comment

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

@Abhishek-17h out of curiosity, where did you get the colors in this PR? I know we're picky about being consistent with colors, especially to ensure sufficient contrast for accessibility. I don't know where we capture them all in a single place, like a style guide.

Hopefully someone from the @plone/volto-accessibility team can inform me. If this thing does not exist, it might be a good thing to add to our accessibility PLIP #6504.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with @stevepiercy. I think we at least need documentation with use cases, such as focus and minimal contrast for the main font color, links, and buttons. It could indeed be a nice touch for the PLIP. I'll add a comment to the PLIP.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@stevepiercy I understand that the Volto team is particular about color choices, which is why I used an existing color from the SelectStyling and applied a lighter version of it. This approach ensures that users can easily differentiate between selected and hovered inputs.
I didn’t use any specific tools to choose the color but made the adjustment to align with accessibility considerations and consistency within the existing design.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@stevepiercy Thank you for your kind words! I'm glad to contribute to improving Volto. 😊

Copy link
Collaborator

@stevepiercy stevepiercy left a comment

Choose a reason for hiding this comment

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

I approve the news item, but this still needs a technical review.

@Abhishek-17h Abhishek-17h force-pushed the bugfix/fix-focus-visibility-dropdown branch 2 times, most recently from 4c12287 to a905bc1 Compare December 19, 2024 17:15
@Abhishek-17h
Copy link
Contributor Author

@ichim-david @JeffersonBledsoe @Wagner3UB can u please review this .

@Abhishek-17h Abhishek-17h force-pushed the bugfix/fix-focus-visibility-dropdown branch from a905bc1 to e8956f9 Compare December 20, 2024 15:55
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.

a11y - site setup - Date and Time - input fields - cms ui - focus visible
3 participants