SCC-4384: My Account 2.0 VQA/a11y round 1 #412

7emansell · 2024-12-05T22:04:06Z

Ticket:

JIRA ticket SCC-4384

This PR does the following:

Addresses comments in My Account 2.0 VQA and accessibility review
- Mainly spacing (VQA) and focus behavior (a11y)

Question/request for help:

On mobile when you click edit for Home library/ Notification preference/Password, the fields jump up slightly like they're losing padding/margin or height is just not the same between the two states. I would love a fresh pair of eyes on this since I'm having trouble tracking down why this is happening

How has this been tested?

Accessibility concerns or updates

Adds focus management to all form fields

Checklist:

I updated the CHANGELOG with the appropriate information and JIRA ticket number (if applicable).
I have added relevant accessibility documentation for this pull request.
All new and existing tests passed.

vercel · 2024-12-05T22:04:09Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
research-catalog	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Dec 9, 2024 7:51pm

7emansell · 2024-12-05T22:06:10Z

src/components/MyAccount/Settings/PasswordForm.tsx

                buttonId="edit-password-button"
                onClick={() => {
                  setIsEditing(true)
                  setEditingField("password")
+                  setTimeout(() => {


@charmingduchess:

Does this shift the focus to the text input? If so, there actually should be two different refs, just to make it explicit what is being focused on.

@EdwinGuzman this is what we discussed– basically now I'm using 1 ref for everything (except in SettingsInputForm, which uses an array of refs to match the array of inputs, and can't really be combined with a button ref). This makes it confusing to predict where the focus will go just from reading the code, but it does work, so I'm torn.

For readability and debugging where focus goes, it seems like having two refs is the approach to follow. When reading useRef<HTMLButtonElement | TextInputRefType | null>, I was thinking that ref is doing too many things. I think we discussed one ref where possible and updating the ref.current value but that's not the case here.

To be clear, what other problems are there?

I think the issue is just conflicting directives. But I think me and Edwin agree this is a case where we can opt for readability.

All refs now split up for readability again 👍

7emansell · 2024-12-05T22:07:36Z

src/components/MyAccount/Settings/SettingsInputForm.tsx

+    }
+  }
+
+  useEffect(() => {


@charmingduchess:

With three text inputs open for the phone settings, this focus is triggering on every keystroke input. ie Typing a single character into input 2 jumps the focus to input 3. I think you want an empty array to only run on the first render. I think this will make typescript unhappy but that's ok.

updated it to tempInputs.length which I think is right because then it updates only when a new input is added/removed

The linter is saying that focusLastInput should also go in here. I'm not sure that's right but if it should go here, then the function should be wrapped in a useCallback.

hmm yes I think the most idiomatic way of doing this is to make the focusLastInput a useCallback with a dependency of tempInputs.length. I'm not too concerned about it. I think it is good to see that the useEffect is going to run when that value updates, instead of having to check a useCallback for a dependency array.

7emansell · 2024-12-05T22:08:40Z

src/components/MyAccount/Settings/SettingsInputForm.tsx

@@ -183,10 +211,15 @@ const SettingsInputForm = ({
                    sx={{
                      width: { base: "87%", md: "300px" },
                    }}
+                    ref={(el) => (inputRefs.current[index] = el)}


@charmingduchess:

Not sure if you came across this ("How to manage a list of refs using a ref callback"), but your solution is react.dev approved! Looking at their solution, though, I think it might be important to remember to delete refs from the array once the input is removed from the dom.

I tried adding 5 inputs, then deleting one from the middle, and the count on the array of input refs actually went up to 6. It doesn't seem to cause any problems, but I think to avoid bugs down the line, the inputRefs array should match the inputs that are actually present on the dom.

Actually just encountered a bug! after opting to stay logged in from the timed logout modal, the focus is lost and the last input is null, which leads to a loss of focus. I think this can be avoided by removing refs as the inputs are removed.

Updated to now remove ref (L104). Discussed offline but the issue with modal focus isn't going to be addressed here

7emansell · 2024-12-05T22:10:46Z

Got a new ticket number so closed the old PR and copied Vera's comments over

EdwinGuzman

As far as I can tell and reviewing the QA, this does fix all the issues. The 1vs2 ref issue could be addressed later if you want to move this back to vqa/a11y review.

After seeing the code, having two refs, similar to SettingsInputForm, is a good tradeoff for readability.

EdwinGuzman · 2024-12-09T14:39:31Z

src/components/MyAccount/Settings/EmailForm.test.tsx

@@ -60,12 +62,37 @@ describe("email form", () => {
    expect(screen.queryByText(/edit/)).not.toBeInTheDocument()
  })

+  it("manages focus", async () => {


EdwinGuzman · 2024-12-09T14:51:49Z

src/components/MyAccount/Settings/SaveCancelButtons.tsx

@@ -4,7 +4,7 @@ type SaveCancelButtonProps = {
  isDisabled?: boolean
  onCancel: () => void
  onSave: () => void
-  inputType?: "phones" | "emails"
+  inputType?: string


Not related to this change, but I was wondering why this was updated to be a generic string. This is used in multiple locations so it makes sense to update this. I think you should list down all possible values and not make it optional.

In some forms, PasswordForm for example, not passing the inputType makes it render undefined in the ids:

EdwinGuzman · 2024-12-09T15:16:07Z

src/components/MyAccount/Settings/PasswordForm.tsx

                buttonId="edit-password-button"
                onClick={() => {
                  setIsEditing(true)
                  setEditingField("password")
+                  setTimeout(() => {


For readability and debugging where focus goes, it seems like having two refs is the approach to follow. When reading useRef<HTMLButtonElement | TextInputRefType | null>, I was thinking that ref is doing too many things. I think we discussed one ref where possible and updating the ref.current value but that's not the case here.

EdwinGuzman · 2024-12-09T15:16:26Z

src/components/MyAccount/Settings/PasswordForm.tsx

                buttonId="edit-password-button"
                onClick={() => {
                  setIsEditing(true)
                  setEditingField("password")
+                  setTimeout(() => {


To be clear, what other problems are there?

EdwinGuzman · 2024-12-09T15:18:06Z

src/components/MyAccount/Settings/UsernameForm.tsx

            {usernameInSierra}
          </Text>
          <EditButton
+            ref={focusRef as React.Ref<HTMLButtonElement>}


From the comment in PasswordForm regarding breaking up the ref into two refs, doing so will allow this as React.Ref... to be removed.

EdwinGuzman · 2024-12-09T15:20:58Z

src/components/MyAccount/Settings/SettingsInputForm.tsx

+    }
+  }
+
+  useEffect(() => {


The linter is saying that focusLastInput should also go in here. I'm not sure that's right but if it should go here, then the function should be wrapped in a useCallback.

EdwinGuzman · 2024-12-09T15:26:08Z

src/components/MyAccount/Settings/SettingsInputForm.tsx

@@ -195,6 +228,7 @@ const SettingsInputForm = ({
                    isClearable
                    isClearableCallback={() => handleClearableCallback(index)}
                  />
+                  {index == 0 && <div style={{ width: "57px" }}> </div>}


I'm not a fan of these magic numbers (72px on line 252). I don't have any good suggestions right now but I'll take a closer look.

Same– there are a lot of random fixed values in the designs which prevent using spacing tokens the way I'd like to

Right, I said I'll take another pass but I don't want to hold up pushing this out for another review. Okay to add comments post merge?

Sure– currently, need to fix the focus issue that occurs when you try to clear the textbox for any of the inputs, so I won't be merging for a bit

EdwinGuzman · 2024-12-09T15:28:02Z

src/components/MyAccount/Settings/SettingsInputForm.tsx

+  const focusLastInput = () => {
+    const lastIndex = tempInputs.length - 1
+    if (inputRefs.current[lastIndex]) {
+      inputRefs.current[lastIndex]?.focus()


I was going to ask if it was possible to use the Map function like the code example in the React docs resource. It'll work but I imagine this part here would make it difficult to keep track of the previous element to focus?

With an Array we get the order maintained for free, which is crucial here. FWIW, the docs do say "This lets you maintain your own array or a Map".

charmingduchess

Looks good!

EdwinGuzman

This addresses all the a11y/vqa issues. The clear X button is an issue that we'll debug on the DS so for now, it won't be included in this update.

7emansell added 13 commits December 3, 2024 12:36

VQA fixes

238e363

Start of accessibility focus work

3f41f55

Labelling corrections

5cd827d

Adding a bunch of refs

c7c18f2

Associating delete message with button

7d58c93

Updating tests with new explicitly numbered labels

7be3cf9

And username test

4d6a400

Reducing refs and styling corrections

612e027

More alignment

54e1d81

More minor tweaking

182416b

Fully focus

3221114

Adding tests for focus

1820027

Correcting inputRefs behavior

46c071b

7emansell requested review from charmingduchess and EdwinGuzman December 5, 2024 22:04

7emansell commented Dec 5, 2024

View reviewed changes

EdwinGuzman reviewed Dec 9, 2024

View reviewed changes

7emansell added 2 commits December 9, 2024 11:27

Explicitly typing the string for save cancel buttons

905c5da

Bringing back multiple refs, currently an issue with clearableCallback

b44aff5

vercel bot deployed to Preview December 9, 2024 17:21 View deployment

charmingduchess approved these changes Dec 9, 2024

View reviewed changes

Testing focus

664cf93

vercel bot deployed to Preview December 9, 2024 18:49 View deployment

Removing isClearable from input fields

90f7041

vercel bot deployed to Preview December 9, 2024 19:51 View deployment

EdwinGuzman approved these changes Dec 9, 2024

View reviewed changes

7emansell merged commit 9f1e092 into main Dec 9, 2024
3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SCC-4384: My Account 2.0 VQA/a11y round 1 #412

SCC-4384: My Account 2.0 VQA/a11y round 1 #412

7emansell commented Dec 5, 2024

vercel bot commented Dec 5, 2024 •

edited

Loading

7emansell Dec 5, 2024 •

edited

Loading

7emansell Dec 5, 2024

EdwinGuzman Dec 9, 2024

EdwinGuzman Dec 9, 2024

charmingduchess Dec 9, 2024

7emansell Dec 9, 2024

7emansell Dec 5, 2024

7emansell Dec 5, 2024

EdwinGuzman Dec 9, 2024

charmingduchess Dec 9, 2024 •

edited

Loading

7emansell Dec 5, 2024 •

edited

Loading

7emansell Dec 5, 2024

7emansell commented Dec 5, 2024

EdwinGuzman left a comment

EdwinGuzman Dec 9, 2024

EdwinGuzman Dec 9, 2024

EdwinGuzman Dec 9, 2024

EdwinGuzman Dec 9, 2024

EdwinGuzman Dec 9, 2024

EdwinGuzman Dec 9, 2024

EdwinGuzman Dec 9, 2024

7emansell Dec 9, 2024

EdwinGuzman Dec 9, 2024

7emansell Dec 9, 2024

EdwinGuzman Dec 9, 2024

charmingduchess Dec 9, 2024

charmingduchess left a comment

EdwinGuzman left a comment

SCC-4384: My Account 2.0 VQA/a11y round 1 #412

SCC-4384: My Account 2.0 VQA/a11y round 1 #412

Conversation

7emansell commented Dec 5, 2024

Ticket:

This PR does the following:

Question/request for help:

How has this been tested?

Accessibility concerns or updates

Checklist:

vercel bot commented Dec 5, 2024 • edited Loading

7emansell Dec 5, 2024 • edited Loading

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

charmingduchess Dec 9, 2024 • edited Loading

Choose a reason for hiding this comment

7emansell Dec 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

7emansell commented Dec 5, 2024

EdwinGuzman 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

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

charmingduchess left a comment

Choose a reason for hiding this comment

EdwinGuzman left a comment

Choose a reason for hiding this comment

vercel bot commented Dec 5, 2024 •

edited

Loading

7emansell Dec 5, 2024 •

edited

Loading

charmingduchess Dec 9, 2024 •

edited

Loading

7emansell Dec 5, 2024 •

edited

Loading