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

fix: placeholders & password autocomplete #78

Merged
merged 9 commits into from
May 17, 2024

Conversation

elenajdanova
Copy link
Contributor

@elenajdanova elenajdanova commented May 13, 2024

Screenshot 2024-05-16 at 1 56 22 PM

@elenajdanova elenajdanova requested a review from DrewHoo May 13, 2024 18:27
// https://developer.mozilla.org/en-US/docs/Web/Security/Securing_your_site/Turning_off_form_autocompletion
return (
<Input.Password
{...({ ...rest, autoComplete: "new-password" } as InputProps)}
Copy link
Contributor

Choose a reason for hiding this comment

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

Setting autoComplete to "new-password" will cause browsers to treat the field as a new password and offer to generate one, for instance. That may be a use case for some folks using this library, but I don't think it's safe to assume it's the primary use case. If you want it to have specific configurable behavior, I'd suggest modifying TextControlOptions in src/ui-schema.ts to have an inputProps property typed similarly to TextControlInputProps in this file, so that users can configured this behavior via a UISchema.

Copy link
Contributor Author

@elenajdanova elenajdanova May 14, 2024

Choose a reason for hiding this comment

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

it was just a workaround to prevent auto-filling the input cause as it's mentioned in the article I posted in the comment

Even without a master password, in-browser password management is generally seen as a net gain for security. Since users do not have to remember passwords that the browser stores for them, they are able to choose stronger passwords than they would otherwise.
For this reason, many modern browsers do NOT support autocomplete="off" for login field

But they do support new-password in the right way.

I def can pass it as an input prop

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I realized we already use autocomplete so it would be strange to override it somewhere else in the code

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should avoid accreting properties on TextControlOptions that are really just antd InputProps--did you try adding a inputProps property to TextControlOptions that's typed as antd's InputProps?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yup, @DrewHoo can you take a look?

@elenajdanova elenajdanova requested a review from DrewHoo May 14, 2024 18:42
Copy link

codecov bot commented May 14, 2024

Codecov Report

Attention: Patch coverage is 44.44444% with 5 lines in your changes are missing coverage. Please review.

Project coverage is 76.60%. Comparing base (768d8c4) to head (0557451).
Report is 2 commits behind head on main.

Files Patch % Lines
src/controls/TextControl.tsx 44.44% 4 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #78      +/-   ##
==========================================
- Coverage   76.78%   76.60%   -0.18%     
==========================================
  Files          36       36              
  Lines         435      436       +1     
  Branches       75       75              
==========================================
  Hits          334      334              
- Misses         77       78       +1     
  Partials       24       24              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

src/ui-schema.ts Outdated Show resolved Hide resolved
@elenajdanova elenajdanova requested a review from DrewHoo May 16, 2024 17:57
@elenajdanova elenajdanova changed the title Fix placeholders & password autocomplete Fix: placeholders & password autocomplete May 16, 2024
@elenajdanova elenajdanova changed the title Fix: placeholders & password autocomplete fix: placeholders & password autocomplete May 16, 2024
@elenajdanova elenajdanova merged commit f8e084a into main May 17, 2024
2 of 5 checks passed
@elenajdanova elenajdanova deleted the b/lakitu/fix-placeholders branch May 17, 2024 14:04
Copy link

🎉 This PR is included in version 1.15.4 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants