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

[TextField] Fix to handle onClick on root element #38072

Merged
merged 3 commits into from
Aug 14, 2023

Conversation

LukasTy
Copy link
Member

@LukasTy LukasTy commented Jul 20, 2023

#36892 changed the onClick handling behavior on the TextField component.
Previously the callback used to be registered on the root element and pickers relied on it (mui/mui-x#6074) to correctly handle click -> open behavior regardless of where the click has been done.
Now we have a regression mui/mui-x#9386 on the mentioned behavior because the fix has basically been negated.

Closes mui/mui-x#9386

If you have any better suggestions on how to solve this conundrum—I'm open to suggestions.

We have considered and tried using onMouseDown, but it has its drawbacks and interferes with other component behavior.
One other easy fix would be to remove the z-index: 1 on the label and figure out another way to achieve the same fix.

@LukasTy LukasTy added the component: text field This is the name of the generic UI component, not the React module! label Jul 20, 2023
@LukasTy LukasTy self-assigned this Jul 20, 2023
@mui-bot
Copy link

mui-bot commented Jul 20, 2023

Netlify deploy preview

https://deploy-preview-38072--material-ui.netlify.app/

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against ecb59eb

@zannager zannager requested a review from mj12albert July 21, 2023 05:10
@LukasTy LukasTy requested a review from sai6855 July 21, 2023 06:02
@mj12albert
Copy link
Member

@LukasTy I think this fix is ok ~ getting around the z-index on the label seems harder but we could look into it for v6, as that comment is 5 years old already ~

@michaldudak michaldudak merged commit 113429c into mui:master Aug 14, 2023
18 checks passed
Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the root problem is with #36892 (comment).

@oliviertassinari oliviertassinari added bug 🐛 Something doesn't work regression A bug, but worse labels Aug 14, 2023
mj12albert added a commit that referenced this pull request Aug 14, 2023
mj12albert added a commit to mj12albert/material-ui that referenced this pull request Aug 14, 2023
@oliviertassinari
Copy link
Member

oliviertassinari commented Aug 14, 2023

While this PR aims to fix a regression, I think it introduces another one, see https://codesandbox.io/s/suspicious-kirch-7wfhxw?file=/Demo.tsx

import * as React from "react";
import Box from "@mui/material/Box";
import TextField from "@mui/material/TextField";

export default function BasicTextFields() {
  return (
    <Box
      onClick={() => {
        console.log("Foo");
      }}
    >
      <TextField
        onClick={() => {
          console.log("textField");
        }}
      />
    </Box>
  );
}

"Foo" doesn't trigger anymore. I was triggered by seeing stopPropagation(). I think we should always see a flag https://css-tricks.com/dangers-stopping-event-propagation/. It can be fine, but it's rare.

mj12albert added a commit to mj12albert/material-ui that referenced this pull request Aug 15, 2023
@LukasTy LukasTy deleted the fix-text-field-root-click-handling branch August 16, 2023 07:06
mj12albert added a commit to mj12albert/material-ui that referenced this pull request Aug 21, 2023
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! regression A bug, but worse
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[pickers] Picker doesn't open when clicking on label position (& mouse up vs. mouse down)
5 participants