Repository navigation
PasswordInput: a password field with reveal and copy, instead of a hand-built row - #1253
JeanMarcMilletScality wants to merge 17 commits into
Conversation
…olve a nested field name
Hello jeanmarcmilletscality,My role is to assist you with the merge of this Available options
Available commands
Status report is not available. |
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
Peer approvals must include at least 1 approval from the following list: |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| // The confirmation's rule reads the password, so it has to be re-run | ||
| // when the password changes; without this a "do not match" error stays | ||
| // on screen after the user has already fixed it. | ||
| onChange: () => trigger(names.confirmation), |
There was a problem hiding this comment.
This validates the confirmation on every keystroke in the password field, even if the user hasn't reached it yet. With a resolver or a required/match rule, a "required" or "do not match" error shows under the empty confirmation as soon as the first character is typed. Only re-run it once the confirmation has a value or has been touched.
| onChange: () => trigger(names.confirmation), | |
| onChange: () => { | |
| if (getFieldState(names.confirmation).isDirty) trigger(names.confirmation); | |
| }, |
| revealed={revealed} | ||
| onToggleReveal={toggle} | ||
| required={required} | ||
| {...register(names.password, { |
There was a problem hiding this comment.
required only sets the asterisk and the HTML attribute. Form renders with noValidate, so in a form without a resolver an empty password still submits. Either pass required through to register (both fields), or say in the JSDoc that required is visual only and must be enforced in the schema.
…onfirmation alone `Form` renders with `noValidate`, so the `required` prop only produced an asterisk and an inert DOM attribute — without a resolver an empty field submitted in silence. It is now registered as a rule on both fields, with the caller's own confirmation rules keeping priority. The password's change handler re-validated the confirmation unconditionally, so the first keystroke raised the confirmation's error under a field the user had not reached yet. It now waits for that field to be dirty.
… point, and cover the copy change The In A Narrow Container story was fixed at 18rem, wider than the row's own minimum, so the field never reached its floor and the story demonstrated nothing. It is now narrower and resizable. CopyButton's test file gains the two cases the widened textToCopy introduced: a getter read at the click rather than at the render, and an empty source that copies nothing and leaves the button idle. Also adds the guideline page CONTRIBUTING.md asks for, says in PasswordFields' JSDoc when to reach for it, and hides the story's username input off-screen rather than with display: none, which several password managers skip.
…oor next to its reader getFieldState read dirtyFields without passing formState, so the confirmation's re-validation worked only because the error read beside it happened to subscribe that slice. Changing the error read would have silently stopped the re-validation, with no test to say so. The floor constant moves from inputv2, which never used it, into the row that is its only reader; inputv2 is no longer touched by this branch. The narrow container story clips instead of scrolling and starts wide enough to have travel before the floor, and the guideline drops imports it never used and controls that drive a story with no args. Adds the test that the caller's own confirmation rules outrank the required rule.
| application, which is the only side that has the username. */} | ||
| <input | ||
| type="text" | ||
| autoComplete="username" |
There was a problem hiding this comment.
This off-screen input can still be focused and has no label. Keyboard users tab into a field they can't see, and screen readers read out an unnamed text box. This story is the pattern callers will copy, so take it out of the tab order and hide it from assistive tech.
| autoComplete="username" | |
| autoComplete="username" | |
| tabIndex={-1} | |
| aria-hidden="true" |
| else return '20.5rem'; | ||
| }; | ||
|
|
||
|
|
There was a problem hiding this comment.
Stray blank line. It is the only change to inputv2.tsx, and the PR description says this file is unchanged. Remove it.
🟡 Approve with nitsGood to merge as it stands — the point(s) below are nits: worth closing, not required. Findings
This review is advisory and does not block the merge. Full reviewFindings
|
…d leave inputv2 untouched
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
Peer approvals must include at least 1 approval from the following list: |
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
Peer approvals must include at least 1 approval from the following list: |
…tory's username field The reveal toggle changed the field and its own icon without saying anything a screen reader could hear. It now carries aria-pressed, so both states are announced rather than left to be inferred from the tooltip. The username field the Set A Password story carries for password managers was off screen but still focusable and unnamed, so a keyboard user landed in a field they could not see — in the pattern callers copy. It now leaves the tab order and the accessibility tree. The two copy tests replaced navigator.clipboard and never put it back, and the field-group test stopped at the buttons: it also pins the field's own disabled state now, which is the state a review reported as missing and the code already had.
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
Peer approvals must include at least 1 approval from the following list: |
| export type PasswordFieldsProps = { | ||
| names?: { password: string; confirmation: string }; | ||
| labels?: { password: string; confirmation: string }; | ||
| required?: boolean; |
There was a problem hiding this comment.
required is enforced as a field-level rule, so in a form with a resolver it only draws the asterisk and an empty password still passes unless the schema rejects it. Hover help is the only doc most callers read, so say so on the prop.
| required?: boolean; | |
| /** | |
| * Marks both fields required and adds a `required` rule to each. A form with a | |
| * `resolver` skips field-level rules, so there the schema must enforce it. | |
| */ | |
| required?: boolean; |
PasswordFields renders its own two FormGroups, so a caller had no way in: the prop did not exist, and wrapping the component in a disabled group is not open to them. It takes `disabled` now and passes it to both groups, which reaches the two inputs and the shared reveal and copy buttons through the field context.
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
Peer approvals must include at least 1 approval from the following list: |
… visual only `required` registers a react-hook-form rule, since Form renders noValidate and the DOM attribute enforces nothing on its own. A form supplying a resolver never runs field-level rules, so there the prop only draws the asterisk and an empty field submits. The component's own JSDoc said this about the match rule, but the props table renders the `required` row on its own, where a caller meets the prop and the component blurb is nowhere in sight.
| // `Form` renders with `noValidate`, so the `required` attribute enforces nothing | ||
| // on its own: without a rule an empty field submits silently. | ||
| const requiredRule = (label: string) => | ||
| required ? { required: `${label} is required` } : {}; |
There was a problem hiding this comment.
disabled only reaches the DOM through FormGroup. React Hook Form still runs register rules on a field that is disabled only by the HTML attribute. So <PasswordFields required disabled /> shows "Password is required" on fields the user cannot edit, and blocks submit. Pass disabled to register too, so RHF skips validation and leaves the values out of the submit.
| required ? { required: `${label} is required` } : {}; | |
| ({ disabled, ...(required ? { required: `${label} is required` } : {}) }); |
disabled reached the DOM through FormGroup only, and react-hook-form keeps evaluating a field's rules when the attribute is the only thing disabling it. A required pair rendered disabled therefore raised its own required errors on two fields nobody could fill, and held the submit back for good. Passing disabled to register skips those rules and leaves both values out of the submit, which is what a disabled control does in a plain form.
| * below its own content; without it the field's `max-width: 100%` has nothing to | ||
| * resolve against and the row refuses to give ground. | ||
| */ | ||
| export const PasswordRow = styled.div` |
There was a problem hiding this comment.
Could it be core-ui stack ?
There was a problem hiding this comment.
Not really for the usage here 🫤
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
Peer approvals must include at least 1 approval from the following list: |
| * the submit forever. | ||
| */ | ||
| disabled?: boolean; | ||
| copyable?: boolean; |
There was a problem hiding this comment.
When I read copyable I imagine that when it's false we prevent copying by preventing selection or other shenanigans. I think the name is very unclear for what it does and never explained in js doc
|
It's strange to not give a default value to that autoComplete field, why ? |
| if (typeof ref === 'function') ref(node); | ||
| else if (ref) ref.current = node; | ||
| }, | ||
| [ref], |
There was a problem hiding this comment.
ref is a new function on every render, this callback is always recomputed
There was a problem hiding this comment.
I changed the way the value is get from the input to avoid this
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
Peer approvals must include at least 1 approval from the following list: |
It's up to the user to put if it's for setting a new password or asking the current one, so I did not set a default. But we could decide which one is the default |
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
Peer approvals must include at least 1 approval from the following list: |
This branch has not been deployed
4 participants
TL;DR — Asking a user for a password meant assembling a field, a reveal toggle and a copy button by hand every time;
PasswordInputandPasswordFieldsnow ship that row.📷 Screenshots
Components/Inputs/PasswordInput › Set A Password — the pair inside a responsive
Form, before and after one press of the eye. The reveal belongs to the pair, not to each field: the confirmation has no toggle of its own and follows the password's.Components/Inputs/PasswordInput › In A Narrow Container — the same row at three container widths. The field gives ground alone until it reaches its 84px floor; the buttons hold 32px throughout, and past the floor the row overflows and the parent clips it rather than the field collapsing.
Context / Why
Three applications were found to have written the same row independently, and none of the three agreed with the others. What they shared was written out by hand and identically: a reveal button in the
outlinevariant,spacing.r36wide, with Show password / Hide password. What they disagreed on is the part that fails silently — theautoCompletevalue, whether a copy affordance belonged on the row at all, and whether the row could shrink. One of the three also sits outside a responsive form and overflows rather than giving ground.🧩 Approach
One internal row holds a field and its affordances and is written once.
PasswordInputis a field, not a labelled row — the caller still wraps it in aFormGroup, which is what keeps it usable in a two-column form, in a modal, and in a layout nobody has designed yet.The row is a plain
styled.divrather than core-ui'sStack:Stacktypes its children as an array, which the pair's single-child confirmation row cannot satisfy, and its API has no way to express themin-width: 0the row depends on.Sweeping In A Narrow Container from 320px down to 112px: the two buttons hold 32px at every width and never shrink, the field alone gives ground until it reaches its 84px floor at a 160px container, and below that the overflow grows one pixel per pixel lost. Overflow therefore begins exactly at the floor plus the gap plus the two buttons, which is the behaviour the floor exists to produce. Set A Password inside a responsive
Formoverflows at no width between 1200 and 320px: the label track stacks around 440–500px and hands the width back to the field, so inside a form the field never reaches its floor at all.The field is always shrinkable, so
fluidleaves the prop surface;typeleaves it too, because the reveal button owns it.revealedandonToggleRevealare a pair or neither, so a controlled reveal cannot be passed without the handler that makes the button work —PasswordFieldsis the first user of that pair.autoCompletebecomes required, with no default:current-passwordandnew-passwordboth look right on screen and only one of them behaves, so there is no value that can be assumed.PasswordFieldsis the one exception to "a field, not a labelled row" — it is the whole pair, so it renders its own twoFormGroups and registers both fields into the caller'sFormProvider.🔧 Usage
CopyButton.textToCopywidens to accept a getter, evaluated at the click:An uncontrolled field does not re-render as it is typed into, so a string captured at render is the one from before the typing. Reading at click time also removes the need to subscribe the whole form to every keystroke through
watch().🔍 Review focus
buttonv2/CopyButton.component.tsx › onClick— behaviour change for every existing caller, not just the new one: a source that resolves empty no longer flips the button to its success state. Until nowwriteText('')resolved, so the button claimed Copied! and replaced whatever was in the clipboard with nothing. The guard is on the collapsed union rather than on the getter alone, and that is the decision to check: narrowing it totypeof textToCopy === 'function'would leave every shipped string caller still overwriting the clipboard on an empty source, and would give one prop two behaviours depending on the shape it is passed. The cost of guarding is that the click becomes silent — a visible empty state is worth having and is deliberately not in this PR.passwordinput/PasswordFields.component.tsx › requiredRule—requiredis enforced as a react-hook-form rule becauseFormrendersnoValidate, so the DOM attribute enforces nothing. A form with aresolvertakes a different path throughhandleSubmitand never runs field-level rules, sorequiredis inert there and the schema has to carry it — the same caveatconfirmationRulesalready documents. That caveat now sits on the prop's own documentation rather than only on the component's: the props table and the editor's hover are where a caller meetsrequired, and what it guards against fails silently.passwordinput/PasswordInput.component.tsx › textToCopy— the forwardedrefreaches the field untouched, so the copy button reads the live value through the row instead:rowRef.current?.querySelector('input'). Putting that ref back on the field would take the slot the caller's registration needs. The row holds one input and is not exported, and a second one there makes the two copy tests fail.passwordinput/PasswordRow.tsx › PasswordFieldSlot—flex: 0 1 autois load-bearing.Inputcaps itself atmax-width: 100%but never grows past the width itssizeasks for, so a growing slot stretches away from the field and leaves the gap between the field and its buttons instead of after them.passwordinput/PasswordFields.component.tsx › errorOf— errors are read throughgetFieldState, not by indexingformState.errors. A field name may be a path, anderrors['credentials.password']isundefined, which renders a field with no message while the form refuses to submit.🧪 How to test
Follow-up
What changed alongside
Form'sresponsiveJSDoc still claimedSearchInputkept a fixed width. It has shrunk since that sentence was written, and a password field now does too, so the sentence named the wrong components twice over. It now namesTextArea— sized from its owncols— as the one that genuinely keeps its width.The row's floor is
convertSizeToRem('1/3'), read frominputv2where the sizes are defined rather than spelled out a second time, and the constant lives inPasswordRow.tsxwith its only reader.inputv2itself is unchanged by this branch.The
Set A Passwordstory hides itsusernameinput off-screen rather than withdisplay: none, which several password managers skip. The field itself stays the application's to render — it is the only side that knows the username — so the story is the only place this library can show the shape.The branch adds the guideline page
CONTRIBUTING.mdasks for, and the two rows it adds toComponentIndex.mdxlink to it, as every other component with a guideline does.Both components are exported from
next.tsrather thanindex.ts. They are new rather than redesigns, which by the letter of the contributing rules points atindex.ts;next.tsis chosen becauseInput,ButtonandCopyButton— everything these compose — already live there, and a password field imported from the root while its own field comes fromnextreads as an oversight.