From 35b3eeb9b1e34b7292f2bf6f1ba9e99337cdd9cb Mon Sep 17 00:00:00 2001 From: Blake Vandercar Date: Mon, 24 Jun 2024 20:38:06 -0600 Subject: [PATCH 01/32] a11y(Tooltip): "tooltip" role --- .../core/src/components/popover/popover.tsx | 13 ++--- .../components/popover/popoverSharedProps.ts | 14 ++++- .../core/src/components/tooltip/tooltip.tsx | 46 ++++++++-------- packages/core/test/popover/popoverTests.tsx | 2 +- packages/core/test/tooltip/tooltipTests.tsx | 54 ++++++++++++++++++- 5 files changed, 92 insertions(+), 37 deletions(-) diff --git a/packages/core/src/components/popover/popover.tsx b/packages/core/src/components/popover/popover.tsx index 9f8d65335b..16f3e8dba0 100644 --- a/packages/core/src/components/popover/popover.tsx +++ b/packages/core/src/components/popover/popover.tsx @@ -25,6 +25,7 @@ import { Reference, type ReferenceChildrenProps, } from "react-popper"; +import innerText from "react-innertext"; import { AbstractPureComponent, @@ -40,7 +41,6 @@ import { Overlay2 } from "../overlay2/overlay2"; import { ResizeSensor } from "../resize-sensor/resizeSensor"; // eslint-disable-next-line import/no-cycle import { Tooltip } from "../tooltip/tooltip"; - import { matchReferenceWidthModifier } from "./customModifiers"; import { POPOVER_ARROW_SVG_SIZE, PopoverArrow } from "./popoverArrow"; import { positionToPlacement } from "./popoverPlacementUtils"; @@ -74,11 +74,6 @@ export interface PopoverProps; - /** - * The content displayed inside the popover. - */ - content?: string | React.JSX.Element; - /** * The kind of interaction that triggers the display of the popover. * @@ -252,7 +247,7 @@ export class Popover< const { disabled, content, placement, position = "auto", positioningStrategy } = this.props; const { isOpen } = this.state; - const isContentEmpty = content == null || (typeof content === "string" && content.trim() === ""); + const isContentEmpty = content == null || innerText(content).trim() == ""; if (isContentEmpty) { // need to do this check in render(), because `isOpen` is derived from // state, and state can't necessarily be accessed in validateProps. @@ -301,7 +296,7 @@ export class Popover< } } - protected validateProps(props: PopoverProps & { children?: React.ReactNode }) { + protected validateProps(props: PopoverProps) { if (props.isOpen == null && props.onInteraction != null) { console.warn(Errors.POPOVER_WARN_UNCONTROLLED_ONINTERACTION); } @@ -417,7 +412,7 @@ export class Popover< tabIndex: targetTabIndex, }); } else { - const childTarget = Utils.ensureElement(React.Children.toArray(children)[0])!; + const childTarget = Utils.ensureElement(React.Children.toArray(children)[0]); if (childTarget === undefined) { return null; diff --git a/packages/core/src/components/popover/popoverSharedProps.ts b/packages/core/src/components/popover/popoverSharedProps.ts index 5c82ee1d2d..e471082c92 100644 --- a/packages/core/src/components/popover/popoverSharedProps.ts +++ b/packages/core/src/components/popover/popoverSharedProps.ts @@ -86,10 +86,17 @@ export type PopoverClickTargetHandlers extends OverlayableProps, Props { +export interface PopoverSharedProps + extends OverlayableProps, + Props { /** Interactive element which will trigger the popover. */ children?: React.ReactNode; + /** + * The content displayed inside the popover. + */ + content?: string | React.JSX.Element; + /** * A boundary element supplied to the "flip" and "preventOverflow" modifiers. * This is a shorthand for overriding Popper.js modifier options with the `modifiers` prop. @@ -238,7 +245,10 @@ export interface PopoverSharedProps. Instead of discriminating, we union the different possible event handlers // that may be passed (they are all optional properties anyway). - props: PopoverTargetProps & PopoverHoverTargetHandlers & PopoverClickTargetHandlers, + props: PopoverTargetProps & + PopoverHoverTargetHandlers & + PopoverClickTargetHandlers & + AdditionalTargetRendererProps, ) => React.JSX.Element; /** diff --git a/packages/core/src/components/tooltip/tooltip.tsx b/packages/core/src/components/tooltip/tooltip.tsx index 4a09e4285d..a34a751d22 100644 --- a/packages/core/src/components/tooltip/tooltip.tsx +++ b/packages/core/src/components/tooltip/tooltip.tsx @@ -17,8 +17,9 @@ import classNames from "classnames"; import * as React from "react"; -import { AbstractPureComponent, DISPLAYNAME_PREFIX, type IntentProps } from "../../common"; +import { AbstractPureComponent, DISPLAYNAME_PREFIX, type IntentProps, Utils } from "../../common"; import * as Classes from "../../common/classes"; +import * as Errors from "../../common/errors"; // eslint-disable-next-line import/no-cycle import { Popover, type PopoverInteractionKind } from "../popover/popover"; import { TOOLTIP_ARROW_SVG_SIZE } from "../popover/popoverArrow"; @@ -26,13 +27,8 @@ import type { DefaultPopoverTargetHTMLProps, PopoverSharedProps } from "../popov import { TooltipContext, type TooltipContextState, TooltipProvider } from "../popover/tooltipContext"; export interface TooltipProps - extends Omit, "shouldReturnFocusOnClose">, + extends Omit, "shouldReturnFocusOnClose">, IntentProps { - /** - * The content that will be displayed inside of the tooltip. - */ - content: React.JSX.Element | string; - /** * Whether to use a compact appearance, which reduces the visual padding around * tooltip content. @@ -42,19 +38,11 @@ export interface TooltipProps) { + const childrenCount = React.Children.count(props.children); + if (childrenCount > 1) { + console.warn(Errors.POPOVER_WARN_TOO_MANY_CHILDREN); + } + // all other warnings should occur in Popover, not here. + } + // any descendant ContextMenus may update this ctxState private renderPopover = (ctxState: TooltipContextState) => { - const { children, compact, disabled, intent, popoverClassName, ...restProps } = this.props; + const { children, content, renderTarget, compact, disabled, intent, popoverClassName, ...restProps } = + this.props; + const popoverClasses = classNames(Classes.TOOLTIP, Classes.intentClass(intent), popoverClassName, { [Classes.COMPACT]: compact, }); + const tooltipId = Utils.uniqueId("tooltip"); + + const childTarget = Utils.ensureElement(React.Children.toArray(children)[0], undefined, { + "aria-describedby": tooltipId, + }); + return ( renderTarget({ ...props, tooltipId }) : undefined} + content={Utils.ensureElement(content, undefined, { role: "tooltip", id: tooltipId })} autoFocus={false} canEscapeKeyClose={false} disabled={ctxState.forceDisabled ?? disabled} @@ -143,7 +143,7 @@ export class Tooltip< portalContainer={this.props.portalContainer} ref={this.popoverRef} > - {children} + {childTarget} ); }; diff --git a/packages/core/test/popover/popoverTests.tsx b/packages/core/test/popover/popoverTests.tsx index 4061178f55..ad5d9bf3e8 100644 --- a/packages/core/test/popover/popoverTests.tsx +++ b/packages/core/test/popover/popoverTests.tsx @@ -78,7 +78,7 @@ describe("", () => { assert.isTrue(warnSpy.calledWith(Errors.POPOVER_WARN_TOO_MANY_CHILDREN)); }); - it("warns if given children and target prop", () => { + it("warns if given children and renderTarget prop", () => { shallow( "boom"}>pow); assert.isTrue(warnSpy.calledWith(Errors.POPOVER_WARN_DOUBLE_TARGET)); }); diff --git a/packages/core/test/tooltip/tooltipTests.tsx b/packages/core/test/tooltip/tooltipTests.tsx index 64ed778c73..83f2364ad1 100644 --- a/packages/core/test/tooltip/tooltipTests.tsx +++ b/packages/core/test/tooltip/tooltipTests.tsx @@ -21,14 +21,64 @@ import { spy, stub } from "sinon"; import { Classes } from "../../src/common"; import { Button, Overlay2 } from "../../src/components"; +import * as Errors from "../../src/common/errors"; import { Popover } from "../../src/components/popover/popover"; import { Tooltip, type TooltipProps } from "../../src/components/tooltip/tooltip"; const TARGET_SELECTOR = `.${Classes.POPOVER_TARGET}`; const TOOLTIP_SELECTOR = `.${Classes.TOOLTIP}`; -const TEST_TARGET_ID = "test-target"; describe("", () => { + describe("validation", () => { + let warnSpy: sinon.SinonStub; + + // use sinon.stub to prevent warnings from appearing in the test logs + before(() => (warnSpy = stub(console, "warn"))); + beforeEach(() => warnSpy.resetHistory()); + after(() => warnSpy.restore()); + + it("throws error if given no target", () => { + mount(Text

} usePortal={false} />); + assert.isTrue(warnSpy.calledWith(Errors.POPOVER_REQUIRES_TARGET)); + }); + + it("warns if given > 1 target elements", () => { + mount( + +