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

[material-ui] Remove remaining IE11 code #42283

Merged
merged 10 commits into from
Jun 3, 2024
Merged
14 changes: 1 addition & 13 deletions packages/mui-material/src/ButtonBase/ButtonBase.js
Original file line number Diff line number Diff line change
Expand Up @@ -219,20 +219,9 @@ const ButtonBase = React.forwardRef(function ButtonBase(inProps, ref) {
return component && component !== 'button' && !(button.tagName === 'A' && button.href);
};

/**
* IE11 shim for https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent/repeat
*/
const keydownRef = React.useRef(false);
const handleKeyDown = useEventCallback((event) => {
// Check if key is already down to avoid repeats being counted as multiple activations
if (
focusRipple &&
!keydownRef.current &&
focusVisible &&
rippleRef.current &&
event.key === ' '
) {
keydownRef.current = true;
if (focusRipple && !event.repeat && focusVisible && rippleRef.current && event.key === ' ') {
Copy link
Member Author

Choose a reason for hiding this comment

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

Refactored with repeat: https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent/repeat.

Screen.Recording.2024-05-22.at.12.02.45.mov

rippleRef.current.stop(event, () => {
rippleRef.current.start(event);
});
Expand Down Expand Up @@ -270,7 +259,6 @@ const ButtonBase = React.forwardRef(function ButtonBase(inProps, ref) {
focusVisible &&
!event.defaultPrevented
) {
keydownRef.current = false;
rippleRef.current.stop(event, () => {
rippleRef.current.pulsate(event);
});
Expand Down
4 changes: 1 addition & 3 deletions packages/mui-material/src/ButtonBase/ButtonBase.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -901,9 +901,7 @@ describe('<ButtonBase />', () => {

expect(container.querySelectorAll('.ripple-visible')).to.have.lengthOf(1);

// technically the second keydown should be fire with repeat: true
// but that isn't implemented in IE11 so we shouldn't mock it here either
fireEvent.keyDown(button, { key: 'Enter' });
fireEvent.keyDown(button, { key: 'Enter', repeat: true });
Copy link
Member Author

Choose a reason for hiding this comment

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

Adjust according to above's (ButtonBase) refactor


expect(container.querySelectorAll('.ripple-visible')).to.have.lengthOf(1);
});
Expand Down
2 changes: 0 additions & 2 deletions packages/mui-material/src/Dialog/Dialog.js
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,6 @@ const DialogPaper = styled(Paper, {
})(({ theme }) => ({
margin: 32,
position: 'relative',
overflowY: 'auto', // Fix IE11 issue, to remove at some point.
Copy link
Member Author

Choose a reason for hiding this comment

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

Introduced in #8877 to fix #8832.

Tested that it's working in Chrome, Firefox and Safari. Video from Chrome:

Screen.Recording.2024-05-22.at.14.25.12.mov

'@media print': {
overflowY: 'visible',
boxShadow: 'none',
Expand All @@ -140,7 +139,6 @@ const DialogPaper = styled(Paper, {
style: {
display: 'inline-block',
verticalAlign: 'middle',
textAlign: 'left', // 'initial' doesn't work on IE11
Copy link
Member Author

Choose a reason for hiding this comment

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

Introduced in: https://github.com/mui/material-ui/pull/15896/files#diff-ceb6f0d9cdbe18a334a4162f10048041b9b83290105a22ffd611608d94b81960R71

ested that it's working in Chrome, Firefox, and Safari. Video from Chrome:

Screen.Recording.2024-05-22.at.14.33.05.mov

Copy link
Member

Choose a reason for hiding this comment

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

I'm probably missing something, but the text in your recording isn't left alingned?

Copy link
Member Author

Choose a reason for hiding this comment

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

You're right 😅 I probably just saw a big block of text and that it scrolled as expected and missed the alignment. Fixed in #42778.

Thanks for the catch.

},
},
{
Expand Down
2 changes: 0 additions & 2 deletions packages/mui-material/src/FilledInput/FilledInput.js
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,6 @@ const FilledInputRoot = styled(InputBaseRoot, {
'&::after': {
left: 0,
bottom: 0,
// Doing the other way around crash on IE11 "''" https://github.com/cssinjs/jss/issues/242
Copy link
Member Author

Choose a reason for hiding this comment

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

There are a bunch of these comments, which I think got copied and pasted from the original issue: cssinjs/jss#242 (comment)

We have other content: '""' occurrences in the codebase without the comment. I think it's not bad to have content: '""', so I just removed the comment.

content: '""',
position: 'absolute',
right: 0,
Expand Down Expand Up @@ -108,7 +107,6 @@ const FilledInputRoot = styled(InputBaseRoot, {
}`,
left: 0,
bottom: 0,
// Doing the other way around crash on IE11 "''" https://github.com/cssinjs/jss/issues/242
content: '"\\00a0"',
position: 'absolute',
right: 0,
Expand Down
1 change: 0 additions & 1 deletion packages/mui-material/src/IconButton/IconButton.js
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,6 @@ const IconButtonRoot = styled(ButtonBase, {
fontSize: theme.typography.pxToRem(24),
padding: 8,
borderRadius: '50%',
overflow: 'visible', // Explicitly set the default value to solve a bug on IE11.
Copy link
Member Author

Choose a reason for hiding this comment

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

Introduced in #12743

Tested that it's working in Chrome, Firefox and Safari. Screenshot from Chrome:

Screenshot 2024-05-22 at 14 41 02

color: (theme.vars || theme).palette.action.active,
transition: theme.transitions.create('background-color', {
duration: theme.transitions.duration.shortest,
Expand Down
2 changes: 0 additions & 2 deletions packages/mui-material/src/Input/Input.js
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,6 @@ const InputRoot = styled(InputBaseRoot, {
'&::after': {
left: 0,
bottom: 0,
// Doing the other way around crash on IE11 "''" https://github.com/cssinjs/jss/issues/242
content: '""',
position: 'absolute',
right: 0,
Expand All @@ -93,7 +92,6 @@ const InputRoot = styled(InputBaseRoot, {
borderBottom: `1px solid ${bottomLineColor}`,
left: 0,
bottom: 0,
// Doing the other way around crash on IE11 "''" https://github.com/cssinjs/jss/issues/242
content: '"\\00a0"',
position: 'absolute',
right: 0,
Expand Down
1 change: 0 additions & 1 deletion packages/mui-material/src/InputAdornment/InputAdornment.js
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,6 @@ const InputAdornmentRoot = styled('div', {
overridesResolver,
})(({ theme }) => ({
display: 'flex',
height: '0.01em', // Fix IE11 flexbox alignment. To remove at some point.
Copy link
Member Author

Choose a reason for hiding this comment

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

Introduced in #12975

Tested it in Chrome, Firefox, and Safari. Screenshot in Chrome:

Screenshot 2024-05-23 at 16 16 02

maxHeight: '2em',
alignItems: 'center',
whiteSpace: 'nowrap',
Expand Down
12 changes: 1 addition & 11 deletions packages/mui-material/src/InputBase/InputBase.js
Original file line number Diff line number Diff line change
Expand Up @@ -184,12 +184,11 @@ export const InputBaseInput = styled('input', {
display: 'block',
// Make the flex item shrink with Firefox
minWidth: 0,
width: '100%', // Fix IE11 width issue
Copy link
Member Author

Choose a reason for hiding this comment

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

flexGrow: 1,
animationName: 'mui-auto-fill-cancel',
animationDuration: '10ms',
'&::-webkit-input-placeholder': placeholder,
'&::-moz-placeholder': placeholder, // Firefox 19+
'&:-ms-input-placeholder': placeholder, // IE11
Copy link
Member Author

Choose a reason for hiding this comment

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

This and the other '&:-ms-input-placeholder' were added in the original InputBase: https://github.com/mui/material-ui/pull/12076/files#diff-a5f495d99c59823944e8c99682e25d509e79d427ab0442c71589da33e6f350f6R81

Tested it in Chrome, Firefox, and Safari. Screenshot in Chrome:

Screen.Recording.2024-05-23.at.16.33.57.mov

'&::-ms-input-placeholder': placeholder, // Edge
'&:focus': {
outline: 0,
Expand All @@ -206,11 +205,9 @@ export const InputBaseInput = styled('input', {
[`label[data-shrink=false] + .${inputBaseClasses.formControl} &`]: {
'&::-webkit-input-placeholder': placeholderHidden,
'&::-moz-placeholder': placeholderHidden, // Firefox 19+
'&:-ms-input-placeholder': placeholderHidden, // IE11
'&::-ms-input-placeholder': placeholderHidden, // Edge
'&:focus::-webkit-input-placeholder': placeholderVisible,
'&:focus::-moz-placeholder': placeholderVisible, // Firefox 19+
'&:focus:-ms-input-placeholder': placeholderVisible, // IE11
'&:focus::-ms-input-placeholder': placeholderVisible, // Edge
},
[`&.${inputBaseClasses.disabled}`]: {
Expand Down Expand Up @@ -390,13 +387,6 @@ const InputBase = React.forwardRef(function InputBase(inProps, ref) {
}, [value, checkDirty, isControlled]);

const handleFocus = (event) => {
// Fix a bug with IE11 where the focus/blur events are triggered
Copy link
Member Author

Choose a reason for hiding this comment

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

Introduced in https://github.com/mui/material-ui/pull/12076/files#diff-a5f495d99c59823944e8c99682e25d509e79d427ab0442c71589da33e6f350f6R225

Tested it in Chrome, Firefox, and Safari. Screenshot in Chrome:

Screen.Recording.2024-05-23.at.16.39.35.mov

// while the component is disabled.
if (fcs.disabled) {
event.stopPropagation();
return;
}

if (onFocus) {
onFocus(event);
}
Expand Down
21 changes: 0 additions & 21 deletions packages/mui-material/src/InputBase/InputBase.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -110,27 +110,6 @@ describe('<InputBase />', () => {
expect(handleFocus.callCount).to.equal(1);
});

// IE11 bug
Copy link
Member Author

Choose a reason for hiding this comment

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

Removed IE11 related test

it('should not respond the focus event when disabled', () => {
const handleFocus = spy();
// non-native input simulating how IE11 treats disabled inputs
const { getByRole } = render(
<div onFocus={handleFocus}>
<InputBase
disabled
inputComponent="div"
inputProps={{ role: 'textbox', tabIndex: -1 }}
onFocus={handleFocus}
/>
</div>,
);

act(() => {
getByRole('textbox').focus();
});
expect(handleFocus.called).to.equal(false);
});

it('fires the click event when the <input /> is disabled', () => {
const handleClick = spy();
const { getByRole } = render(<InputBase disabled onClick={handleClick} />);
Expand Down
4 changes: 0 additions & 4 deletions packages/mui-material/src/NativeSelect/NativeSelectInput.js
Original file line number Diff line number Diff line change
Expand Up @@ -35,10 +35,6 @@ export const StyledSelectSelect = styled('select')(({ theme }) => ({
// Reset Chrome style
borderRadius: 0,
},
// Remove IE11 arrow
Copy link
Member Author

Choose a reason for hiding this comment

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

Couldn't trace it further than https://github.com/mui/material-ui/pull/24698/files#diff-eca42ced51d2f5fc85b4932a8d16e28b815c6f193514d1f4101949e879970b12R53

Looks good in Chrome, Firefox, and Safari. Screenshot in Chrome:

Screenshot 2024-05-23 at 16 45 49

Copy link
Member

Choose a reason for hiding this comment

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

'&::-ms-expand': {
display: 'none',
},
[`&.${nativeSelectClasses.disabled}`]: {
cursor: 'default',
},
Expand Down
1 change: 0 additions & 1 deletion packages/mui-material/src/StepLabel/StepLabel.js
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,6 @@ const StepLabelIconContainer = styled('span', {
slot: 'IconContainer',
overridesResolver: (props, styles) => styles.iconContainer,
})({
flexShrink: 0, // Fix IE11 issue
Copy link
Member Author

Choose a reason for hiding this comment

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

Introduced in #13375

Tested in Chrome, Firefox, and Safari. Screenshot in Chrome:

Screenshot 2024-05-23 at 16 47 21

display: 'flex',
paddingRight: 8,
[`&.${stepLabelClasses.alternativeLabel}`]: {
Expand Down
17 changes: 6 additions & 11 deletions packages/mui-material/src/Tabs/Tabs.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ import { useRtl } from '@mui/system/RtlProvider';
import { styled, createUseThemeProps } from '../zero-styled';
import useTheme from '../styles/useTheme';
import debounce from '../utils/debounce';
import { getNormalizedScrollLeft, detectScrollType } from '../utils/scrollLeft';
Copy link
Member Author

Choose a reason for hiding this comment

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

These are no longer needed as negative has been standardized on all supported browsers: https://developer.mozilla.org/en-US/docs/Web/API/Element/scrollLeft#browser_compatibility

Tested in Chrome, Safari, Firefox, and Edge. Recording from Edge:

Screen.Recording.2024-05-28.at.16.02.54.mov

Copy link
Member

Choose a reason for hiding this comment

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

Oh wow, they did, it has never been either to build great components, fewer weird browser bugs to fix 😄.

import animate from '../internal/animate';
import ScrollbarSize from './ScrollbarSize';
import TabScrollButton from '../TabScrollButton';
Expand Down Expand Up @@ -380,7 +379,6 @@ const Tabs = React.forwardRef(function Tabs(inProps, ref) {
clientWidth: tabsNode.clientWidth,
scrollLeft: tabsNode.scrollLeft,
scrollTop: tabsNode.scrollTop,
scrollLeftNormalized: getNormalizedScrollLeft(tabsNode, isRtl ? 'rtl' : 'ltr'),
scrollWidth: tabsNode.scrollWidth,
top: rect.top,
bottom: rect.bottom,
Expand Down Expand Up @@ -452,11 +450,9 @@ const Tabs = React.forwardRef(function Tabs(inProps, ref) {
} else {
startIndicator = isRtl ? 'right' : 'left';
if (tabMeta && tabsMeta) {
const correction = isRtl
? tabsMeta.scrollLeftNormalized + tabsMeta.clientWidth - tabsMeta.scrollWidth
: tabsMeta.scrollLeft;
startValue =
(isRtl ? -1 : 1) * (tabMeta[startIndicator] - tabsMeta[startIndicator] + correction);
(isRtl ? -1 : 1) *
(tabMeta[startIndicator] - tabsMeta[startIndicator] + tabsMeta.scrollLeft);
}
}

Expand All @@ -466,9 +462,10 @@ const Tabs = React.forwardRef(function Tabs(inProps, ref) {
[size]: tabMeta ? tabMeta[size] : 0,
};

// IE11 support, replace with Number.isNaN
// eslint-disable-next-line no-restricted-globals
if (isNaN(indicatorStyle[startIndicator]) || isNaN(indicatorStyle[size])) {
if (
typeof indicatorStyle[startIndicator] !== 'number' ||
typeof indicatorStyle[size] !== 'number'
) {
setIndicatorStyle(newIndicatorStyle);
} else {
const dStart = Math.abs(indicatorStyle[startIndicator] - newIndicatorStyle[startIndicator]);
Expand Down Expand Up @@ -497,8 +494,6 @@ const Tabs = React.forwardRef(function Tabs(inProps, ref) {
scrollValue += delta;
} else {
scrollValue += delta * (isRtl ? -1 : 1);
// Fix for Edge
scrollValue *= isRtl && detectScrollType() === 'reverse' ? -1 : 1;
}

scroll(scrollValue);
Expand Down
4 changes: 0 additions & 4 deletions packages/mui-material/src/utils/scrollLeft.js
Copy link
Member Author

Choose a reason for hiding this comment

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

This reexport was only used in Tabs, and is no longer needed.

I'll handle the utils package in a separate PR.

Copy link
Member

@ZeeshanTamboli ZeeshanTamboli May 29, 2024

Choose a reason for hiding this comment

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

If this utility was only used in Material UI Tabs, why not remove it in this PR from the mui-utils package? Also, does this complete the third task in #40958?

Copy link
Member Author

@DiegoAndai DiegoAndai May 30, 2024

Choose a reason for hiding this comment

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

I would like to handle the mui-utils in a separate PR to have smaller PRs, making it easier to revert any issues on a package-per-package basis.

There is more IE11-related code in the utils; I don't want this PR to grow larger.

This file was deleted.