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

thatblindgeye · 2024-04-17T17:41:04Z

What: Closes #10249

Some comments on changes below

Additional issues:

thatblindgeye · 2024-04-17T17:42:52Z

packages/react-core/src/components/Wizard/WizardNavItem.tsx

-  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]);
-


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.

patternfly-build · 2024-04-17T17:46:21Z

Preview: https://patternfly-react-pr-10285.surge.sh

A11y report: https://patternfly-react-pr-10285-a11y.surge.sh

thatblindgeye · 2024-04-17T17:48:36Z

packages/react-core/src/components/Wizard/Wizard.tsx

+  const focusMainContentElement = () =>
+    setTimeout(() => {
+      wrapperRef?.current?.focus && wrapperRef.current.focus();
+    }, 0);


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).

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.

(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?

jessiehuff · 2024-04-19T16:02:21Z

packages/react-core/src/components/Wizard/Wizard.tsx

+  /** @beta Flag indicating whether the wizard content should be focused after the onNext or onBack callbacks
+   * are called.
+   */
+  shouldFocusContentOnNextOrBack?: boolean;


A little nit picky, but I find this name a little confusing. Is there something that we could use that would be a little clearer?

I'd definitely be in favor of updating this prop name, I just couldn't really think of a better alternative at the time 😆 shouldFocusContentOnStepChange wouldn't be totally accurate since technically clicking a nav item changes the step, but we don't move focus on that. shouldFocusContent would be shorter, but probably less clear? shouldFocusContentOnFooterAction?

If it matters the prop is beta, so we should be able to tweak the naming if needed.

@tlabaj do you have any suggestions for a potential prop name here?

I would vote for the shorter name. I think shouldFocusContent will work as long as description is clear.

jessiehuff · 2024-04-19T16:05:34Z

packages/react-core/src/components/Wizard/Wizard.tsx

+  const focusMainContentElement = () =>
+    setTimeout(() => {
+      wrapperRef?.current?.focus && wrapperRef.current.focus();
+    }, 0);


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.

jessiehuff

LGTM! Still not sure on naming, but that is not my forte. 😆

tlabaj

Other than changing the prop name this looks good to me

patternfly-build · 2024-04-29T16:14:56Z

Your changes have been released in:

@patternfly/[email protected]
@patternfly/[email protected]
@patternfly/[email protected]
@patternfly/[email protected]
[email protected]
@patternfly/[email protected]
@patternfly/[email protected]

Thanks for your contribution! 🎉

thatblindgeye requested a review from jessiehuff April 17, 2024 17:41

thatblindgeye commented Apr 17, 2024

View reviewed changes

jessiehuff reviewed Apr 19, 2024

View reviewed changes

thatblindgeye force-pushed the iss10249_wizardFocus branch from d5887dd to 4f591c7 Compare April 19, 2024 19:08

jessiehuff approved these changes Apr 19, 2024

View reviewed changes

thatblindgeye requested a review from tlabaj April 22, 2024 12:58

tlabaj reviewed Apr 26, 2024

View reviewed changes

thatblindgeye force-pushed the iss10249_wizardFocus branch from 4f591c7 to 3133abd Compare April 26, 2024 19:43

thatblindgeye added 6 commits April 29, 2024 09:08

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

ae63285

Added new example

ae016cd

Removed beta flag on context props

386180b

Removed aria-live attr

5b5a690

Updated prop name

bf7318d

Updated leftover instnaces of ...onNextOrBack prop name

a2061ad

thatblindgeye force-pushed the iss10249_wizardFocus branch from 3133abd to a2061ad Compare April 29, 2024 13:09

tlabaj approved these changes Apr 29, 2024

View reviewed changes

tlabaj merged commit fe1d86c into patternfly:main Apr 29, 2024
13 checks passed

thatblindgeye mentioned this pull request Apr 30, 2024

Wizard v6 - Consider making WizardBody required #10319

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

thatblindgeye commented Apr 17, 2024

thatblindgeye Apr 17, 2024

patternfly-build commented Apr 17, 2024 •

edited

Loading

thatblindgeye Apr 17, 2024 •

edited

Loading

jessiehuff Apr 19, 2024

tlabaj Apr 25, 2024

mcoker Apr 25, 2024

jessiehuff Apr 19, 2024

thatblindgeye Apr 19, 2024

thatblindgeye Apr 22, 2024

tlabaj Apr 25, 2024

jessiehuff Apr 19, 2024

jessiehuff left a comment

tlabaj left a comment

patternfly-build commented Apr 29, 2024

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

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

Conversation

thatblindgeye commented Apr 17, 2024

Choose a reason for hiding this comment

patternfly-build commented Apr 17, 2024 • edited Loading

thatblindgeye Apr 17, 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

jessiehuff left a comment

Choose a reason for hiding this comment

tlabaj left a comment

Choose a reason for hiding this comment

patternfly-build commented Apr 29, 2024

patternfly-build commented Apr 17, 2024 •

edited

Loading

thatblindgeye Apr 17, 2024 •

edited

Loading