[Checkbox] You can click through an overlay to check a checkbox

steverecio commented 4 years ago

Paper component (elevation=1) accepts clicks from UI elements underneath. I have the following code:

<Popper>
  <Paper>
     <ClickAwayListener>
         // some stuff here
     </ClickAwayListener>
  </Paper>
</Popper>

Any elements (buttons, checkboxes, etc) that sit below the popper / paper component above, accept click events. The component should block any click events from flowing to components underneath it.

Tech	Version
Material-UI	v4.9.9
React	v16.8.0
Browser	Chrome
TypeScript
etc.

eps1lon commented 4 years ago

Could you share a codesandbox and describe the expected and current behavior in more detail? It's not clear why a clickawaylistener should prevent click events.

steverecio commented 4 years ago

@eps1lon https://codesandbox.io/s/material-demo-mu35u?file=/demo.js

Notice how you can still toggle the checkbox underneath the paper when the popper is open.

Note: This seems to only be the case with MUI checkboxes. I tested it with a simple html button and I'm unable to trigger the click event in that case (when its underneath the paper).

oliviertassinari commented 4 years ago

I have no idea how to explain this behavior. Adding a z-index: 1 on the Popper solves the issue, however, it's not something we can apply. I would propose we ignore the issue.

mrcoles commented 4 years ago

the issue @steverecio is seeing appears to be more with the '@material-ui/core/Checkbox' element than the 'Popper'. Upon inspecting the rendered “Checkbox” it has an invisible (opacity: 0;) checkbox that ends up rendering above the popper, because the same checkbox has z-index: 1;.

It’s not clear to me that the z-index setting on the hidden input is necessary or why there isn’t a different solution without z-index—I see Checkbox.js gets it from SwitchBase.js. However, I see it was added in this PR https://github.com/mui-org/material-ui/pull/17389 to fix this bug: https://github.com/mui-org/material-ui/issues/17366

oliviertassinari commented 4 years ago

Looking back at history, we have #18905 that is very related. I would propose this resolution: in the documentation, we encourage setting a z-index to the Popper component.

oliviertassinari commented 4 years ago

@steverecio You can manually set a z-index on the Popper element to solve the issue.

steverecio commented 3 years ago

Here is a code sandbox: https://codesandbox.io/s/material-demo-forked-mldm9?file=/demo.js:0-674

Note that the button is unclickable but the Checkbox accepts clicks / hovers when it should not be receiving user input.

I would suggest re-opeing this issue as z-index does not fix this example.

oliviertassinari commented 3 years ago

@steverecio Thanks for the reproduction. I have further simplified it down to https://codesandbox.io/s/material-demo-forked-hq39t?file=/demo.js.

import React from "react";

export default function SimpleModal() {
  return (
    <div>
      <div style={{ display: "flex" }}>
        <input defaultChecked type="checkbox" style={{ zIndex: 1 }} />
      </div>
      <input defaultChecked type="checkbox" style={{ position: 'absolute', zIndex: 1 }} />
      <br />
      text bellow the overlay
      <div
        style={{
          position: "fixed",
          backgroundColor: "rgba(0, 0, 0, 0.2)",
          top: 0,
          left: 0,
          right: 0,
          bottom: 0
        }}
      />
    </div>
  );
}

I can reproduce it in Firefox, Safari, Chrome, Edge, so it seems to be per the spec. The zIndex is present to fix #17366.

oliviertassinari commented 3 years ago

One solution is:

diff --git a/packages/material-ui/src/internal/SwitchBase.js b/packages/material-ui/src/internal/SwitchBase.js
index 16d9fd3e21..d731d5f85f 100644
--- a/packages/material-ui/src/internal/SwitchBase.js
+++ b/packages/material-ui/src/internal/SwitchBase.js
@@ -36,7 +36,6 @@ const SwitchBaseInput = experimentalStyled('input')({
   left: 0,
   margin: 0,
   padding: 0,
-  zIndex: 1,
 });

 /**
@@ -141,6 +140,7 @@ const SwitchBase = React.forwardRef(function SwitchBase(props, ref) {
       ref={ref}
       {...other}
     >
+      {checked ? checkedIcon : icon}
       <SwitchBaseInput
         autoFocus={autoFocus}
         checked={checkedProp}
@@ -159,7 +159,6 @@ const SwitchBase = React.forwardRef(function SwitchBase(props, ref) {
         value={value}
         {...inputProps}
       />
-      {checked ? checkedIcon : icon}
     </SwitchBaseRoot>
   );
 });

but it breaks https://next.material-ui.com/components/radio-buttons/#customized-radios and https://next.material-ui.com/components/checkboxes/#customized-checkbox. We would need to fix them with:

diff --git a/docs/src/pages/components/checkboxes/CustomizedCheckbox.tsx b/docs/src/pages/components/checkboxes/CustomizedCheckbox.tsx
index 6be50606d1..19dc8ba76c 100644
--- a/docs/src/pages/components/checkboxes/CustomizedCheckbox.tsx
+++ b/docs/src/pages/components/checkboxes/CustomizedCheckbox.tsx
@@ -13,10 +13,10 @@ const Icon = styled('span')({
     outline: '2px auto rgba(19,124,189,.6)',
     outlineOffset: 2,
   },
-  'input:hover ~ &': {
+  '.MuiCheckbox-root:hover &': {
     backgroundColor: '#ebf1f5',
   },
-  'input:disabled ~ &': {
+  '.MuiCheckbox-root.Mui-disabled &': {
     boxShadow: 'none',
     background: 'rgba(206,217,224,.5)',
   },
@@ -35,7 +35,7 @@ const CheckedIcon = styled(Icon)({
       "1.003 0 00-1.42 1.42l3 3c.18.18.43.29.71.29s.53-.11.71-.29l5-5A1.003 1.003 0 0012 5z' fill='%23fff'/%3E%3C/svg%3E\")",
     content: '""',
   },
-  'input:hover ~ &': {
+  '.MuiCheckbox-root:hover &': {
     backgroundColor: '#106ba3',
   },
 });
diff --git a/docs/src/pages/components/radio-buttons/CustomizedRadios.tsx b/docs/src/pages/components/radio-buttons/CustomizedRadios.tsx
index bc780d19a5..87416589f9 100644
--- a/docs/src/pages/components/radio-buttons/CustomizedRadios.tsx
+++ b/docs/src/pages/components/radio-buttons/CustomizedRadios.tsx
@@ -17,10 +17,10 @@ const Icon = styled('span')({
     outline: '2px auto rgba(19,124,189,.6)',
     outlineOffset: 2,
   },
-  'input:hover ~ &': {
+  '.MuiRadio-root:hover &': {
     backgroundColor: '#ebf1f5',
   },
-  'input:disabled ~ &': {
+  '.MuiRadio-root.Mui-disabled &': {
     boxShadow: 'none',
     background: 'rgba(206,217,224,.5)',
   },
@@ -36,7 +36,7 @@ const CheckedIcon = styled(Icon)({
     backgroundImage: 'radial-gradient(#fff,#fff 28%,transparent 32%)',
     content: '""',
   },
-  'input:hover ~ &': {
+  '.MuiRadio-root:hover &': {
     backgroundColor: '#106ba3',
   },
 });

A breaking change.

The only other alternative I can think of is to make the lack of z-index easier to spot (not requiring an interaction) with:

diff --git a/packages/material-ui/src/internal/SwitchBase.js b/packages/material-ui/src/internal/SwitchBase.js
index 16d9fd3e21..4d1a93d3bf 100644
--- a/packages/material-ui/src/internal/SwitchBase.js
+++ b/packages/material-ui/src/internal/SwitchBase.js
@@ -23,6 +23,7 @@ const useUtilityClasses = (styleProps) => {
 const SwitchBaseRoot = experimentalStyled(IconButton)({
   /* Styles applied to the root element. */
   padding: 9,
+  zIndex: 1,
 });

 const SwitchBaseInput = experimentalStyled('input')({

oliviertassinari commented 3 years ago

It seems we don't get any preference on the direction we should take. In this case, I would recommend the first one. Solve once for all.

steverecio commented 3 years ago

@oliviertassinari Thanks for looking into this. I would vote for whichever solution is default for native html input checkboxes. As long as the solution is easy to find / documented, then the default being most similar to html has my vote.

mare1601 commented 3 years ago

Hi! Thread seems quiet and I'd love to snag this issue - will tackle if that's cool 😊

mare1601 commented 3 years ago

My solution wasn't up to snuff - someone else is welcome to take the ticket 🙂

anushaNemilidinne commented 2 years ago

is this issue still open?

Avni1802 commented 2 years ago

Can i work on this issue??

oliviertassinari commented 2 years ago

@Avni1802 Yes. @michaldudak any concern with swapping the position of the input before the overlay?

Avni1802 commented 2 years ago

We just need to remove z-index: 1 from the class named PrivateSwitchBase-input-7 because as it is z-index : 1 it will always be on the top layer and the click event triggers. For better understanding i am attaching the image. Screenshot (260)_LI

michaldudak commented 2 years ago

@michaldudak any concern with swapping the position of the input before the overlay?

No, I don't see any issue with that. However, is removing the z-index from the input not sufficient?

oliviertassinari commented 2 years ago

The <input> needs to be last in the DOM to render above it's previous siblings.

michaldudak commented 2 years ago

All right, looks like it should work, then.

BensonLiao commented 2 years ago

I vote for removing the z-index too, the native radio button using auto and it works fine on all these cases.

And I've also done a few test about this issue under the environment according to #17366, you can see the one with implicit label doesn’t have this problem.

I think the problem could be the lack of label, maybe put <label> element as default input sibling, whatever implicit or explicit ways like change one of span to label 螢幕快照 2021-08-04 下午8 28 20 , or expose it as an option to the user ? thanks for reading~

oliviertassinari commented 2 years ago

@BensonLiao Please go ahead with a PR

BensonLiao commented 2 years ago

Thanks and I'm honor to take the ticket! You agree with this solution too? I though it may raise a debate for paring the label with some type of input, it seems no definite answer for now, while I personally stands for pairing label with these type of input, because it provide more a11y and focus/activate area to trigger event like MDN says.

oliviertassinari commented 2 years ago

@BensonLiao The label is off-topic to the problem we are going after. The checkbox is standalone.

BensonLiao commented 2 years ago

@oliviertassinari Thanks for reply~ so it seems this checkbox problem was not that simple if we just remove z-index from SwitchBase? ok then I'll try go for some test with the label issue under current environment.

BensonLiao commented 2 years ago

Hi @oliviertassinari, one little question. so the PR for radio's label workaround should link back to #17366, right? thanks.