Skip to content

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

Merged
oliviertassinari merged 3 commits intomui:masterfrom
captain-yossarian:issue/19783
Feb 20, 2020
Merged

[ButtonBase] Fix space handling for non native button elements#19784
oliviertassinari merged 3 commits intomui:masterfrom
captain-yossarian:issue/19783

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

captain-yossarian commented Feb 19, 2020

@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
eps1lon reviewed Feb 19, 2020
View reviewed changes
@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
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 type: bug It doesn't behave as expected. label Feb 20, 2020
@oliviertassinari oliviertassinari merged commit 56db981 into mui:master Feb 20, 2020
@oliviertassinari
Copy link
Member

oliviertassinari commented Feb 20, 2020

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
@michaldudak michaldudak mentioned this pull request Aug 5, 2021
Merged
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
@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

component: ButtonBase The React component. type: bug It doesn't behave as expected.

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