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(rating): fix keyboard navigation styles bug on Safari #34687

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

vanyaxk
Copy link
Contributor

@vanyaxk vanyaxk commented Oct 9, 2022

This PR fixes #34194; feels a little hacky but there is no better way to do it as far as my knowledge goes

@mui-bot
Copy link

mui-bot commented Oct 9, 2022

Details of bundle changes

Generated by 🚫 dangerJS against bfb8436

@@ -102,7 +102,7 @@ export function teardown(doc: Document): void {
function isFocusVisible(event: React.FocusEvent): boolean {
const { target } = event;
try {
return target.matches(':focus-visible');
return target.isEqualNode(document.activeElement);
Copy link
Member

Choose a reason for hiding this comment

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

This is wrong, focus-visible !== focus

Suggested change
return target.isEqualNode(document.activeElement);
return target.matches(':focus-visible');

Copy link
Contributor Author

Choose a reason for hiding this comment

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

true, though I believe this hook should be renamed or approached differently because current logic does not work in Safari

Copy link
Member

Choose a reason for hiding this comment

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

because current logic does not work in Safari

Do you have a reproduction? As far as I know, it works on Safari. Be aware that how tabs work on Safari is different than on Chrome, the browser doesn't let you tab focus the same elements.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, the bug is reproducible as described in the issue - the :focus-visible implementation in Safari does not work as intended, besides, it hadn't been supported until recently

I believe my solution to be a bit more bug-proof for Safari and other browsers than this one

packages/mui-material/src/Rating/Rating.js Outdated Show resolved Hide resolved
@oliviertassinari oliviertassinari added the PR: needs test The pull request can't be merged label Oct 9, 2022
Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

How can we reproduce the issue?

@vanyaxk vanyaxk force-pushed the fix/safari-problems-with-keyboard-navigation branch from 5266938 to bfb8436 Compare October 9, 2022 17:35
@vanyaxk
Copy link
Contributor Author

vanyaxk commented Oct 9, 2022

This can be tested via this demo (any of the components within it): https://mui.com/material-ui/react-rating/#main-content

@oliviertassinari
Copy link
Member

oliviertassinari commented Oct 9, 2022

What are the steps? Is there an existing issue open about it?

@oliviertassinari oliviertassinari added status: waiting for author Issue with insufficient information and removed PR: needs test The pull request can't be merged labels Oct 9, 2022
@vanyaxk
Copy link
Contributor Author

vanyaxk commented Oct 9, 2022

Ugh, sorry, linked the wrong issue in description - updated the link, the problem in the issue at hand is 100% reproducible

@github-actions github-actions bot removed the status: waiting for author Issue with insufficient information label Oct 9, 2022
@michaldudak michaldudak added the component: rating This is the name of the generic UI component, not the React module! label Oct 12, 2022
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: rating This is the name of the generic UI component, not the React module! PR: out-of-date The pull request has merge conflicts and can't be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Rating] For keyboard users on Safari browser, Unable to see effects and outline on focus / active stars
4 participants