patternfly-react icon indicating copy to clipboard operation
patternfly-react copied to clipboard

Bug - [Popover] - [accessibility issues with footer, header, and heading]

Open pedrottimark opened this issue 1 year ago • 7 comments

Describe the problem

Accessibility issues for Popover element:

  • with footerContent prop
  • with headerContent prop
  • with default value h6 of headerComponent prop

Heading levels should only increase by one

https://dequeuniversity.com/rules/axe/4.9/heading-order

<h6 class="pf-v5-c-popover__title-text"><div>Popover header</div></h6>

Document should not have more than one banner landmark

https://dequeuniversity.com/rules/axe/4.9/landmark-no-duplicate-banner

<header class="pf-v5-c-masthead pf-m-display-inline ws-masthead">

Related node:

<header class="pf-v5-c-popover__header"><div class="pf-v5-c-popover__title" id="popover-pf-17234753664484trsgdfq2md-header"><h6 class="pf-v5-c-popover__title-text"><div>Popover header</div></h6></div></header>

Document should not have more than one contentinfo landmark

https://dequeuniversity.com/rules/axe/4.9/landmark-no-duplicate-contentinfo

<footer class="pf-v5-c-page__main-section ws-org-pfsite-l-footer">

Related node:

<footer class="pf-v5-c-popover__footer" id="popover-pf-17234753664484trsgdfq2md-footer">Popover footer</footer>

Ensures landmarks are unique

https://dequeuniversity.com/rules/axe/4.9/landmark-unique

<header class="pf-v5-c-masthead pf-m-display-inline ws-masthead">

Related node:

<header class="pf-v5-c-popover__header"><div class="pf-v5-c-popover__title" id="popover-pf-17234753664484trsgdfq2md-header"><h6 class="pf-v5-c-popover__title-text"><div>Popover header</div></h6></div></header>
<footer class="pf-v5-c-page__main-section ws-org-pfsite-l-footer">

Related node:

<footer class="pf-v5-c-popover__footer" id="popover-pf-17234753664484trsgdfq2md-footer">Popover footer</footer>

How do you reproduce the problem?

  1. Visit https://www.patternfly.org/components/popover/#basic
  2. Click Toggle popover button.
  3. Scan page with axe DevTools and ignore the last issue as irrelevant.

Expected behavior

Render markup that passes axe DevTools rules.

Is this issue blocking you?

No, we will fix 10 occurrences as follows:

  1. Move occurrences of footerContent, or headerContent, or both, to props of a pure presentation component that composes them with bodyContent and returns markup for bodyContent prop of Popover element. That is, replace footer and header elements with div elements that have PatternFly classes for same visual appearance.
  2. Render (instead of heading element) p element that has PatternFly class for same visual appearance, but takes out of heading hierarchy of page. Similar solution as our workaround component="p" prop for Alert elements that do not seem to fit heading hierarchy of page.

Screenshots

patternfly_components_popover_basic

What is your environment?

  • OS: macOS 14.6.1
  • Browser: Chrome 127
  • PatternFly: react-core 5.2.2

What is your product and what release date are you targeting?

Red Hat Advanced Cluster Security

Any other information?

pedrottimark avatar Aug 12 '24 15:08 pedrottimark

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

github-actions[bot] avatar Dec 26 '24 11:12 github-actions[bot]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

github-actions[bot] avatar Mar 09 '25 11:03 github-actions[bot]

https://github.com/patternfly/patternfly-react/issues/11393 is related.

Part of this could be resolved by us implementing a prop to control the wrapper element in PopoverHeader and PopoverFooter (maybe either div, span, or the default header|footer elements). The issue arises when the Popover is appended to the document body and thus outside the main element; at that point header and footer retain their semantic roles, but already having a header and footer on the page itself conflicts. Inside a main element those elements have different semantics, so appending the Popover "inline" or somewhere else within the main element would also help resolve the issue.

The heading level issue requires more manual update as it's dependent on the context and up to the consumer to ensure the appropriate heading level is being used. I'd recommend rethinking the use of p elements that retain the "heading" styling, though. Either the heading element should be used and just update which heading level is used, or no heading should be included. It may lead to confusion to have text that looks like heading text in other views, but isn't semantically a heading element.

thatblindgeye avatar Mar 21 '25 12:03 thatblindgeye

Thank you for commenting.

For your info, a more recent situation with info prop of Th element prevented even the work-around. https://www.patternfly.org/components/table#custom-row-wrapper-header-tooltips--popovers

I'd recommend rethinking the use of p elements that retain the "heading" styling, though. Either the heading element should be used and just update which heading level is used, or no heading should be included. It may lead to confusion to have text that looks like heading text in other views, but isn't semantically a heading element.

Aha, for Popover element, I will review uses to omit heading whenever possible, like first example in guidelines: https://www.patternfly.org/components/popover/design-guidelines#usage

For Alert element:

  • The default h6 element confirms that consistent formatting is intuitive, independent of context on page.
  • Context-dependent heading element, but consistent formatting independent of level seems like similar problem to what you noted. As you can imagine, a few components render reusable application-specific React elements (that have Alert elements) in different heading hierarchies.

Is there a generic solution to heading-ish content independent of hierarchy?

During a past tour of duty in technical documentation, we used a specific style for equivalent to Alert elements.

When fixing issues from axe DevTools, I searched unsuccessfully for guidance (PatternFly or otherwise) on the subject.

pedrottimark avatar Mar 21 '25 13:03 pedrottimark

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

github-actions[bot] avatar May 21 '25 11:05 github-actions[bot]

So sad 😰

pedrottimark avatar May 21 '25 13:05 pedrottimark

Sorry for the delay in response @pedrottimark. FWIW our github actions now only labels things as stale when there's a lack of activity and simply gives us a reminder to look at those stale things (no more auto-closing of issues).

For your info, a more recent situation with info prop of Th element prevented even the work-around.

Yeah that would be expected. As part of adding the ability to override the wrapper of popover header/footer elements, being able to adjust them via that info prop in Th component should also work. If you'd be okay with it I can update this issue to be scoped to adding that customization for Popover headers/footers wrappers.

Is there a generic solution to heading-ish content independent of hierarchy?

Not sure if I 100% understand this question. Do you mean a solution for having content look heading-ish without being a part of the hierarchical heading structure of the rest of the page?

thatblindgeye avatar May 22 '25 13:05 thatblindgeye

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

github-actions[bot] avatar Jul 23 '25 11:07 github-actions[bot]