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.

Sign up for GitHub

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

Jump to bottom

fix(Wizard): added prop to focus content on next/back #10285

Merged
merged 6 commits into from
Apr 29, 2024
+97 −40
Merged

fix(Wizard): added prop to focus content on next/back #10285

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
ae63285
fix(Wizard): added prop to focus content on next/back
thatblindgeye Apr 17, 2024
ae016cd
Added new example
thatblindgeye Apr 17, 2024
386180b
Removed beta flag on context props
thatblindgeye Apr 17, 2024
5b5a690
Removed aria-live attr
thatblindgeye Apr 19, 2024
bf7318d
Updated prop name
thatblindgeye Apr 26, 2024
a2061ad
Updated leftover instnaces of ...onNextOrBack prop name
thatblindgeye Apr 29, 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
15 changes: 15 additions & 0 deletions packages/react-core/src/components/Wizard/Wizard.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,10 @@ export interface WizardProps extends React.HTMLProps<HTMLDivElement> {
onSave?: (event: React.MouseEvent<HTMLButtonElement>) => void | Promise<void>;
/** Callback function to close the wizard */
onClose?: (event: React.MouseEvent<HTMLButtonElement>) => void;
/** @beta Flag indicating whether the wizard content should be focused after the onNext or onBack callbacks
* are called.
*/
shouldFocusContent?: boolean;
}

export const Wizard = ({
Expand All @@ -72,11 +76,13 @@ export const Wizard = ({
onStepChange,
onSave,
onClose,
shouldFocusContent = false,
...wrapperProps
}: WizardProps) => {
const [activeStepIndex, setActiveStepIndex] = React.useState(startIndex);
const initialSteps = buildSteps(children);
const firstStepRef = React.useRef(initialSteps[startIndex - 1]);
const wrapperRef = React.useRef(null);

// When the startIndex maps to a parent step, focus on the first sub-step
React.useEffect(() => {
Expand All @@ -85,6 +91,11 @@ export const Wizard = ({
}
}, [startIndex]);

const focusMainContentElement = () =>
setTimeout(() => {
wrapperRef?.current?.focus && wrapperRef.current.focus();
}, 0);
Comment on lines +94 to +97
Copy link
Contributor Author

@thatblindgeye thatblindgeye Apr 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Preface by saying I don't exactly love this 😆

So something I think we should consider is adding a new wrapper element to the actual Wizard step content. Right now we have a structure of:

< .pf-v5-c-wizard__inner-wrap >
  < .pf-v5-c-wizard__nav >
  < .pf-v5-c-wizard__main >
    < .pf-v5-c-wizard__main-body >
  < ...several empty divs with display: none for steps taht aren't rendered >

(Note that Core does not have a bunch of empty divs with display none for steps whose content is not currently rendered, so maybe the intent was that __main element should be static and __main-body dynamically renders step content? Curious as to whether we need these empty div's rendered in React at all.)

When we could either add a wrapper element around the .pf-v5-c-wizard__main and the display: none elements, or have pf-v5-c-wizard__main always rendered and whatever step content just gets placed in there (so pf-v5-c-wizard__main-body and the empty div elements).

Right now we allow preventing that .pf-v5-c-wizard__main from being rendered which will be an issue for this focus management. Having it always rendered and placing all the step contents in there, rather than each step content dynamically updating from an empty div to a div with class .pf-v5-c-wizard__main would help with this focus management (shouldn't need to wait for the __main element to render before placing focus since it'd always be rendered) as well as being able to set the aria-live on it instead of the pf-v5-c-wizard__inner-wrap element (which doing this, any updates to nav content will be announced which may or may not be wanted/beneficial).

Copy link
Contributor

Choose a reason for hiding this comment

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

Definitely curious about what scenario we could have that wouldn't include a main. If we went the aria-live route this would be necessary. However, maybe it's ok if we avoid using aria-live for now. I typically see live regions used for things like updates/status, alerts, logs, etc. Like this article talks about, live regions typically aren't used for interactive content (and I imagine we'd expect wizard content to be interactive often). Based on our discussion, I agree that it probably makes sense to omit aria-live for now and rely on the announcement of the shifted focus.

Copy link
Contributor

Choose a reason for hiding this comment

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

cc @mcoker

Copy link
Contributor

Choose a reason for hiding this comment

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

(Note that Core does not have a bunch of empty divs with display none for steps whose content is not currently rendered, so maybe the intent was that __main element should be static and __main-body dynamically renders step content? Curious as to whether we need these empty div's rendered in React at all.)

FWIW the wizard is built very similarly to a page, so __main was intended to mimic the page main container. In both cases, it's more of a static, structural element of the layout, so I would expect content within it (__main-body elements) to re-render, or possibly attributes and stuff on __main to change, but not the whole element to re-render... though I suppose that would be fine as long as it didn't cause layout shifts or anything?


const goToNextStep = (event: React.MouseEvent<HTMLButtonElement>, steps: WizardStepType[] = initialSteps) => {
const newStep = steps.find((step) => step.index > activeStepIndex && isStepEnabled(steps, step));

Expand All @@ -94,6 +105,7 @@ export const Wizard = ({

setActiveStepIndex(newStep?.index);
onStepChange?.(event, newStep, steps[activeStepIndex - 1], WizardStepChangeScope.Next);
shouldFocusContent && focusMainContentElement();
};

const goToPrevStep = (event: React.MouseEvent<HTMLButtonElement>, steps: WizardStepType[] = initialSteps) => {
Expand All @@ -103,6 +115,7 @@ export const Wizard = ({

setActiveStepIndex(newStep?.index);
onStepChange?.(event, newStep, steps[activeStepIndex - 1], WizardStepChangeScope.Back);
shouldFocusContent && focusMainContentElement();
};

const goToStepByIndex = (
Expand Down Expand Up @@ -157,6 +170,8 @@ export const Wizard = ({
goToStepById={goToStepById}
goToStepByName={goToStepByName}
goToStepByIndex={goToStepByIndex}
shouldFocusContent={shouldFocusContent}
mainWrapperRef={wrapperRef}
>
<div
className={css(styles.wizard, className)}
Expand Down
18 changes: 9 additions & 9 deletions packages/react-core/src/components/Wizard/WizardBody.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,15 +36,14 @@ export const WizardBody = ({
}: WizardBodyProps) => {
const [hasScrollbar, setHasScrollbar] = React.useState(false);
const [previousWidth, setPreviousWidth] = React.useState<number | undefined>(undefined);
const wrapperRef = React.useRef(null);
const WrapperComponent = component;
const { activeStep } = React.useContext(WizardContext);
const { activeStep, shouldFocusContent, mainWrapperRef } = React.useContext(WizardContext);
const defaultAriaLabel = ariaLabel || `${activeStep?.name} content`;

React.useEffect(() => {
const resize = () => {
if (wrapperRef?.current) {
const { offsetWidth, offsetHeight, scrollHeight } = wrapperRef.current;
if (mainWrapperRef?.current) {
const { offsetWidth, offsetHeight, scrollHeight } = mainWrapperRef.current;

if (previousWidth !== offsetWidth) {
setPreviousWidth(offsetWidth);
Expand All @@ -56,12 +55,12 @@ export const WizardBody = ({
const handleResizeWithDelay = debounce(resize, 250);
let observer = () => {};

if (wrapperRef?.current) {
observer = getResizeObserver(wrapperRef.current, handleResizeWithDelay);
const { offsetHeight, scrollHeight } = wrapperRef.current;
if (mainWrapperRef?.current) {
observer = getResizeObserver(mainWrapperRef.current, handleResizeWithDelay);
const { offsetHeight, scrollHeight } = mainWrapperRef.current;

setHasScrollbar(offsetHeight < scrollHeight);
setPreviousWidth((wrapperRef.current as HTMLElement).offsetWidth);
setPreviousWidth((mainWrapperRef.current as HTMLElement).offsetWidth);
}

return () => {
Expand All @@ -71,7 +70,8 @@ export const WizardBody = ({

return (
<WrapperComponent
ref={wrapperRef}
ref={mainWrapperRef}
{...(shouldFocusContent && { tabIndex: -1 })}
{...(component === 'div' && hasScrollbar && { role: 'region' })}
{...(hasScrollbar && { 'aria-label': defaultAriaLabel, 'aria-labelledby': ariaLabelledBy, tabIndex: 0 })}
className={css(styles.wizardMain)}
Expand Down
16 changes: 14 additions & 2 deletions packages/react-core/src/components/Wizard/WizardContext.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,12 @@ export interface WizardContextProps {
getStep: (stepId: number | string) => WizardStepType;
/** Set step by ID */
setStep: (step: Pick<WizardStepType, 'id'> & Partial<WizardStepType>) => void;
/** Flag indicating whether the wizard content should be focused after the onNext or onBack callbacks
* are called.
*/
shouldFocusContent: boolean;
/** Ref for main wizard content element. */
mainWrapperRef: React.RefObject<HTMLElement>;
}

export const WizardContext = React.createContext({} as WizardContextProps);
Expand All @@ -47,6 +53,8 @@ export interface WizardContextProviderProps {
steps: WizardStepType[],
index: number
): void;
shouldFocusContent: boolean;
mainWrapperRef: React.RefObject<HTMLElement>;
}

export const WizardContextProvider: React.FunctionComponent<WizardContextProviderProps> = ({
Expand All @@ -59,7 +67,9 @@ export const WizardContextProvider: React.FunctionComponent<WizardContextProvide
onClose,
goToStepById,
goToStepByName,
goToStepByIndex
goToStepByIndex,
shouldFocusContent,
mainWrapperRef
}) => {
const [currentSteps, setCurrentSteps] = React.useState<WizardStepType[]>(initialSteps);
const [currentFooter, setCurrentFooter] = React.useState<WizardFooterType>();
Expand Down Expand Up @@ -139,7 +149,9 @@ export const WizardContextProvider: React.FunctionComponent<WizardContextProvide
goToStepByIndex: React.useCallback(
(index: number) => goToStepByIndex(null, steps, index),
[goToStepByIndex, steps]
)
),
shouldFocusContent,
mainWrapperRef
}}
>
{children}
Expand Down
28 changes: 7 additions & 21 deletions packages/react-core/src/components/Wizard/WizardNavItem.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ export const WizardNavItem = ({
content = '',
isCurrent = false,
isDisabled = false,
// eslint-disable-next-line @typescript-eslint/no-unused-vars
isVisited = false,
stepIndex,
onClick,
Expand All @@ -68,23 +69,6 @@ export const WizardNavItem = ({
console.error('WizardNavItem: When using an anchor, please provide an href');
}

const ariaLabel = React.useMemo(() => {
if (status === WizardNavItemStatus.Error || (isVisited && !isCurrent)) {
let label = content.toString();

if (status === WizardNavItemStatus.Error) {
label += `, ${status}`;
}

// No need to signify step is visited if current
if (isVisited && !isCurrent) {
label += ', visited';
}

return label;
}
}, [content, isCurrent, isVisited, status]);

Comment on lines -71 to -87
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Setting this aria-label was kinda interfering/producing extra noise with the aria-live attr added in. We probably shouldn't append "visited" text via aria-label for a Wizard step, and just let users know whether a specific step is the current one or not. If it's vital for users to know what steps they've visited/successfully completed, we could just utilize the hidden text that is used in this PR now.

return (
<li
className={css(
Expand All @@ -110,7 +94,6 @@ export const WizardNavItem = ({
aria-disabled={isDisabled ? true : null}
aria-current={isCurrent && !children ? 'step' : false}
{...(isExpandable && { 'aria-expanded': isExpanded })}
{...(ariaLabel && { 'aria-label': ariaLabel })}
{...ouiaProps}
>
{isExpandable ? (
Expand All @@ -127,9 +110,12 @@ export const WizardNavItem = ({
{content}
{/* TODO, patternfly/patternfly#5142 */}
{status === WizardNavItemStatus.Error && (
<span style={{ marginLeft: globalSpacerSm.var }}>
<ExclamationCircleIcon color={globalDangerColor100.var} />
</span>
<>
<span className="pf-v5-screen-reader">, {status}</span>
<span style={{ marginLeft: globalSpacerSm.var }}>
<ExclamationCircleIcon color={globalDangerColor100.var} />
</span>
</>
)}
</>
)}
Expand Down
8 changes: 4 additions & 4 deletions packages/react-core/src/components/Wizard/__tests__/Wizard.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -411,7 +411,7 @@ test('incrementally shows/hides steps based on the activeStep when isProgressive
await user.click(nextButton);
expect(
screen.getByRole('button', {
name: 'Test step 1, visited'
name: 'Test step 1'
})
).toBeVisible();
expect(
Expand All @@ -429,12 +429,12 @@ test('incrementally shows/hides steps based on the activeStep when isProgressive
await user.click(nextButton);
expect(
screen.getByRole('button', {
name: 'Test step 1, visited'
name: 'Test step 1'
})
).toBeVisible();
expect(
screen.getByRole('button', {
name: 'Test step 2, visited'
name: 'Test step 2'
})
).toBeVisible();
expect(
Expand All @@ -447,7 +447,7 @@ test('incrementally shows/hides steps based on the activeStep when isProgressive
await user.click(backButton);
expect(
screen.getByRole('button', {
name: 'Test step 1, visited'
name: 'Test step 1'
})
).toBeVisible();
expect(
Expand Down
30 changes: 29 additions & 1 deletion packages/react-core/src/components/Wizard/examples/Wizard.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ propComponents:
'WizardContextProps',
'WizardBasicStep',
'WizardParentStep',
'WizardSubStep',
'WizardSubStep'
]
---

Expand Down Expand Up @@ -57,91 +57,119 @@ import layout from '@patternfly/react-styles/css/layouts/Bullseye/bullseye';
### Basic

```ts file="./WizardBasic.tsx"

```

### Focus content on next/back

To focus the main content element of the `Wizard`, pass in the `shouldFocusContent` property. It is recommended that this is passed in so that users can navigate through a `WizardStep` content in order.

If a `WizardStep` is passed a `body={null}` property, you must manually handle focus.

```ts file="./WizardFocusOnNextBack.tsx"

```

### Basic with disabled steps

```ts file="./WizardBasicDisabledSteps.tsx"

```

### Anchors for nav items

```ts file="./WizardWithNavAnchors.tsx"

```

### Incrementally enabled steps

```ts file="./WizardStepVisitRequired.tsx"

```

### Expandable steps

```ts file="./WizardExpandableSteps.tsx"

```

### Progress after submission

```ts file="./WizardWithSubmitProgress.tsx"

```

### Enabled on form validation

```ts file="./WizardEnabledOnFormValidation.tsx"

```

### Validate on button press

```ts file="./WizardValidateOnButtonPress.tsx"

```

### Progressive steps

```ts file="./WizardProgressiveSteps.tsx"

```

### Get current step

```ts file="./WizardGetCurrentStep.tsx"

```

### Within modal

```ts file="./WizardWithinModal.tsx"

```

### Step drawer content

```ts file="./WizardStepDrawerContent.tsx"

```

### Custom navigation

```ts file="./WizardWithCustomNav.tsx"

```

### Header

```ts file="./WizardWithHeader.tsx"

```

### Custom footer

```ts file="./WizardWithCustomFooter.tsx"

```

### Custom navigation item

```ts file="./WizardWithCustomNavItem.tsx"

```

### Toggle step visibility

```ts file="./WizardToggleStepVisibility.tsx"

```

### Step error status

```ts file="./WizardStepErrorStatus.tsx"

```

## Hooks
Expand Down
16 changes: 16 additions & 0 deletions packages/react-core/src/components/Wizard/examples/WizardFocusOnNextBack.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
import React from 'react';
import { Wizard, WizardStep } from '@patternfly/react-core';

export const WizardFocusOnNextBack: React.FunctionComponent = () => (
<Wizard shouldFocusContent title="Wizard that focuses content on next or back click">
<WizardStep name="Step 1" id="wizard-focus-first-step">
Step 1 content
</WizardStep>
<WizardStep name="Step 2" id="wizard-focus-second-step">
Step 2 content
</WizardStep>
<WizardStep name="Review" id="wizard-focus-review-step" footer={{ nextButtonText: 'Finish' }}>
Review step content
</WizardStep>
</Wizard>
);
Loading
Loading