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

[Bug]: PasswordInput - onTogglePasswordVisibility handler returns multiple elements #15600

Closed
2 tasks done
silvaDominic opened this issue Jan 24, 2024 · 1 comment
Closed
2 tasks done
Labels
adopter: strategic-product Work-stream that directly effects the Product-led Growth initiative. status: needs triage 🕵️‍♀️ type: bug 🐛

Comments

@silvaDominic
Copy link

silvaDominic commented Jan 24, 2024

Package

@carbon/react

Browser

Chrome, Firefox

Package version

v11.48.0

React version

v18.2.0

Description

The onTogglePasswordVisibility handler is returning the exact element being clicked on rather than just the button or some other useful state object/prop.

Is this issue related to a specific component?

PasswordInput
packages/react/src/components/TextInput/PasswordInput.tsx

What did you expect to happen?

Expected to receive just the button element.

The doc comment and return type for the prop seems to indicate it should just be button:

  /**
   * Callback function that is called whenever the toggle password visibility button is clicked
   * @param evt Mouse event triggered by the password visibility `<button>`
   * @returns {void}
   */
  onTogglePasswordVisibility?: (
    evt: React.MouseEvent<HTMLButtonElement>
  ) => void;

What happened instead?

Exact element clicked is returned (e.g. svg, path, button).

What would you like to see changed?

It's not clear what the utility of receiving any click event is here (What would a user do with this information?). Personally, I thought it would give me the ability to override the default behavior which simply toggles the input type. Perhaps receiving the current input type would be passed back. Example:

<TextInput.PasswordInput
...
onTogglePasswordVisibility={(inputType) => myCustomHandler(inputType)}
...
...
/>

Reproduction/example

https://stackblitz.com/edit/github-q71nkn?file=src%2FApp.jsx

Steps to reproduce

Add this line:
onTogglePasswordVisibility={(e) => console.log(e.target)} // <--- This line

to the TogglePasswordVisibility component in the story:
packages/react/src/components/TextInput/TextInput.stories.js

Like so:

export const TogglePasswordVisibility = () => {
  return (
    <TextInput.PasswordInput
      id="text-input-1"
      labelText="Text input label"
      helperText="Optional help text"
      autoComplete="true"
      onTogglePasswordVisibility={(e) => console.log(e.target)} // <--- This line
    />
  );
};

Check the console and click on various spots on the visibility toggle. Observe the behavior as shown in video below.

Screen.Recording.2024-01-24.at.2.06.51.PM.mov

Suggested Severity

Severity 3 = User can complete task, and/or has a workaround within the user experience of a given component.

Application/PAL

Turbonomic

Code of Conduct

@tay1orjones
Copy link
Member

@silvaDominic We're not doing anything with the value returned directly from the react event

const handleTogglePasswordVisibility = (event) => {
setInputType(inputType === 'password' ? 'text' : 'password');
onTogglePasswordVisibility && onTogglePasswordVisibility(event);
};

<button
type="button"
className={passwordVisibilityToggleClasses}
disabled={disabled}
onClick={handleTogglePasswordVisibility}>

Expected to receive just the button element.

You should be able to get this via event.currentTarget, the element that listens to the event vs event.target, the element that triggered event.

Personally, I thought it would give me the ability to override the default behavior which simply toggles the input type.

If you wanted to, you could change the input type yourself based on this event using the type prop. It'll cause the component to re-render of course. I'm not sure what the use case would be for this though? It looks like the reason onTogglePasswordVisibility exists is to support the hybrid controlled/uncontrolled pattern of the internal state events being available to you if you'd prefer to have it fully controlled (via the type prop described earlier)

Based on this I'll close this one as I think it resolves the primary concerns you had. Let us know though - we can always reopen if you think there's more to address here. Thanks!

@tay1orjones tay1orjones closed this as not planned Won't fix, can't repro, duplicate, stale Jan 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
adopter: strategic-product Work-stream that directly effects the Product-led Growth initiative. status: needs triage 🕵️‍♀️ type: bug 🐛
Projects
Archived in project
Development

No branches or pull requests

2 participants