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

Disabled Button focus within Portal #23493

Open
2 tasks done
Mangatt opened this issue Nov 12, 2020 · 9 comments
Open
2 tasks done

Disabled Button focus within Portal #23493

Mangatt opened this issue Nov 12, 2020 · 9 comments
Labels
bug 🐛 Something doesn't work component: button This is the name of the generic UI component, not the React module!

Comments

@Mangatt
Copy link
Contributor

Mangatt commented Nov 12, 2020

When is disabled Button placed within Portal, parent outside Portal does not receive focus properly.

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

Current Behavior 😯

When you click on normally placed Button, parent receives focus correctly even when that Button is disabled. When you place that Button in portal, only enabled Button do that.

Expected Behavior 🤔

Events should work exactly the same in both cases, no matter if Portal is used.

Steps to Reproduce 🕹

https://codesandbox.io/s/material-ui-issue-forked-eu17z?file=/src/Demo.js

Steps:

  1. Click on normally placed enabled/disabled Button. Parent receives focus (marked by red outline)
  2. Click on enabled Button in Portal. Parent receives focus as well.
  3. Click disabled Button in Portal. Parent does not receive or lose focus depending on previous state.

Context 🔦

Focused state of parent affects display of some controls within its tree. When user clicks disabled Button, these controls are hidden even though they should be displayed.

Your Environment 🌎

Tech Version
Material-UI v4.11.0
React 17.0.1
Browser Chrome 86
@Mangatt Mangatt added the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Nov 12, 2020
@oliviertassinari oliviertassinari added external dependency Blocked by external dependency, we can’t do anything about it and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Nov 12, 2020
@oliviertassinari
Copy link
Member

Why is this not an issue with React?

@Mangatt
Copy link
Contributor Author

Mangatt commented Nov 12, 2020

@oliviertassinari Because it is Material UI problem, see updated demo with regular buttons:
https://codesandbox.io/s/material-ui-issue-forked-eu17z?file=/src/Demo.js

When parent has focus and you click on normal disabled button, nothing happens. When MUI Button is clicked, focus is lost.

@eps1lon eps1lon added bug 🐛 Something doesn't work component: button This is the name of the generic UI component, not the React module! and removed external dependency Blocked by external dependency, we can’t do anything about it labels Nov 12, 2020
@eps1lon
Copy link
Member

eps1lon commented Nov 12, 2020

This happens because we disable pointer-events when the button is disabled. I'm currently working on removing that behavior but it requires some more work. You can follow #23101

Wrong analysis. Will keep you posted.

@oliviertassinari
Copy link
Member

oliviertassinari commented Nov 12, 2020

I think that we can close, it's an edge case (i.e. negative ROI). It has to do with the nested span.

@Mangatt
Copy link
Contributor Author

Mangatt commented Nov 12, 2020

Is there a workaround then? Because that way we can't use any MUI Button-based component at all.

@eps1lon
Copy link
Member

eps1lon commented Nov 12, 2020

That's definitely odd behavior but since click-focus is not specified and up to the user-agent not very surprising. The inner span is on the chopping block for v5 anyway but we'll need to investigate it first. Alternatively we can work around it with pointerdown events.

@eps1lon eps1lon added this to the v5 milestone Nov 12, 2020
@eps1lon
Copy link
Member

eps1lon commented Nov 12, 2020

Is there a workaround then?

const Button = React.forwardRef(function Button(props, ref) {
  const { onPointerDown = () => {}, disabled, ...other } = props;

  function handlePointerDown(event) {
    if (disabled) {
      event.preventDefault();
    }
    onPointerDown(event);
  }

  return (
    <MuiButton
      ref={ref}
      disabled={disabled}
      onPointerDown={handlePointerDown}
      {...other}
    />
  );
});

-- https://codesandbox.io/s/material-ui-issue-forked-ybvw2?file=/src/Demo.js:141-535

@Mangatt
Copy link
Contributor Author

Mangatt commented Nov 12, 2020

@eps1lon Thanks, but this is solution just for Button alone, not for every other Button based components. I'll have something more generic in mind, hopefully it will work.

@Mangatt
Copy link
Contributor Author

Mangatt commented Nov 12, 2020

Ok, just for reference, when any parent element of Button inside portal is able to receive focus, everything works fine:

https://codesandbox.io/s/material-ui-issue-forked-mzj74?file=/src/Demo.js

For time being we will use this workaround, it is probably most universal, even though still not entirely optimal.

@oliviertassinari oliviertassinari removed this from the v5-beta milestone May 8, 2021
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: button This is the name of the generic UI component, not the React module!
Projects
None yet
Development

No branches or pull requests

3 participants