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

Adds FormControl's "Auto-Wiring" to SelectPanel v2 Component #4389

Merged
Merged
Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
0f4cec5
autowires SelectPanelv2 to FormControl
JeffreyKozik Mar 13, 2024
06b048a
added changeset
JeffreyKozik Mar 13, 2024
5e46bce
Update afraid-beds-lick.md
siddharthkp Mar 14, 2024
d1df0dd
Button is aria-labelledby by 2 different components now
JeffreyKozik Mar 19, 2024
3d69e09
using querySelector instead of VisuallyHidden approach
JeffreyKozik Mar 19, 2024
969c97c
Merge branch 'main' into jeffreykozik/autolinking_selectpanelv2_to_fo…
JeffreyKozik Mar 19, 2024
4d7b5d7
Added visually hidden punctuation
JeffreyKozik Mar 20, 2024
616ad2b
removed selectPanelButtonId
JeffreyKozik Mar 22, 2024
d26d79a
hiding visuallyhidden text from screen readers as well
JeffreyKozik Mar 22, 2024
d6b08a2
use ariaLabel instead of ariaLabelledby
JeffreyKozik Mar 23, 2024
6ea27c7
added a 2nd test for complex button case
JeffreyKozik Mar 23, 2024
006a61b
updated tests
JeffreyKozik Mar 23, 2024
e8aa873
Merge branch 'main' into jeffreykozik/autolinking_selectpanelv2_to_fo…
JeffreyKozik Mar 26, 2024
672aaab
using aria-labelledby to reference itself, updated tests
JeffreyKozik Mar 26, 2024
c3e1594
flipped order within aria-label
JeffreyKozik Mar 26, 2024
71ea3b5
updated conditional, added negative test
JeffreyKozik Mar 26, 2024
a3db265
Merge branch 'main' into jeffreykozik/autolinking_selectpanelv2_to_fo…
JeffreyKozik Apr 17, 2024
e6d93e1
removed aria-labelledby
JeffreyKozik Apr 17, 2024
38d9748
Merge branch 'main' into jeffreykozik/autolinking_selectpanelv2_to_fo…
JeffreyKozik Apr 19, 2024
856e957
test
JeffreyKozik Apr 19, 2024
eacfa7c
fix
JeffreyKozik Apr 19, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/afraid-beds-lick.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@primer/react': minor
---

experimental/SelectPanel + FormControl: Automatically wires SelectPanel v2 to the accessibility and validation provided by the FormControl component it's nested within
Original file line number Diff line number Diff line change
Expand Up @@ -847,6 +847,38 @@ export const NestedSelection = () => {
)
}

export const WithinForm = () => {
const [selectedTag, setSelectedTag] = React.useState<string>()

const onSubmit = () => {
if (!selectedTag) return
data.ref = selectedTag // pretending to persist changes
}

const itemsToShow = data.tags

return (
<>
<h1>Within Form</h1>

<FormControl>
<FormControl.Label>SelectPanel within FormControl</FormControl.Label>
<SelectPanel title="Choose a tag" selectionVariant="instant" onSubmit={onSubmit}>
<SelectPanel.Button leadingVisual={TagIcon}>{selectedTag || 'Choose a tag'}</SelectPanel.Button>

<ActionList>
{itemsToShow.map(tag => (
<ActionList.Item key={tag.id} onSelect={() => setSelectedTag(tag.id)} selected={selectedTag === tag.id}>
{tag.name}
</ActionList.Item>
))}
</ActionList>
</SelectPanel>
</FormControl>
</>
)
}

export const CreateNewRow = () => {
const initialSelectedLabels = data.issue.labelIds // mock initial state: has selected labels
const [selectedLabelIds, setSelectedLabelIds] = React.useState<string[]>(initialSelectedLabels)
Expand Down
48 changes: 47 additions & 1 deletion packages/react/src/drafts/SelectPanel2/SelectPanel.test.tsx
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import React from 'react'
import {ThemeProvider, ActionList} from '../../'
import {ThemeProvider, ActionList, FormControl} from '../../'
import type {RenderResult} from '@testing-library/react'
import {render} from '@testing-library/react'
import type {UserEvent} from '@testing-library/user-event'
Expand Down Expand Up @@ -59,6 +59,36 @@ const Fixture = ({onSubmit, onCancel}: Pick<SelectPanelProps, 'onSubmit' | 'onCa
)
}

function SelectPanelWithinForm(): JSX.Element {
return (
<FormControl>
<FormControl.Label>Select Panel Label</FormControl.Label>
<Fixture />
</FormControl>
)
}

function SelectPanelWithComplexButtonWithinForm(): JSX.Element {
return (
<FormControl>
<FormControl.Label>Select Panel Label</FormControl.Label>
<SelectPanel title="Select labels" onSubmit={() => {}} onCancel={() => {}}>
<SelectPanel.Button>
<div>Assign label</div>
</SelectPanel.Button>

<ActionList>
<ActionList.Item key={'Item'} onSelect={() => {}} selected={true}>
Item
<ActionList.Description variant="block">Item description</ActionList.Description>
</ActionList.Item>
</ActionList>
<SelectPanel.Footer />
</SelectPanel>
</FormControl>
)
}

describe('SelectPanel', () => {
it('renders Button by default', async () => {
const container = render(<Fixture />)
Expand Down Expand Up @@ -148,4 +178,20 @@ describe('SelectPanel', () => {
expect(mockOnCancel).toHaveBeenCalledTimes(1)
expect(mockOnSubmit).toHaveBeenCalledTimes(0)
})

it('SelectPanel within FormControl should be labelled by FormControl.Label', async () => {
const component = render(<SelectPanelWithinForm />)
const buttonByRole = component.getByRole('button')
expect(buttonByRole).toBeVisible()
expect(buttonByRole).toHaveAttribute('aria-label', 'Select Panel Label, Assign label')
expect(buttonByRole).toHaveAttribute('aria-labelledby', buttonByRole.id)
})

it('SelectPanel with complex button within FormControl should be labelled by FormControl.Label', async () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a 2nd test to ensure the aria-label is still correct even if a button's contents isn't solely text (ie it includes HTML in it)

const component = render(<SelectPanelWithComplexButtonWithinForm />)
const buttonByRole = component.getByRole('button')
expect(buttonByRole).toBeVisible()
expect(buttonByRole).toHaveAttribute('aria-label', 'Select Panel Label, Assign label')
expect(buttonByRole).toHaveAttribute('aria-labelledby', buttonByRole.id)
})
})
39 changes: 36 additions & 3 deletions packages/react/src/drafts/SelectPanel2/SelectPanel.tsx
Original file line number Diff line number Diff line change
@@ -1,8 +1,21 @@
import React from 'react'
import React, {useEffect, useState, type MutableRefObject} from 'react'
import {SearchIcon, XCircleFillIcon, XIcon, FilterRemoveIcon, AlertIcon, ArrowLeftIcon} from '@primer/octicons-react'

import type {ButtonProps, TextInputProps, ActionListProps, LinkProps, CheckboxProps} from '../../index'
import {Button, IconButton, Heading, Box, Tooltip, TextInput, Spinner, Text, Octicon, Link, Checkbox} from '../../index'
import {
Button,
IconButton,
Heading,
Box,
Tooltip,
TextInput,
Spinner,
Text,
Octicon,
Link,
Checkbox,
useFormControlForwardedProps,
} from '../../index'
Copy link
Contributor Author

@JeffreyKozik JeffreyKozik Mar 13, 2024

Choose a reason for hiding this comment

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

Using the hook recently created by @iansan5653 in this PR: #3632. This hook is also being used in the InlineAutocomplete draft component:

const inputProps = useFormControlForwardedProps(externalInputProps)

import {ActionListContainerContext} from '../../ActionList/ActionListContainerContext'
import {useSlots} from '../../hooks/useSlots'
import {useProvidedRefOrCreate, useId, useAnchoredPosition} from '../../hooks'
Expand Down Expand Up @@ -337,7 +350,27 @@ const Panel: React.FC<SelectPanelProps> = ({
}

const SelectPanelButton = React.forwardRef<HTMLButtonElement, ButtonProps>((props, anchorRef) => {
return <Button ref={anchorRef} {...props} />
const inputProps = useFormControlForwardedProps(props)
const [labelText, setLabelText] = useState('')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

useEffect(() => {
const label = document.querySelector(`[for='${inputProps.id}']`)
if (label?.textContent) {
setLabelText(label.textContent)
}
}, [inputProps.id])

if (labelText && inputProps.children) {
return (
<Button
ref={anchorRef}
aria-label={`${labelText}, ${(anchorRef as MutableRefObject<HTMLButtonElement>).current.textContent}`}
aria-labelledby={inputProps.id}
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 think this implementation is cleaner overall since it doesn't require having a hidden element just to add punctuation and seems more similar to what we do elsewhere in the Primer codebase.

Copy link
Contributor

Choose a reason for hiding this comment

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

Could we flip this, so that the button text content is first?

Suggested change
aria-label={`${labelText}, ${(anchorRef as MutableRefObject<HTMLButtonElement>).current.textContent}`}
aria-label={`${(anchorRef as MutableRefObject<HTMLButtonElement>).current.textContent}, ${labelText}`}

This way, the contents of the button is announced first, and the label second. We'd want this mainly because the most important content is the button's text content, whereas the label itself would be secondary to that, after it has been read.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure I can go ahead and flip that 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changes are pushed

{...inputProps}
/>
)
} else {
return <Button ref={anchorRef} {...props} />
}
})
Copy link
Contributor Author

@JeffreyKozik JeffreyKozik Mar 20, 2024

Choose a reason for hiding this comment

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

This achieves the intended accessible name:
image

The other implementation I tried was

return (
    <>
      {inputProps.id && (
        <VisuallyHidden id={`${inputProps.id}--select-panel-button-children`}>{inputProps.children}</VisuallyHidden>
      )}
      <Button
        ref={anchorRef}
        {...inputProps}
        aria-labelledby={`${inputProps.id} ${inputProps.id}--select-panel-button-children`}
      />
    </>
  )

However I wasn't thrilled about using VisuallyHidden because it can have unintended consequences, although it is used in a similar situation (but not identical) @siddharthkp referenced so it might be okay.

The solution I ended up implementing is similar to what @TylerJDev suggested:

<label id="label1">SelectPanel within FormControl</label>
<button id="label2" aria-labelledby="label1 label2">Choose a tag</button>

The only wrinkle is that I had to give the Button a new id (so it's no longer linked to the for tag in the label).

Is there a more elegant solution here, perhaps involving forwarding React refs? Or is this solution satisfactory?

Copy link
Member

Choose a reason for hiding this comment

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

The only wrinkle is that I had to give the Button a new id (so it's no longer linked to the for tag in the label).

Yeah, that's a problem 🤔

Is there a more elegant solution here?

I need to check if we can reuse the id that's generated by the FormControl for this purpose

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When I tried reusing the id that's generated by the FormControl for this purpose it ended up giving it the name: "SelectPanel within FormControl SelectPanel within FormControl" since the label overrides the inner text of the button when linking with the for attribute

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I've been busy with first responder duties. I need to check if this is safe/fine.

@TylerJDev can you validate this for me, please?

Copy link
Contributor

Choose a reason for hiding this comment

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

When I tried reusing the id that's generated by the FormControl for this purpose it ended up giving it the name: "SelectPanel within FormControl SelectPanel within FormControl"

I'm curious about where it gave it the name twice? Could we not utilize the ID FormControl provides, instead of using selectPanelButtonId? I see the potential issue where the for could duplicate the label text, but when I tested, it stated both properly 🤔

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'll take a video and sent it over. When I was doing it it was duplicating the text

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh interesting.

When it's this order it works

aria-labelledby={`${labelId} ${inputProps.id}`}

When it's this order it duplicates "SelectPanel within FormControl`

aria-labelledby={`${inputProps.id} ${labelId}`}
Screen.Recording.2024-03-22.at.1.11.56.PM.mov

Copy link
Contributor Author

@JeffreyKozik JeffreyKozik Mar 22, 2024

Choose a reason for hiding this comment

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

Alright I went ahead and removed selectPanelButtonId in favor of inputProps.id to avoid any a11y issues.


const SelectPanelHeader: React.FC<React.PropsWithChildren & {onBack?: () => void}> = ({children, onBack, ...props}) => {
Expand Down
Loading