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.

Sign up for GitHub

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

Jump to bottom

[ButtonBase] Fix space handling for non native button elements #19784

Merged
merged 3 commits into from
Feb 20, 2020
Merged

[ButtonBase] Fix space handling for non native button elements #19784

merged 3 commits into from
Feb 20, 2020

Conversation

captain-yossarian
Copy link
Contributor

@captain-yossarian captain-yossarian commented Feb 19, 2020 •
edited by oliviertassinari
Loading

The bug can be reproduced on https://codesandbox.io/s/lively-morning-0nbh5. Compare the different behavior between the native button and the button base.

@captain-yossarian
Copy link
Contributor Author

@oliviertassinari
This PR should be merged after #19724
Then I will test this changes again.

P.S. I've tested this changes in context of #19724 and it worked.

@mui-pr-bot
Copy link

mui-pr-bot commented Feb 19, 2020 •
edited
Loading

Details of bundle changes.

Comparing: 08e7bf5...4e175e6

bundle Size Change Size Gzip Change Gzip
Alert ▲ +45 B (+0.05% ) 83.8 kB ▲ +14 B (+0.05% ) 26.4 kB
docs.main ▲ +45 B (+0.01% ) 604 kB ▲ +14 B (+0.01% ) 196 kB
Checkbox ▲ +45 B (+0.05% ) 83.4 kB ▲ +13 B (+0.05% ) 26.3 kB
ButtonGroup ▲ +45 B (+0.05% ) 83.6 kB ▲ +12 B (+0.05% ) 25.6 kB
Chip ▲ +45 B (+0.05% ) 83 kB ▲ +11 B (+0.04% ) 25.4 kB
Fab ▲ +45 B (+0.06% ) 77.2 kB ▲ +11 B (+0.05% ) 24.1 kB
IconButton ▲ +45 B (+0.06% ) 76.6 kB ▲ +11 B (+0.05% ) 23.9 kB
Pagination ▲ +45 B (+0.05% ) 85.6 kB ▲ +11 B (+0.04% ) 26.3 kB
@material-ui/lab ▲ +45 B (+0.02% ) 194 kB ▲ +10 B (+0.02% ) 57.2 kB
Autocomplete ▲ +45 B (+0.03% ) 132 kB ▲ +10 B (+0.02% ) 41.4 kB
BottomNavigationAction ▲ +45 B (+0.06% ) 75.9 kB ▲ +10 B (+0.04% ) 24 kB
Button ▲ +45 B (+0.06% ) 80.1 kB ▲ +10 B (+0.04% ) 24.5 kB
CardActionArea ▲ +45 B (+0.06% ) 75.5 kB ▲ +10 B (+0.04% ) 23.8 kB
ListItem ▲ +45 B (+0.06% ) 77.6 kB ▲ +10 B (+0.04% ) 24.2 kB
MenuItem ▲ +45 B (+0.06% ) 78.6 kB ▲ +10 B (+0.04% ) 24.5 kB
PaginationItem ▲ +45 B (+0.06% ) 81.2 kB ▲ +10 B (+0.04% ) 25 kB
Radio ▲ +45 B (+0.05% ) 84.4 kB ▲ +10 B (+0.04% ) 26.6 kB
SpeedDialAction ▲ +45 B (+0.04% ) 119 kB ▲ +10 B (+0.03% ) 37.6 kB
Switch ▲ +45 B (+0.05% ) 82.6 kB ▲ +10 B (+0.04% ) 26 kB
Tab ▲ +45 B (+0.06% ) 76.7 kB ▲ +10 B (+0.04% ) 24.3 kB
TablePagination ▲ +45 B (+0.03% ) 144 kB ▲ +10 B (+0.02% ) 42 kB
TableSortLabel ▲ +45 B (+0.06% ) 77.8 kB ▲ +10 B (+0.04% ) 24.4 kB
Tabs ▲ +45 B (+0.05% ) 85.8 kB ▲ +10 B (+0.04% ) 27.2 kB
ToggleButton ▲ +45 B (+0.06% ) 76.6 kB ▲ +10 B (+0.04% ) 24.2 kB
@material-ui/core ▲ +45 B (+0.01% ) 362 kB ▲ +9 B (+0.01% ) 98.9 kB
@material-ui/core[umd] ▲ +45 B (+0.01% ) 318 kB ▲ +9 B (+0.01% ) 92 kB
ButtonBase ▲ +45 B (+0.06% ) 74.4 kB ▲ +9 B (+0.04% ) 23.3 kB
ExpansionPanelSummary ▲ +45 B (+0.06% ) 78.5 kB ▲ +9 B (+0.04% ) 24.8 kB
SpeedDial ▲ +45 B (+0.05% ) 86.7 kB ▲ +9 B (+0.03% ) 27.3 kB
StepButton ▲ +45 B (+0.05% ) 82.8 kB ▲ +9 B (+0.03% ) 26.2 kB
@material-ui/styles -- 51.4 kB -- 15.4 kB
@material-ui/system -- 16.5 kB -- 4.32 kB
AlertTitle -- 64.5 kB -- 20.3 kB
AppBar -- 64.4 kB -- 20.2 kB
Avatar -- 65.6 kB -- 20.7 kB
AvatarGroup -- 62.6 kB -- 19.7 kB
Backdrop -- 68.2 kB -- 21.1 kB
Badge -- 65.7 kB -- 20.4 kB
BottomNavigation -- 62.7 kB -- 19.7 kB
Box -- 72.2 kB -- 21.9 kB
Breadcrumbs -- 68.1 kB -- 21.4 kB
Card -- 63.2 kB -- 19.8 kB
CardActions -- 62.4 kB -- 19.6 kB
CardContent -- 62.3 kB -- 19.5 kB
CardHeader -- 65.4 kB -- 20.6 kB
CardMedia -- 62.7 kB -- 19.7 kB
CircularProgress -- 64.4 kB -- 20.3 kB
ClickAwayListener -- 3.91 kB -- 1.55 kB
Collapse -- 68.4 kB -- 21.2 kB
colorManipulator -- 3.88 kB -- 1.52 kB
Container -- 63.5 kB -- 19.9 kB
CssBaseline -- 62.3 kB -- 19.6 kB
Dialog -- 83.4 kB -- 26 kB
DialogActions -- 62.4 kB -- 19.6 kB
DialogContent -- 62.6 kB -- 19.6 kB
DialogContentText -- 64.4 kB -- 20.2 kB
DialogTitle -- 64.6 kB -- 20.3 kB
Divider -- 63 kB -- 19.8 kB
docs.landing -- 56.8 kB -- 15.6 kB
Drawer -- 85.2 kB -- 25.9 kB
ExpansionPanel -- 72.7 kB -- 22.7 kB
ExpansionPanelActions -- 62.4 kB -- 19.6 kB
ExpansionPanelDetails -- 62.3 kB -- 19.5 kB
Fade -- 23.6 kB -- 8.01 kB
FilledInput -- 73.9 kB -- 23 kB
FormControl -- 64.8 kB -- 20.2 kB
FormControlLabel -- 65.9 kB -- 20.6 kB
FormGroup -- 62.3 kB -- 19.6 kB
FormHelperText -- 63.7 kB -- 20 kB
FormLabel -- 63.8 kB -- 19.8 kB
Grid -- 65.4 kB -- 20.5 kB
GridList -- 62.8 kB -- 19.7 kB
GridListTile -- 64.1 kB -- 20.1 kB
GridListTileBar -- 63.6 kB -- 19.9 kB
Grow -- 24.2 kB -- 8.22 kB
Hidden -- 66.3 kB -- 20.8 kB
Icon -- 63.1 kB -- 19.8 kB
Input -- 72.9 kB -- 22.8 kB
InputAdornment -- 65.4 kB -- 20.6 kB
InputBase -- 71 kB -- 22.3 kB
InputLabel -- 65.7 kB -- 20.5 kB
LinearProgress -- 65.7 kB -- 20.5 kB
Link -- 67 kB -- 21.1 kB
List -- 62.7 kB -- 19.6 kB
ListItemAvatar -- 62.5 kB -- 19.5 kB
ListItemIcon -- 62.5 kB -- 19.6 kB
ListItemSecondaryAction -- 62.3 kB -- 19.5 kB
ListItemText -- 65.3 kB -- 20.5 kB
ListSubheader -- 63.1 kB -- 19.8 kB
Menu -- 89.1 kB -- 27.4 kB
MenuList -- 66.4 kB -- 20.8 kB
MobileStepper -- 68.2 kB -- 21.4 kB
Modal -- 14.5 kB -- 5.04 kB
NativeSelect -- 77.2 kB -- 24.3 kB
NoSsr -- 2.19 kB -- 1.04 kB
OutlinedInput -- 74.9 kB -- 23.3 kB
Paper -- 62.7 kB -- 19.6 kB
Popover -- 83.5 kB -- 25.8 kB
Popper -- 28.8 kB -- 10.3 kB
Portal -- 2.92 kB -- 1.3 kB
RadioGroup -- 64.8 kB -- 20.1 kB
Rating -- 70.8 kB -- 22.8 kB
RootRef -- 4.24 kB -- 1.64 kB
ScopedCssBaseline -- 63.2 kB -- 19.9 kB
Select -- 117 kB -- 34.6 kB
Skeleton -- 63.3 kB -- 20 kB
Slide -- 25.7 kB -- 8.74 kB
Slider -- 77 kB -- 24.2 kB
Snackbar -- 75.7 kB -- 23.7 kB
SnackbarContent -- 63.9 kB -- 20.1 kB
SpeedDialIcon -- 64.9 kB -- 20.3 kB
Step -- 63 kB -- 19.8 kB
StepConnector -- 63.1 kB -- 19.9 kB
StepContent -- 69.5 kB -- 21.8 kB
StepIcon -- 65 kB -- 20.3 kB
StepLabel -- 69 kB -- 21.7 kB
Stepper -- 65.2 kB -- 20.6 kB
styles/createMuiTheme -- 16.6 kB -- 5.85 kB
SvgIcon -- 63.4 kB -- 19.8 kB
SwipeableDrawer -- 92.6 kB -- 28.9 kB
Table -- 62.9 kB -- 19.7 kB
TableBody -- 62.4 kB -- 19.5 kB
TableCell -- 64.4 kB -- 20.3 kB
TableContainer -- 62.3 kB -- 19.5 kB
TableFooter -- 62.5 kB -- 19.5 kB
TableHead -- 62.4 kB -- 19.5 kB
TableRow -- 62.8 kB -- 19.7 kB
TextareaAutosize -- 5.12 kB -- 2.14 kB
TextField -- 126 kB -- 36.6 kB
ToggleButtonGroup -- 63.5 kB -- 20 kB
Toolbar -- 62.7 kB -- 19.7 kB
Tooltip -- 103 kB -- 32.4 kB
TreeItem -- 74.3 kB -- 23.5 kB
TreeView -- 67 kB -- 21.1 kB
Typography -- 64 kB -- 20 kB
useAutocomplete -- 14.7 kB -- 5.31 kB
useMediaQuery -- 2.58 kB -- 1.06 kB
Zoom -- 23.6 kB -- 8.12 kB

Generated by 🚫 dangerJS against 4e175e6

@eps1lon
Copy link
Member

eps1lon commented Feb 19, 2020

This PR is not related to #19783 since BreadcrumbsCollapsed isn't using ButtonBase.

@oliviertassinari oliviertassinari changed the title [ButtonBase] Page scrolling down on Space press when button in focus [ButtonBase] Fix space handling for non native button elements Feb 19, 2020
@oliviertassinari oliviertassinari added the component: ButtonBase The React component. label Feb 19, 2020
oliviertassinari
oliviertassinari reviewed Feb 19, 2020
View reviewed changes
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 updated the pull request description with a codesandbox to present the issue. We can observe different handling of the space key interaction.
I have also updated the tests to make sure we assert the keydown is preventDefault.

Given that @eps1lon has more context on the logic, the outcome of the effort is on his hands now.

eps1lon
eps1lon reviewed Feb 20, 2020
View reviewed changes
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.

Small nit about the test description. The rest looks fine.

@eps1lon eps1lon added the bug 🐛 Something doesn't work label Feb 20, 2020
@oliviertassinari oliviertassinari merged commit 56db981 into mui:master Feb 20, 2020
@oliviertassinari
Copy link
Member

Awesome, one great addition to the logic of the ButtonBase :D

@oliviertassinari oliviertassinari mentioned this pull request Feb 20, 2020
Merged
1 task
This was referenced Mar 10, 2020
Merged
Closed
Open
This was referenced Mar 21, 2020
Merged
Open
Closed
EsoterikStare pushed a commit to EsoterikStare/material-ui that referenced this pull request Mar 30, 2020
@captain-yossarian @EsoterikStare
[ButtonBase] Fix space handling for non native button elements (mui#1…
63abd74 
…9784)
This was referenced Apr 16, 2020
Merged
Closed
@snyk-bot snyk-bot mentioned this pull request May 4, 2020
Closed
@michaldudak michaldudak mentioned this pull request Aug 5, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oliviertassinari oliviertassinari oliviertassinari left review comments

@eps1lon eps1lon eps1lon left review comments

Assignees
No one assigned
Labels
bug 🐛 Something doesn't work component: ButtonBase The React component.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@captain-yossarian @mui-pr-bot @eps1lon @oliviertassinari