-
-
Notifications
You must be signed in to change notification settings - Fork 32.4k
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
Conversation
Netlify deploy previewhttps://deploy-preview-42283--material-ui.netlify.app/ @material-ui/core: parsed: -0.22% 😍, gzip: -0.29% 😍 Bundle size reportDetails of bundle changes (Toolpad) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Explanation on some of the changes (some missing)
event.key === ' ' | ||
) { | ||
keydownRef.current = true; | ||
if (focusRipple && !event.repeat && focusVisible && rippleRef.current && event.key === ' ') { |
There was a problem hiding this comment.
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
// 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 }); |
There was a problem hiding this comment.
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
@@ -117,7 +117,6 @@ const DialogPaper = styled(Paper, { | |||
})(({ theme }) => ({ | |||
margin: 32, | |||
position: 'relative', | |||
overflowY: 'auto', // Fix IE11 issue, to remove at some point. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -140,7 +139,6 @@ const DialogPaper = styled(Paper, { | |||
style: { | |||
display: 'inline-block', | |||
verticalAlign: 'middle', | |||
textAlign: 'left', // 'initial' doesn't work on IE11 |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
@@ -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 |
There was a problem hiding this comment.
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.
// IE11 support, replace with Number.isNaN | ||
// eslint-disable-next-line no-restricted-globals | ||
if (isNaN(indicatorStyle[startIndicator]) || isNaN(indicatorStyle[size])) { | ||
if (Number.isNaN(+indicatorStyle[startIndicator]) || Number.isNaN(+indicatorStyle[size])) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually we don't need to check for NaN
global type since it will be never be NaN
as per the logic. See discussion in #41709 especially #41709 (comment) and #41709 (comment). We can use a typeof
number check safely.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for bringing this up, updated!
@@ -110,27 +110,6 @@ describe('<InputBase />', () => { | |||
expect(handleFocus.callCount).to.equal(1); | |||
}); | |||
|
|||
// IE11 bug |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed IE11 related test
@@ -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. |
There was a problem hiding this comment.
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:
@@ -45,7 +45,6 @@ const InputAdornmentRoot = styled('div', { | |||
overridesResolver, | |||
})(({ theme }) => ({ | |||
display: 'flex', | |||
height: '0.01em', // Fix IE11 flexbox alignment. To remove at some point. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
animationName: 'mui-auto-fill-cancel', | ||
animationDuration: '10ms', | ||
'&::-webkit-input-placeholder': placeholder, | ||
'&::-moz-placeholder': placeholder, // Firefox 19+ | ||
'&:-ms-input-placeholder': placeholder, // IE11 |
There was a problem hiding this comment.
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
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested it in Chrome, Firefox, and Safari. Screenshot in Chrome:
Screen.Recording.2024-05-23.at.16.39.35.mov
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -35,10 +35,6 @@ export const StyledSelectSelect = styled('select')(({ theme }) => ({ | |||
// Reset Chrome style | |||
borderRadius: 0, | |||
}, | |||
// Remove IE11 arrow |
There was a problem hiding this comment.
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:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -104,7 +104,6 @@ const StepLabelIconContainer = styled('span', { | |||
slot: 'IconContainer', | |||
overridesResolver: (props, styles) => styles.iconContainer, | |||
})({ | |||
flexShrink: 0, // Fix IE11 issue |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On hold: #42358 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not yet, because there are still some occurrences in other packages and the docs, as you pointed out. I want to handle those separately. This PR only covers Material UI. |
@@ -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'; |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 😄.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
@@ -13,11 +13,6 @@ export default function ServerModal() { | |||
flexGrow: 1, | |||
minWidth: 300, | |||
transform: 'translateZ(0)', | |||
// The position fixed scoping doesn't work in IE11. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Introduced in #17972. Rolling back to original.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about removing this warning? We no longer want to suggest using Material UI version 4.
@ZeeshanTamboli Good catch. I agree; we officially do not support IE, so there is no need for the warning. Implemented. |
Don't forget all the uses of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me! 👍
@mbrookes, thanks for the callout. Do you remember an example in particular so we can create a separate issue for it? |
Sorry, I meant Array.includes, which IE 11 doesn't support. These should find the Material-UI specific ones but there are plenty more: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, thanks for simplifying the codebase 👍
@@ -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'; |
There was a problem hiding this comment.
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 😄.
@@ -35,10 +35,6 @@ export const StyledSelectSelect = styled('select')(({ theme }) => ({ | |||
// Reset Chrome style | |||
borderRadius: 0, | |||
}, | |||
// Remove IE11 arrow |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Material UI portion of #14420
Remove the remaining IE11-related code from the Material UI package.