Material-ui: [TextField] Improve z-index logic

[lcswillems]

• [x] I have searched the issues of this repository and believe that this is not a duplicate.

Summary 💡

Currently, is appended before the , but it requires to add a z-index: 1 to the label to prevent a Chrome UI bug with autocomplete (https://github.com/mui-org/material-ui/blob/master/packages/material-ui/src/InputLabel/InputLabel.js#L52-L55).

However, this z-index: 1 is hacky and source of bugs (see below). I propose to remove it and to append the after the . This also prevents Chrome bugs.

Motivation 🔦

I have several forms on my website. An example:

When a form is submitted, I overlay the form with a loader:

But, the label displays over the overlay.

I could add a z-index to my overlay, but it breaks over parts of my website. For example, in some pages, a menu can display over the overlay, but with the z-index, the overlay displays on top of the menu.

So, I would really appreciate seeing this z-index removed. z-index is handy but I think that using it is a bad practice.

[oliviertassinari]

@lcswillems I think that adding a z-index here is the best approach, I don't think that changing the label order is an option. We rely on the position of the styles. For instance:

https://github.com/mui-org/material-ui/blob/9a8ce0f091bc9e7b1ee97d314371e99bcb27d1d2/packages/material-ui/src/InputBase/InputBase.js#L108

[lcswillems]

And what about adding a negative z-index at &:-webkit-autofill?

[oliviertassinari]

@lcswillems It seems to work, awesome. Do you want to take care of it? :) I haven't spotted any issue with this diff:

diff --git a/packages/material-ui/src/InputBase/InputBase.js b/packages/material-ui/src/InputBase/InputBase.js
index fa142c669..22a3b282b 100644
--- a/packages/material-ui/src/InputBase/InputBase.js
+++ b/packages/material-ui/src/InputBase/InputBase.js
@@ -119,6 +119,7 @@ export const styles = theme => {
         opacity: 1, // Reset iOS opacity
       },
       '&:-webkit-autofill': {
+ move the comment here, oh and it's no longer yellow, it's blue now
+        zIndex: -1,
         animationDuration: '5000s',
         animationName: '$auto-fill',
       },
diff --git a/packages/material-ui/src/InputLabel/InputLabel.js b/packages/material-ui/src/InputLabel/InputLabel.js
index 6819fdade..3fa1a275c 100644
--- a/packages/material-ui/src/InputLabel/InputLabel.js
+++ b/packages/material-ui/src/InputLabel/InputLabel.js
@@ -49,11 +49,6 @@ export const styles = theme => ({
   },
   /* Styles applied to the root element if `variant="filled"`. */
   filled: {
-    // Chrome's autofill feature gives the input field a yellow background.
-    // Since the input field is behind the label in the HTML tree,
-    // the input field is drawn last and hides the label with an opaque background color.
-    // zIndex: 1 will raise the label above opaque background-colors of input.
-    zIndex: 1,
     pointerEvents: 'none',
     transform: 'translate(12px, 20px) scale(1)',
     '&$marginDense': {
@@ -68,8 +63,6 @@ export const styles = theme => ({
   },
   /* Styles applied to the root element if `variant="outlined"`. */
   outlined: {
-    // see comment above on filled.zIndex
-    zIndex: 1,
     pointerEvents: 'none',
     transform: 'translate(14px, 20px) scale(1)',
     '&$marginDense': {

[lcswillems]

Great! I am okay to do it! I will submit a PR tonight :)

[eps1lon]

@lcswillems Could you share how your spinner is styled? It seems like you want to make the rest of your UI inert while loading something. You will encounter more issues related to z-index if you don't elevate the spinner above the rest of your UI.

[oliviertassinari]

For context, I was using this logic in the past to solve this problem. Notice the position and z-index. I didn't have a problem. I agree with @eps1lon there, I suspect there is more potential to improve the overlay logic than Material-UI's text field. Soon or later, you will face a similar problem:

import React from 'react'
import PropTypes from 'prop-types'
import classNames from 'classnames'
import withStyles from '@material-ui/core/styles/withStyles'
import Fade from '@material-ui/core/Fade'
import recompose from 'modules/recompose'
import Typography from '@material-ui/core/Typography'
import withI18n from 'web/modules/components/withI18n'

const styles = theme => ({
  root: {
    position: 'relative',
    minHeight: theme.spacing.unit * 13,
  },
  overlay: {
    bottom: 0,
    left: 0,
    position: 'absolute',
    right: 0,
    top: 0,
    zIndex: 1,
  },
  content: {
    alignItems: 'center',
    backgroundColor: theme.palette.background.default,
    display: 'flex',
    height: '100%',
    justifyContent: 'center',
  },
  loading: {
    opacity: 0.5,
  },
})

const showErrorMessage = ({ query, errorMessages }) => {
  if (!query.error) {
    return null
  }

  if (!errorMessages) {
    return query.error.message
  }

  if (errorMessages[query.error.graphQLErrors[0].message]) {
    return errorMessages[query.error.graphQLErrors[0].message]
  }

  return null
}

function IntermediateState(props) {
  const { children, classes, className, emptyMessage, name, query, errorMessages, t } = props
  const display =
    !!query.error ||
    query.loading ||
    query[name] === undefined ||
    (!!query[name] && !!query[name].edges && !query[name].edges.length)

  return (
    <div className={classNames(classes.root, className)}>
      <Fade in={display} unmountOnExit>
        <div className={classes.overlay} data-e2e="intermediateState.overlay">
          <div className={classNames(classes.content, { [classes.loading]: query.loading })}>
            <Typography>
              {query.loading && query[name] === undefined ? t('common:loading') : null}
              {showErrorMessage({ query, errorMessages })}
              {!query.loading &&
              !query.error &&
              !!query[name] &&
              !!query[name].edges &&
              !query[name].edges.length
                ? emptyMessage || t('common:intermediateState.defaultEmpty')
                : null}
            </Typography>
          </div>
        </div>
      </Fade>
      {!query.error &&
        query[name] !== undefined &&
        (!query[name] || (!query[name].edges || !!query[name].edges.length)) &&
        (typeof children === 'function' ? children() : children)}
    </div>
  )
}

IntermediateState.displayName = 'IntermediateState'

IntermediateState.propTypes = {
  children: PropTypes.oneOfType([PropTypes.func, PropTypes.node]),
  classes: PropTypes.object.isRequired,
  className: PropTypes.string,
  emptyMessage: PropTypes.string,
  errorMessages: PropTypes.object,
  name: PropTypes.string.isRequired,
  query: PropTypes.object.isRequired,
  t: PropTypes.func.isRequired,
}

export default recompose.compose(
  withI18n(),
  withStyles(styles)
)(IntermediateState)

[lcswillems]

Here is a simplified version of my overlay component:

const Overlay = ({children}) => (
    <div css="position: relative">
        {children}
        <div css="position: absolute; top: 0; right: 0; bottom: 0; left: 0; background: rgba(255, 255, 255, 0.8)" />
    </div>
)

There is no problem with z-index. I think it is implemented the correct way?

[oliviertassinari]

z-index: 1 should do the trick :)