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

[SwitchBase] Ignore focus and blur events #18303

Closed
wants to merge 2 commits into from

Conversation

rbrishabh
Copy link
Contributor

Fixes #18210

Hey @oliviertassinari!
The very first attempt to solve the issue. I just added a noFocus option that can be passed as a prop to Form Label Component. But it is still not a default for checkboxes, radios or switches.

Is this the correct way to go about this?

Thanks!

@rbrishabh rbrishabh changed the title adding noFocus option for form-label Fix - adding noFocus option for form-label Nov 10, 2019
@mui-pr-bot
Copy link

mui-pr-bot commented Nov 10, 2019

Details of bundle changes.

Comparing: e0dd45f...7f846b2

bundle Size Change Size Gzip Change Gzip
Checkbox ▼ -151 B (-0.19% ) 80 kB ▼ -65 B (-0.26% ) 25.1 kB
Radio ▼ -151 B (-0.19% ) 80.9 kB ▼ -61 B (-0.24% ) 25.4 kB
Switch ▼ -151 B (-0.19% ) 79.3 kB ▼ -59 B (-0.24% ) 24.6 kB
@material-ui/core[umd] ▼ -151 B (-0.05% ) 310 kB ▼ -57 B (-0.06% ) 89.1 kB
@material-ui/core ▼ -151 B (-0.04% ) 350 kB ▼ -47 B (-0.05% ) 95.8 kB
@material-ui/lab -- 171 kB -- 51.6 kB
@material-ui/styles -- 50.8 kB -- 15.4 kB
@material-ui/system -- 14.8 kB -- 4.06 kB
AppBar -- 62.2 kB -- 19.5 kB
Autocomplete -- 126 kB -- 39.8 kB
Avatar -- 61.2 kB -- 19.3 kB
Backdrop -- 66.2 kB -- 20.4 kB
Badge -- 63.8 kB -- 19.7 kB
BottomNavigation -- 60.8 kB -- 19 kB
BottomNavigationAction -- 73.9 kB -- 23.3 kB
Box -- 69.2 kB -- 20.9 kB
Breadcrumbs -- 66.4 kB -- 20.8 kB
Button -- 77.8 kB -- 23.8 kB
ButtonBase -- 72.3 kB -- 22.6 kB
ButtonGroup -- 80.4 kB -- 24.7 kB
Card -- 61.2 kB -- 19.1 kB
CardActionArea -- 73.4 kB -- 23.2 kB
CardActions -- 60.5 kB -- 18.9 kB
CardContent -- 60.4 kB -- 18.9 kB
CardHeader -- 63.5 kB -- 20 kB
CardMedia -- 60.8 kB -- 19.1 kB
Chip -- 80.9 kB -- 24.7 kB
CircularProgress -- 62.5 kB -- 19.7 kB
ClickAwayListener -- 3.87 kB -- 1.56 kB
Collapse -- 66.3 kB -- 20.5 kB
colorManipulator -- 3.83 kB -- 1.52 kB
Container -- 61.6 kB -- 19.2 kB
CssBaseline -- 56 kB -- 17.5 kB
Dialog -- 80.9 kB -- 25.1 kB
DialogActions -- 60.5 kB -- 18.9 kB
DialogContent -- 60.6 kB -- 19 kB
DialogContentText -- 62.5 kB -- 19.6 kB
DialogTitle -- 62.7 kB -- 19.7 kB
Divider -- 61 kB -- 19.2 kB
docs.landing -- 55.8 kB -- 14.3 kB
docs.main -- 606 kB -- 193 kB
Drawer -- 82.7 kB -- 25.6 kB
ExpansionPanel -- 69.6 kB -- 21.7 kB
ExpansionPanelActions -- 60.5 kB -- 19 kB
ExpansionPanelDetails -- 60.4 kB -- 18.9 kB
ExpansionPanelSummary -- 76.5 kB -- 24.1 kB
Fab -- 75.2 kB -- 23.3 kB
Fade -- 22 kB -- 7.6 kB
FilledInput -- 72 kB -- 22.3 kB
FormControl -- 62.8 kB -- 19.5 kB
FormControlLabel -- 63.9 kB -- 20.1 kB
FormGroup -- 60.4 kB -- 18.9 kB
FormHelperText -- 61.7 kB -- 19.3 kB
FormLabel -- 61.9 kB -- 19.1 kB
Grid -- 63.5 kB -- 19.9 kB
GridList -- 60.9 kB -- 19.1 kB
GridListTile -- 62.2 kB -- 19.5 kB
GridListTileBar -- 61.6 kB -- 19.3 kB
Grow -- 22.6 kB -- 7.72 kB
Hidden -- 64.5 kB -- 20.2 kB
Icon -- 61.2 kB -- 19.2 kB
IconButton -- 74.5 kB -- 23.2 kB
Input -- 70.9 kB -- 22.1 kB
InputAdornment -- 63.5 kB -- 20 kB
InputBase -- 69 kB -- 21.6 kB
InputLabel -- 63.7 kB -- 19.8 kB
LinearProgress -- 63.8 kB -- 19.9 kB
Link -- 65 kB -- 20.6 kB
List -- 60.8 kB -- 18.9 kB
ListItem -- 75.5 kB -- 23.5 kB
ListItemAvatar -- 60.5 kB -- 18.9 kB
ListItemIcon -- 60.6 kB -- 19 kB
ListItemSecondaryAction -- 60.4 kB -- 18.9 kB
ListItemText -- 63.4 kB -- 19.9 kB
ListSubheader -- 61.2 kB -- 19.2 kB
Menu -- 86.6 kB -- 27.2 kB
MenuItem -- 76.5 kB -- 23.8 kB
MenuList -- 64.4 kB -- 20.1 kB
MobileStepper -- 66.2 kB -- 20.6 kB
Modal -- 14.2 kB -- 4.96 kB
NativeSelect -- 75.2 kB -- 23.7 kB
NoSsr -- 2.19 kB -- 1.04 kB
OutlinedInput -- 72.5 kB -- 22.5 kB
Paper -- 60.7 kB -- 18.9 kB
Popover -- 81 kB -- 25 kB
Popper -- 28.5 kB -- 10.2 kB
Portal -- 2.87 kB -- 1.3 kB
RadioGroup -- 61.7 kB -- 19.3 kB
Rating -- 68.4 kB -- 21.9 kB
RootRef -- 4.43 kB -- 1.67 kB
Select -- 112 kB -- 33.4 kB
Skeleton -- 60.9 kB -- 19.1 kB
Slide -- 24.1 kB -- 8.21 kB
Slider -- 74 kB -- 23.3 kB
Snackbar -- 75.6 kB -- 23.5 kB
SnackbarContent -- 64.1 kB -- 20.1 kB
SpeedDial -- 84.4 kB -- 26.6 kB
SpeedDialAction -- 115 kB -- 36.3 kB
SpeedDialIcon -- 63 kB -- 19.8 kB
Step -- 61 kB -- 19.1 kB
StepButton -- 80.7 kB -- 25.3 kB
StepConnector -- 61.1 kB -- 19.2 kB
StepContent -- 67.4 kB -- 21 kB
StepIcon -- 63.1 kB -- 19.6 kB
StepLabel -- 67 kB -- 21 kB
Stepper -- 63.2 kB -- 19.9 kB
styles/createMuiTheme -- 15.2 kB -- 5.36 kB
SvgIcon -- 61.5 kB -- 19.1 kB
SwipeableDrawer -- 90.1 kB -- 28 kB
Tab -- 74.7 kB -- 23.7 kB
Table -- 61 kB -- 19.1 kB
TableBody -- 60.5 kB -- 18.9 kB
TableCell -- 62.5 kB -- 19.6 kB
TableFooter -- 60.5 kB -- 18.9 kB
TableHead -- 60.5 kB -- 18.9 kB
TablePagination -- 139 kB -- 40.5 kB
TableRow -- 61 kB -- 19.1 kB
TableSortLabel -- 75.7 kB -- 23.9 kB
Tabs -- 83.8 kB -- 26.7 kB
TextareaAutosize -- 5.06 kB -- 2.11 kB
TextField -- 121 kB -- 35.4 kB
ToggleButton -- 74.5 kB -- 23.5 kB
ToggleButtonGroup -- 61.6 kB -- 19.4 kB
Toolbar -- 60.8 kB -- 19 kB
Tooltip -- 98.9 kB -- 31.3 kB
TreeItem -- 72 kB -- 22.7 kB
TreeView -- 64.8 kB -- 20.3 kB
Typography -- 62.1 kB -- 19.3 kB
useAutocomplete -- 12.1 kB -- 4.47 kB
useMediaQuery -- 2.49 kB -- 1.05 kB
Zoom -- 22.1 kB -- 7.6 kB

Generated by 🚫 dangerJS against 7f846b2

@oliviertassinari
Copy link
Member

@rbrishabh Could we look for a solution that doesn't involve a new API?

@rbrishabh
Copy link
Contributor Author

Okay sure. I will look into it!

@oliviertassinari
Copy link
Member

Thanks

@oliviertassinari oliviertassinari changed the title Fix - adding noFocus option for form-label [SwitchBase] Ignore focus and blur events Nov 13, 2019
@oliviertassinari oliviertassinari added component: checkbox This is the name of the generic UI component, not the React module! component: radio This is the name of the generic UI component, not the React module! component: switch This is the name of the generic UI component, not the React module! labels Nov 13, 2019
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.

I have proposed a different approach in the last commit. Let me know what you think about it.

Copy link
Member

@eps1lon eps1lon left a comment

Choose a reason for hiding this comment

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

Why is this marked as ready to ship? I offered an alternative explanation of the issue in #18210 (comment). Please address this (is the explanation not correct?) before merging this PR

@oliviertassinari
Copy link
Member

oliviertassinari commented Nov 14, 2019

The label was meant to trigger this potential concern :). I thought #18210 (comment) was not directly related to this pull request as the behavior can be reproduced in codesandbox. What do you recommend here?

@eps1lon
Copy link
Member

eps1lon commented Nov 15, 2019

I thought #18210 (comment) was not directly related to this pull request as the behavior can be reproduced in codesandbox.

I couldn't find a codesandbox. I guess you mean the demo in a codesandbox.

This issue is fundamental to anything that moves focus on mouseup. This approach is not correct (obvious from the test that needs to be removed).

I don't think we should change something in the SwitchBase. The focus styling on the FormControl is there to, well, signal focus-within. If I mousedown then focus is removed which means the FormControl should no longer display that it has focus-within.

If we actually want to prevent blur on mousedown then the platform has a built-in solution with preventDefault. This might be better idea (which assumes users rarely "interrupt" a label click by moving away from the label after mousedown).

https://codesandbox.io/s/formlabel-preventdefault-6rbwl

@oliviertassinari
Copy link
Member

oliviertassinari commented Nov 15, 2019

Yes, you are exact. I meant that we can open the docs demo in codesandbox and reproduce the issue.

The focus styling on the FormControl is there to, well, signal focus-within.

I think that we should challenge the existence of this signal in the first place. I have been keen, for a long time, to explore if we shouldn't remove it, this is a good opportunity to explore it. I doubt that the form label should signal should be present because it's not a common pattern. It feels like a distraction.

@eps1lon
Copy link
Member

eps1lon commented Nov 15, 2019

I have been keen, for a long time, to explore if we shouldn't remove it, this is a good opportunity to explore it.

We can schedule this for v5, sure. It's not a good oppurtinity to explore this now. And then it would require, as you said, exploring this first: Is a :focus-within polyfill needed in core? What do the guidelines say about form groups etc.

@oliviertassinari
Copy link
Member

I could find a design system that handles the focus state:

Capture d’écran 2019-11-15 à 16 18 22
Capture d’écran 2019-11-15 à 15 22 54
https://elastic.github.io/eui/#/utilities/focus-trap

Ok, let's move this to v5

@oliviertassinari
Copy link
Member

oliviertassinari commented Nov 15, 2019

@rbrishabh Thanks for raising, we would recommend customizing the focus and not focused state at the same time.

@rbrishabh
Copy link
Contributor Author

@oliviertassinari @eps1lon Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: checkbox This is the name of the generic UI component, not the React module! component: radio This is the name of the generic UI component, not the React module! component: switch This is the name of the generic UI component, not the React module!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Form labels for check boxes might be misbehaving
4 participants