[popover2] feat: Popover2 popupkind #5300
Conversation
dlech
force-pushed
the
popover2-popupkind
branch
4 times, most recently
from
8dc1942 to
ff6de09
Compare
packages/popover2/src/popover2.tsx
Outdated
| @@ -50,14 +50,28 @@ export const Popover2InteractionKind = { | |||
| // eslint-disable-next-line @typescript-eslint/no-redeclare | |||
| export type Popover2InteractionKind = typeof Popover2InteractionKind[keyof typeof Popover2InteractionKind]; | |||
|
|
|||
| /** Specifies the popup kind for aria-haspopup. */ | |||
| export enum Popover2PopupKind { | |||
There was a problem hiding this comment.
I think we can generalize this enum name and just call it PopupKind, and move it to a new file popupKind.ts in this package. Also please include a link to the docs in the JSDoc for this symbol.
packages/popover2/src/popover2.tsx
Outdated
| @@ -327,7 +352,7 @@ export class Popover2<T> extends AbstractPureComponent2<Popover2Props<T>, IPopov | |||
| // Ensure target is focusable if relevant prop enabled | |||
| const targetTabIndex = openOnTargetFocus && isHoverInteractionKind ? 0 : undefined; | |||
| const targetProps = { | |||
| "aria-haspopup": "true", | |||
| "aria-haspopup": this.props.popupKind ?? (isHoverInteractionKind ? undefined : "true"), | |||
There was a problem hiding this comment.
I think this should be only undefined if this.props.interactionKind === Popover2InteractionKind.HOVER_TARGET_ONLY.
in the case of Popover2InteractionKind.HOVER popovers, you can still interact with the content, and we have instances of that in our applications. so haspopup should be set to something.
when you make this change, please make sure to update the new prop JSDoc.
| }); | ||
|
|
||
| it("renders without aria-haspopup attr for hover interaction", () => { | ||
| wrapper = renderPopover({ isOpen: true, interactionKind: Popover2InteractionKind.HOVER }); |
There was a problem hiding this comment.
this test will have to be updated if you make my change suggested above.
This adds a new `popupKind` property to the `Popover2` component for controlling the `aria-haspopup` attribute of the target element. The current implementation hard-codes `aria-haspopup` to "true" which means that the popup is a menu. However, this is not true in all cases. Furthermore, the `aria-haspopup` attribute should not be set for tooltips since they are not interactive elements. So if the interaction kind is a hover interaction, this new property is ignored. Reference: https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Attributes/aria-haspopup
|
thanks @dlech! |
Fixes #0000
Checklist
Changes proposed in this pull request:
This adds a new
popupKindproperty to thePopover2component for controlling thearia-haspopupattribute of the target element.The current implementation hard-codes
aria-haspopupto "true" which means that the popup is a menu. However, this is not true in all cases. Furthermore, thearia-haspopupattribute should not be set for tooltips since they are not interactive elements. So if the interaction kind is a hover interaction, this new property is ignored.Reference: https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Attributes/aria-haspopup
Reviewers should focus on:
Screenshot
Before:
After:
