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] Centering issue in Safari #19846

Closed
2 tasks done
YuryShkoda opened this issue Feb 25, 2020 · 19 comments · Fixed by #23239
Closed
2 tasks done

[Radio] Centering issue in Safari #19846

YuryShkoda opened this issue Feb 25, 2020 · 19 comments · Fixed by #23239
Assignees
Labels
bug 🐛 Something doesn't work component: radio This is the name of the generic UI component, not the React module! good first issue Great for first contributions. Enable to learn the contribution process. hacktoberfest https://hacktoberfest.digitalocean.com/

Comments

@YuryShkoda
Copy link

YuryShkoda commented Feb 25, 2020

Dot in radio button is not centered in Safari for 13 and 15 inches screens. On iphone (checked on iphone Xs), chrome and firefox the dot is perfectly centered.

  • The issue is present in the latest release.
  • I have searched the issues of this repository and believe that this is not a duplicate.

Steps to Reproduce 🕹

That is how I style my radio button:

const RadioBtn = withStyles({
    root: {
        color: 'default',
        '&$checked': {
            color: '#604f93',
        },
    },
    checked: {},
})(props => <Radio color="default" {...props} />)

Live example of this issue can be found here: https://www.erudicat.com/heuristic-question-147 or https://www.erudicat.com/monte-carlo-analysis-question-337

Context 🔦

The majority of css rules in my project are scoped to particular components and the fact that I have such issue only in one browser make me assume that the issue is probably with MUI.

Thank you in advance!

Your Environment 🌎

Tech Version
Material-UI v4.8.2
React 16.12.0
Browser Safari v13.0.5
@joshwooding
Copy link
Member

joshwooding commented Feb 25, 2020

Hmm, looks like it might be slightly off.

Above website:
765BA58B-49A0-484E-B90C-1E31E62BC671

Docs:
96C14D40-C2BA-4317-8260-8282F8A66822

Tested on an iPhone. The two examples above don’t look exactly the same either so maybe this doesn’t prove anything...

@joshwooding joshwooding added the component: radio This is the name of the generic UI component, not the React module! label Feb 25, 2020
@oliviertassinari oliviertassinari added bug 🐛 Something doesn't work good first issue Great for first contributions. Enable to learn the contribution process. labels Feb 25, 2020
@oliviertassinari
Copy link
Member

@joshwooding Thanks for the confirmation, the solution seems simple: apply the scale transformation to the overlapping svg elements.

@oliviertassinari
Copy link
Member

oliviertassinari commented Feb 25, 2020

Do you confirm the following?

diff --git a/packages/material-ui/src/Radio/RadioButtonIcon.js b/packages/material-ui/src/Radio/RadioButtonIcon.js
index 8316fe4ed..e8746b1aa 100644
--- a/packages/material-ui/src/Radio/RadioButtonIcon.js
+++ b/packages/material-ui/src/Radio/RadioButtonIcon.js
@@ -9,7 +9,7 @@ export const styles = theme => ({
   root: {
     position: 'relative',
     display: 'flex',
-    '&$checked $layer': {
+    '&$checked $dot': {
       transform: 'scale(1)',
       transition: theme.transitions.create('transform', {
         easing: theme.transitions.easing.easeOut,
@@ -17,7 +17,11 @@ export const styles = theme => ({
       }),
     },
   },
-  layer: {
+  checked: {},
+  background: {
+    transform: 'scale(1)',
+  },
+  dot: {
     left: 0,
     position: 'absolute',
     transform: 'scale(0)',
@@ -26,7 +30,6 @@ export const styles = theme => ({
       duration: theme.transitions.duration.shortest,
     }),
   },
-  checked: {},
 });

Maybe with a comment too?

@oliviertassinari oliviertassinari changed the title Dot in radio button is not centered in Safari [Radio] Centering issue in Safari Feb 25, 2020
@joshwooding
Copy link
Member

Can confirm that worked :)

Implementation comment for someone who will take this on: We probably don't want to change layer to dot currently just in case it breaks styles. It might be something to include in v5

@oliviertassinari
Copy link
Member

Well, it's a private component @ignore - internal component.

@joshwooding
Copy link
Member

Do we classify the CSS it exposes as private then?

@oliviertassinari
Copy link
Member

As far as I know, developers can't interact with the style of private components, the class names are non-deterministic, not accessible with the theme.

@parthjawale
Copy link

parthjawale commented Feb 27, 2020

Hey, can I work on this?
This would be my first issue.

@joshwooding
Copy link
Member

@parthjawale Sure! Hopefully the first of many :)

@parthjawale
Copy link

Great and thanks!

@parthjawale
Copy link

parthjawale commented Feb 27, 2020

@joshwooding I tested it on my iPhone 11 Pro. For some reason all radio buttons behave differently.
Tested on the documentation label placement section (https://material-ui.com/components/radio-buttons/#label-placement)
The dot is placed at the left in the first and second case, right for the third case and at the exact center for the last one.

@oliviertassinari The scale transformation fix seems to work sometimes and sometimes not.

Working on a fix.

@oliviertassinari
Copy link
Member

@parthjawale Do you have an example when the scale(1) approach doesn't work?

@YuryShkoda
Copy link
Author

Hey folks,

Do you have any suggestion how I can fix it in my project until this issue will be fixed in upcoming release?
My project is a PMP Exam Simulator with 1k+ sample questions meaning there are 4k+ radio buttons and it is very important for me to fix it asap.

Thank you in advance.

@joshwooding
Copy link
Member

@YuryShkoda you should be able to apply the fix in your theme using overrides. I’ll post a code snippet later.

@oliviertassinari
Copy link
Member

oliviertassinari commented Mar 11, 2020

@YuryShkoda Do you want to submit a pull request with the proposed fix :)? #19846 (comment)

@YuryShkoda
Copy link
Author

YuryShkoda commented Mar 11, 2020

@oliviertassinari I've been trying to implement fix, but it was working for some screen resolutions only and I have limited options for tests using real devices, that is why I'm afraid to bring more harm than use. And I have too many radio buttons across my project to risk.

@joshwooding
Copy link
Member

@YuryShkoda Sorry it took so long. https://codesandbox.io/s/material-demo-sueb1 - but if you think the fix above won't work then I'm not sure this will work for you. Also the css selector is fragile so use at your own risk :)

@YuryShkoda
Copy link
Author

Just tried the fix using scale() and it looks perfect.
root: { "& svg:first-child": { transform: "scale(1)" } }
I was able to test on 4 real screen resolutions.
Huge thanks to everyone!

@mbrookes mbrookes added the hacktoberfest https://hacktoberfest.digitalocean.com/ label Oct 10, 2020
@anasufana
Copy link
Contributor

Could I try this one?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work component: radio This is the name of the generic UI component, not the React module! good first issue Great for first contributions. Enable to learn the contribution process. hacktoberfest https://hacktoberfest.digitalocean.com/
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants