Skip to content

Conversation

@thatblindgeye
Copy link
Contributor

@thatblindgeye thatblindgeye commented Feb 3, 2023

What: Closes #5897

Wizard examples

Will open up codemod issues for the below depending on if there are any changes made.

  • Alert: Re-implemented the fix from chore: fix various a11y violations in examples (2) #7604 - was considering having roles of region/alert/status added depending on use case (region for static alerts on page load, alert/status for alerts that dynamically render), but something like that may depend on how we intend for Alert and Alert Group to be used, i.e. Alerts should always go in an Alert Group or not
  • Wizard / Wizard Next: added logic to conditionally render aria labeling attributes only if a scrollbar is present (and add 'role="region"` if the Wizard body is a div)
  • Menu / MenuList: Removed the arialabel from Menu and added it to MenuList
  • MenuItem: Conditionally applied arialabel to either the internal li or Component element, depending on if hasCheckbox is true
  • PageGroup / PageNavigation updated logic so arialabel only applies when hasOverflowScroll is true
    • For PageNavigation, it might make sense to remove/deprecate aria-label and hasOverflowScroll and instead have the overflow styling applied directly to the Nav component. Right now with this setup, the "Group section" Page example has some extraneous focus happening. In a vertical nav with scrollbar the focus should end up within the scrollable region via nav items in order to scroll via keyboard
  • ProgressStep (in ProgressStepper component): made the aria-labelledby apply only when popoverRender is passed

Additional issues:

@patternfly-build
Copy link
Collaborator

patternfly-build commented Feb 3, 2023

@thatblindgeye thatblindgeye linked an issue Feb 3, 2023 that may be closed by this pull request
@thatblindgeye thatblindgeye marked this pull request as ready for review February 3, 2023 19:12
@nicolethoen
Copy link
Contributor

@jeffpuzzo @thatblindgeye
I thought I was losing my mind, but the kitchen sink example you updated in this PR, it's no longer in the docs anymore, right? Though It is referenced from the Custom Footer example.

@thatblindgeye
Copy link
Contributor Author

@nicolethoen Ah yeah that's true. As a quick fix I can delete the KitchenSink example and remove the link reference to it. If we want to go another route (add back in KitchenSink?) I'd be happy to go with that, though.

@jpuzz0
Copy link
Contributor

jpuzz0 commented Feb 10, 2023

@nicolethoen Ah yeah that's true. As a quick fix I can delete the KitchenSink example and remove the link reference to it. If we want to go another route (add back in KitchenSink?) I'd be happy to go with that, though.

It looks like KitchenSink was maybe added back as a part of a rebase of v5. @thatblindgeye It should be removed. Also, I had this change locally, which I think you could make here to cut ties with it altogether (in the Next wizard's markdown for examples):

image

@thatblindgeye thatblindgeye force-pushed the iss5897_arialabelDivs branch 2 times, most recently from a0b18b3 to 553a77a Compare February 10, 2023 21:29
@nicolethoen
Copy link
Contributor

is there any reason we should be demonstrating this feature somewhere in the examples since the kitchen sink example is gone? Or will documentation like props descriptions and the accessibility docs be enough?

@thatblindgeye
Copy link
Contributor Author

is there any reason we should be demonstrating this feature somewhere in the examples since the kitchen sink example is gone? Or will documentation like props descriptions and the accessibility docs be enough?

@nicolethoen The description + future a11y docs should cover it, but I updated the basic examples for Wizard/Wizard Next to create that scrollbar for the first step since it doesn't hurt to showcase.

Copy link
Contributor

@jessiehuff jessiehuff left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for answering all my questions offline. :) I also tested the Alert specifically in JAWS because I know it can be finicky. It seemed to work ok. I could also test the AlertGroup or any of the other components if we're worried about them.

@kmcfaul kmcfaul merged commit b459b91 into patternfly:v5 Feb 22, 2023
dlabrecq pushed a commit to dlabrecq/patternfly-react that referenced this pull request Feb 22, 2023
…tternfly#8649)

* chore(multiple components): updated arialabel on unsupported divs

* Updated arialabeling on Wizard

* Updated arialabeling on Wizard Next

* Updated arialabeling on Accordion, Menu, and Page components

* Updated arialabelledby on ProgressStep

* Added integration test for Wizard focus functionality

* Updated failing unit test

* Removed kitchen sink example/references

* Updated basic examples
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Aria-label not supported on specific div elements

7 participants