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

[FormControl] The addition of z-index 0 breaks third party dropdowns #19677

Closed
mbyrne00 opened this issue Feb 12, 2020 · 26 comments · Fixed by #20016
Closed

[FormControl] The addition of z-index 0 breaks third party dropdowns #19677

mbyrne00 opened this issue Feb 12, 2020 · 26 comments · Fixed by #20016
Labels
bug 🐛 Something doesn't work component: text field This is the name of the generic UI component, not the React module! good first issue Great for first contributions. Enable to learn the contribution process.

Comments

@mbyrne00
Copy link

mbyrne00 commented Feb 12, 2020

  • [ x ] The issue is present in the latest release.
  • [ x ] I have searched the issues of this repository and believe that this is not a duplicate.

Current Behavior 😯

After upgrading material-ui my dropdown that uses react-select appears behind any checkboxes. See example:
image

After much investigation it was caused by the addition of z-index: 0 on .MuiFormControl-root. If I do z-index: unset !important globally it fixes my issue, but I see that this was added to fix a blurry text issue: #19547

Expected Behavior 🤔

Expect not to set the stacking context for all form controls: https://philipwalton.com/articles/what-no-one-told-you-about-z-index/.

I'll admit I'm not a genius at this stuff, but seems a bit harsh to force z-index 0 for all field components and then require them to override. More than happy to be proven wrong and have an explanation though.

Steps to Reproduce 🕹

I use https://github.com/iulian-radu-at/react-select-material-ui and have a couple of checkboxes below my select field.

This same error does not happen for standard material select field because it seems to be adding dom elements to another root rather than as children. So it seems this will break any other third-party tools that add elements as children.

Context 🔦

My case was a search form with filters and checkboxes.

Your Environment 🌎

Tech Version
Material-UI v4.9.2
React v16.12.0
react-select-material-ui v6.3.0
@mbyrne00 mbyrne00 changed the title [FormControl] The addition of z-index 0 breaks dropdowns [FormControl] The addition of z-index 0 breaks third party dropdowns Feb 12, 2020
@oliviertassinari
Copy link
Member

Do you have a minimal reproduction? What does the current style prevent?
(I don't think that we should optimize for react-select-material-ui, we can leave it outside of the equation).

@oliviertassinari oliviertassinari added the component: text field This is the name of the generic UI component, not the React module! label Feb 12, 2020
@mbyrne00
Copy link
Author

mbyrne00 commented Feb 12, 2020

I can whip something up. It's not just react-select. It's any component that would render children and expect the z-index to take effect. Adding z-index: 0 to the form component restricts any of these.

Also it's not really the TextField component ... it's FormControl in general.

@oliviertassinari
Copy link
Member

oliviertassinari commented Feb 12, 2020

Setting a z-index solves the issue.

@oliviertassinari oliviertassinari added out of scope The problem looks valid but we won't fix it (maybe we will revisit it in the future) discussion and removed out of scope The problem looks valid but we won't fix it (maybe we will revisit it in the future) labels Feb 12, 2020
@oliviertassinari
Copy link
Member

oliviertassinari commented Feb 12, 2020

However, the requirement for a z-index on the form index (relative element) might be confusing: https://codesandbox.io/s/material-demo-zd19u, most people would expect it on the absolute positioned element. Let's wait to get more feedback before trading this for the cleaner text rendering. Thanks for raising.

@oliviertassinari oliviertassinari added the waiting for 👍 Waiting for upvotes label Feb 12, 2020
@mbyrne00
Copy link
Author

Exactly my point ... thanks. Another user raised it as an issue in the linked PR. For now I'll work around. Thanks for taking a look into it.

@mbyrne00
Copy link
Author

Hey @oliviertassinari - should this issue be re-opened considering you have asked for community discussion? It seems it's not closed just yet.

@oliviertassinari
Copy link
Member

@chybisov What do you think about the idea to revert the change in order to minimize unexpected behavior when attaching a pop-up to a form control?

@chybisov
Copy link
Contributor

@oliviertassinari good question... I think to minimize unexpected behavior we can make additional fix and apply z-index: 0 in FormControl only for variant: filled, because this is the only variant where z-index: 0 is really useful. This will be sort of trade off and I can provide PR for this. What do you think?

@oliviertassinari
Copy link
Member

@chybisov I suspect that consistency will help, so not changing the behavior based on the variant value.

@chybisov
Copy link
Contributor

chybisov commented Feb 18, 2020

@oliviertassinari perhaps, but variant: filled has text rendering issue and I think it's not good practice to leave such issues in codebase due to some 3rd party libs made some components over it. We should investigate more complex solution that will satisfy both sides, e.g. fix FormControl to not using z-index at all. I think there might be another solution. I'll try to look.

@chybisov
Copy link
Contributor

chybisov commented Feb 18, 2020

@oliviertassinari I have one idea. Have these lines 52 71 made for fixing Chrome autofill purposes only?

If so, we can remove z-index from InputLabel and FormControl and just swap these lines 168 173.

This will render label below input in DOM tree so we don't need z-index anymore, it works perfectly. because InputLabel has position: absolute and there is no difference whether we place it below or above input, it will also works with Chrome autofill.

Do you see any other pitfalls here?

@oliviertassinari
Copy link
Member

I think that swapping the dom structure would be equality deceptive than the z-index change, if not more. Also, as far as I remember, a few CSS selectors relies on the DOM structure.

@chybisov
Copy link
Contributor

chybisov commented Feb 18, 2020

@oliviertassinari could you please check this? I've tried it locally and it works fine. Currently I don't see any other possible solutions to fit both sides.

@oliviertassinari
Copy link
Member

oliviertassinari commented Feb 22, 2020

@chybisov I think that we should revert and ignore the blurry issue.
As far as I understand it, there is no clear explanation of why a z-index fixed the problem in the first place, nor that it will keep working.

diff --git a/packages/material-ui/src/FormControl/FormControl.js b/packages/material-ui/src/FormControl/FormControl.js
index 7419a431b..2aa6d4dc6 100644
--- a/packages/material-ui/src/FormControl/FormControl.js
+++ b/packages/material-ui/src/FormControl/FormControl.js
@@ -18,7 +18,6 @@ export const styles = {
     padding: 0,
     margin: 0,
     border: 0,
-    zIndex: 0, // Fix blur label text issue
     verticalAlign: 'top', // Fix alignment issue on Safari.
   },
   /* Styles applied to the root element if `margin="normal"`.

@oliviertassinari oliviertassinari added good first issue Great for first contributions. Enable to learn the contribution process. and removed discussion waiting for 👍 Waiting for upvotes labels Feb 22, 2020
@chybisov
Copy link
Contributor

@oliviertassinari blurry text with z-index and transitions is known chrome issue and I've already explained why this happen in PR.

@oliviertassinari
Copy link
Member

Does Chrome as an issue on its bug tracker we can look at?

@chybisov
Copy link
Contributor

@oliviertassinari I don't have exact link to an issue. Text becomes blurry when transform transition with z-index is used what other confirmations are needed? :)

@oliviertassinari
Copy link
Member

@chybisov Could it be a Chrome only thing? Does it reproduce on Safari of Firefox?

@chybisov
Copy link
Contributor

chybisov commented Feb 23, 2020

@oliviertassinari I just checked Chrome (Windows, Mac), Firefox (Windows, Mac), Safari and it seems only Chrome Windows has this issue.

@oliviertassinari oliviertassinari added good first issue Great for first contributions. Enable to learn the contribution process. bug 🐛 Something doesn't work and removed good first issue Great for first contributions. Enable to learn the contribution process. labels Feb 25, 2020
@oliviertassinari
Copy link
Member

@chybisov Ok, in this case, I think that we should remove the z-index. The fewer of them we have, the better.

@chybisov
Copy link
Contributor

@oliviertassinari I have one idea. Have these lines 52 71 made for fixing Chrome autofill purposes only?

If so, we can remove z-index from InputLabel and FormControl and just swap these lines 168 173.

This will render label below input in DOM tree so we don't need z-index anymore, it works perfectly. because InputLabel has position: absolute and there is no difference whether we place it below or above input, it will also works with Chrome autofill.

Do you see any other pitfalls here?

@oliviertassinari do you mean we should make above changes or just remove z-index as it was before and leave blur issues as is?

@oliviertassinari
Copy link
Member

I meant to leave the blur as is.

@chybisov
Copy link
Contributor

chybisov commented Feb 28, 2020

@oliviertassinari from design point of view I can't agree with you here 😁

@piotros
Copy link
Contributor

piotros commented Mar 6, 2020

Any updates on this issue? Will addition of z-index in FormControl be reverted?

@oliviertassinari
Copy link
Member

@piotros Do you want to submit a pull request for it :)?

@mbyrne00
Copy link
Author

mbyrne00 commented Mar 7, 2020

It’s a simple revert pending decision. Hardly needs a PR if decision hasn’t been made

piotros added a commit to piotros/material-ui that referenced this issue Mar 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work component: text field This is the name of the generic UI component, not the React module! good first issue Great for first contributions. Enable to learn the contribution process.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants