feat(fxa-settings): Add input component with separated characters #16741

vpomerleau · 2024-04-18T21:32:03Z

Because

New designs for reset password with code include code inputs with individual input boxes

This pull request

Create a TotpInputGroup component that contains individual input boxes for each character, with associated methods for handling input, keyboard navigation (including arrow keys, backspace, delete), pasting content into the boxes
Add inline error feedback below new code input component, instead of tooltip
Create a FormVerifyTotp component that includes the new TotpInputGroup (configurable for 6 or 8 digit codes) and submission button, with pre-submission validation that code contains the expected number of digits and no other characters
Add l10n and storybooks

Issue that this pull request solves

Closes: #FXA-7888

Checklist

Put an x in the boxes that apply

My commit is GPG signed.
If applicable, I have modified or added tests which pass locally.
I have added necessary documentation (if appropriate).
I have verified that my changes render correctly in RTL (if appropriate).

Screenshots (Optional)

**Storybook links:
FormVerifyTotp with 6-digit input

FormVerifyTotp with 8-digit input

FormVerifyTotp with error on submit

Other information (Optional)

Notes: PIN code must be displayed LTR for BOTH left-to-right and right-to-left languages

vpomerleau · 2024-04-22T23:42:35Z

packages/fxa-settings/src/components/TotpInputGroup/index.tsx

+      focusOnSpecifiedInput(index);
+    }
+  };
+  // TODO in FXA-7888 review for LTR/RTL


@bcolsson I've seen some comments that indicate that PIN code inputs should be rendered LTR for both LTR and RTL languages. Do you know if this is accurate?

In most RTL languages that I'm aware of, numbers are typically written LTR (though it gets more confusing if they're separated into groups of numbers - like phone numbers), so the comment linked makes sense.

In this case, yeah I think it's accurate to use LTR for RTL languages here.

LZoog

This is looking so good! I'm going to leave these comments for now, they are all nits, and I'm currently checking it out with a screenreader. I'll let you know if I've got any suggestions.

LZoog · 2024-04-23T15:28:53Z

packages/fxa-settings/src/components/TotpInputGroup/index.tsx

+          // also ensure that emailed code is displayed in correct direction or might fail
+          'flex my-2 rtl:flex-row-reverse',
+          codeLength === 6 && 'gap-4',
+          codeLength === 8 && 'gap-2'


Can we add a blue focus color so it doesn't use the system default (mine's showing up pink)? Not sure if you'll need anything else besides the shadow-input-blue-focus class.

What do you think about codeLength === 8 && 'gap-1 mobileLandscape:gap-2' to help a little bit with mobile, especially small mobile devices? 320px width before and after:

This also looks OK to me with the standard mobile width (375px):

packages/fxa-settings/src/components/FormVerifyTotp/index.tsx

LZoog · 2024-04-23T15:38:23Z

packages/fxa-settings/src/components/FormVerifyTotp/index.tsx

+    <form
+      noValidate
+      className="flex flex-col gap-4 my-6"
+      onSubmit={(e) => handleSubmit(e)}


You can also just do: onSubmit={handleSubmit(e)}

Tried, but does not work. e is not defined.

I think it's actually onSubmit={handleSubmit} (if you want to use the shorthand)

git grep " onSubmit=" | cat

packages/fxa-settings/src/components/FormVerifyTotp/index.tsx

LZoog · 2024-04-23T15:44:09Z

packages/fxa-settings/src/components/TotpInputGroup/index.test.tsx

+  });
+
+  describe('keyboard navigation', () => {
+    it('can navigate between inputs with arrow keys', async () => {


🎉 Very nice, and nice to have tests for these too!

LZoog · 2024-04-23T16:01:10Z

packages/fxa-settings/src/components/TotpInputGroup/index.tsx

+    let currentCodeArray = [...codeArray];
+    currentCodeArray[index] = undefined;
+    await setCodeArray(currentCodeArray);


Couple nits:

Can currentCodeArray become a const (also in delete fn)?

Does setCodeArray need an await (also in delete fn)?

Drop focusOnSpecifiedInput(index); to its own line after the else?

Yes to 1 and 3, but await is needed for setCodeArray else there are glitches with the focus change in the next step.

🤔 We might need a comment above that then. When I was in this branch I saw my linter saying "await" wasn't needed. Not sure how that works since it is sync.

packages/fxa-settings/src/components/TotpInputGroup/index.tsx

packages/fxa-settings/src/components/TotpInputGroup/mocks.tsx

LZoog · 2024-04-23T16:49:08Z

@vpomerleau Just tested with VO on Mac and here's some of my thoughts:

Not really a11y related but do we want to allow non-numbers at all to be entered for this case, e.g. check for that regex you made in handleInput? I've seen inputs that just don't type the character in that case.
I would update the label from "Code digit {inputNumber}" to "Code digit {inputNumber} of {codeLength}". I would also add an aria-label on the input for this label that reads the same thing, because when I'm simply typing numbers with a screenreader, it doesn't look like the updated label is read per textfield.
It would be nice if the entered digit could be read before navigating to the next input. This may need to be done with a setTimeout but hopefully not, I'd have to play around with it. Otherwise mistyping and not realizing it could be really easy.
I'm not sure if we need to account for up and down arrow keys to increase or decrease the value. I'm imagining a blind person accidentally typing "4" instead of "5", hearing it (point before this one), and backspacing to tab backwards and then wanting to just increase the value by 1. We could ask the folks in #a11y for a review via Storybook link after this is in and see what their opinion is?
I think it'd be lovely if the user could hear the entire number when the reader is on the submit button. Sighted users can see the full number and then "Submit", so it'd be nice if SRs could read "Submit 12345678". It gives them a chance to review the number before they submit it in case it's wrong.

Excellent work! 👏

packages/fxa-settings/src/components/FormVerifyTotp/en.ftl

packages/fxa-settings/src/components/TotpInputGroup/index.tsx

packages/fxa-settings/src/components/FormVerifyTotp/index.tsx

packages/fxa-settings/src/components/TotpInputGroup/index.tsx

LZoog

LGTM, thanks for all your work here!

LZoog · 2024-05-01T15:01:47Z

packages/fxa-settings/src/components/FormVerifyTotp/en.ftl

@@ -0,0 +1,5 @@
+## FormVerifyTotp
+
+# When focused on the button, screen reader will read the action and entire number that will be submitted


LZoog · 2024-05-01T15:06:19Z

packages/fxa-settings/src/components/FormVerifyTotp/index.test.tsx

+      expect(button).toBeDisabled();
+
+      await waitFor(() => {
+        user.click(screen.getByRole('textbox', { name: 'Digit 1 of 6' }));


This reads so much nicer in my opinion, not nearly as easy to get lost when tabbing forwards/backwards through the inputs. 😍

LZoog · 2024-05-01T15:31:23Z

packages/fxa-settings/src/components/FormVerifyTotp/index.tsx

+          <button
+            type="submit"
+            className="cta-primary cta-xl"
+            disabled={isSubmitting || codeArray.includes(undefined)}


Maybe stringifiedCode.length < codeLength is more clear than codeArray.includes(undefined)? I don't feel strongly at all though, I just squinted at this for a sec before realizing what it was for.

Agreed!! Will update

LZoog · 2024-05-01T15:31:57Z

packages/fxa-settings/src/components/TotpInputGroup/index.test.tsx

+      // and focus shifts to next input
+      expect(
+        screen.getByRole('textbox', { name: 'Digit 2 of 6' })
+      ).toHaveFocus();


Nice tests 😎

LZoog · 2024-05-01T15:34:11Z

@vpomerleau Just noticed after submitting my review, mobile looks even more squished than before. There's something off with the gap here:

…vidual character inputs Because: * New designs for reset password with code include code inputs with individual input boxes This commit: * Create a TotpInputGroup component that contains individual input boxes for each character, with associated methods for handling input, keyboard navigation (including arrow keys, backspace, delete), pasting content into the boxes * Add inline error feedback below new code input component, instead of tooltip * Create a FormVerifyTotp component that includes the new TotpInputGroup (configurable for 6 or 8 digit codes) and submission button, with pre-submission validation that code contains the expected number of digits and no other characters * Add l10n and storybooks Closes #FXA-7888

vpomerleau · 2024-05-01T22:48:17Z

@vpomerleau Just noticed after submitting my review, mobile looks even more squished than before. There's something off with the gap

Thanks for catching this! I added a min width to the input boxes.

vpomerleau force-pushed the FXA-7888 branch 2 times, most recently from d9fdbae to ab094b9 Compare April 22, 2024 23:29

vpomerleau commented Apr 22, 2024

View reviewed changes

vpomerleau requested review from LZoog and chenba April 22, 2024 23:46

LZoog reviewed Apr 23, 2024

View reviewed changes

pdehaan reviewed Apr 23, 2024

View reviewed changes

packages/fxa-settings/src/components/FormVerifyTotp/en.ftl Outdated Show resolved Hide resolved

packages/fxa-settings/src/components/TotpInputGroup/index.tsx Outdated Show resolved Hide resolved

vpomerleau force-pushed the FXA-7888 branch from 367949b to 91d13a2 Compare April 26, 2024 20:12

pdehaan reviewed Apr 27, 2024

View reviewed changes

packages/fxa-settings/src/components/FormVerifyTotp/index.tsx Outdated Show resolved Hide resolved

packages/fxa-settings/src/components/TotpInputGroup/index.tsx Outdated Show resolved Hide resolved

packages/fxa-settings/src/components/TotpInputGroup/index.tsx Outdated Show resolved Hide resolved

vpomerleau changed the title ~~DRAFT feat(fxa-settings): Add input component with separated characters~~ feat(fxa-settings): Add input component with separated characters Apr 30, 2024

vpomerleau marked this pull request as ready for review April 30, 2024 06:10

vpomerleau requested review from a team as code owners April 30, 2024 06:10

flodolo approved these changes Apr 30, 2024

View reviewed changes

vpomerleau requested review from LZoog and removed request for chenba April 30, 2024 16:21

LZoog approved these changes May 1, 2024

View reviewed changes

vpomerleau force-pushed the FXA-7888 branch from 3de4924 to 399b18d Compare May 1, 2024 22:46

vpomerleau merged commit cd8c5e3 into main May 1, 2024
17 of 18 checks passed

vpomerleau deleted the FXA-7888 branch May 1, 2024 23:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(fxa-settings): Add input component with separated characters #16741

feat(fxa-settings): Add input component with separated characters #16741

vpomerleau commented Apr 18, 2024 •

edited

Loading

vpomerleau Apr 22, 2024

bcolsson Apr 23, 2024

LZoog left a comment

LZoog Apr 23, 2024

LZoog Apr 23, 2024

vpomerleau Apr 23, 2024

pdehaan Apr 29, 2024

LZoog Apr 23, 2024

LZoog Apr 23, 2024

vpomerleau Apr 23, 2024

LZoog Apr 24, 2024

LZoog commented Apr 23, 2024 •

edited

Loading

LZoog left a comment

LZoog May 1, 2024

LZoog May 1, 2024

LZoog May 1, 2024

vpomerleau May 1, 2024

LZoog May 1, 2024

LZoog commented May 1, 2024

vpomerleau commented May 1, 2024

		@@ -0,0 +1,5 @@
		## FormVerifyTotp

		# When focused on the button, screen reader will read the action and entire number that will be submitted

feat(fxa-settings): Add input component with separated characters #16741

feat(fxa-settings): Add input component with separated characters #16741

Conversation

vpomerleau commented Apr 18, 2024 • edited Loading

Because

This pull request

Issue that this pull request solves

Checklist

Screenshots (Optional)

Other information (Optional)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LZoog left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LZoog commented Apr 23, 2024 • edited Loading

LZoog left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LZoog commented May 1, 2024

vpomerleau commented May 1, 2024

vpomerleau commented Apr 18, 2024 •

edited

Loading

LZoog commented Apr 23, 2024 •

edited

Loading