Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

a11y(Tooltip): "tooltip" role #6865

Open
wants to merge 42 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
42 commits
Select commit Hold shift + click to select a range
35b3eeb
a11y(Tooltip): "tooltip" role
bvandercar-vt Jun 25, 2024
a007008
chore: add dep
bvandercar-vt Jun 25, 2024
8ef625b
lint fixes
bvandercar-vt Jun 25, 2024
d5b5e01
lint fixes
bvandercar-vt Jun 25, 2024
5d3fff2
chore: yarn.lock
bvandercar-vt Jun 25, 2024
8f47c5d
none if empty content
bvandercar-vt Jun 25, 2024
8a7ede2
style: lint fix
bvandercar-vt Jun 25, 2024
bdcb9a8
no innerText check
bvandercar-vt Jun 25, 2024
1345c97
fix test
bvandercar-vt Jun 25, 2024
73a0519
add changelog entry
bvandercar-vt Jun 25, 2024
8c44382
revert change to Utils.isReactNodeEmpty
bvandercar-vt Jun 26, 2024
730d177
fix: aria-describedby
bvandercar-vt Jun 26, 2024
8851895
fix children for popover warning
bvandercar-vt Jun 26, 2024
c2d103a
re-add jsdocs
bvandercar-vt Jun 26, 2024
d5795a2
Merge branch 'develop' into bvandercar/tooltip-role
bvandercar-vt Jun 26, 2024
bf0706e
style: re-add jsdocs
bvandercar-vt Jun 26, 2024
8d19874
duplicate type
bvandercar-vt Jun 26, 2024
1d5a83a
fix: isContentEmpty
bvandercar-vt Jun 26, 2024
be12eb3
refactor to allow for passed content with ID
bvandercar-vt Jun 26, 2024
4010a2e
Merge branch 'develop' into bvandercar/tooltip-role
bvandercar-vt Jul 1, 2024
720a1b2
use useMemo
bvandercar-vt Jul 1, 2024
3b062f1
useCallback
bvandercar-vt Jul 1, 2024
05113cb
Utils.uniq
bvandercar-vt Jul 1, 2024
be533a0
refactor:: other describedby
bvandercar-vt Jul 1, 2024
2acbda8
remove uniq
bvandercar-vt Jul 1, 2024
31a7fdc
style: use Utils.isEmptyString
bvandercar-vt Jul 1, 2024
241696b
Merge branch 'develop' of https://github.com/bvandercar-vt/blueprint …
bvandercar-vt Jul 8, 2024
150687f
Merge branch 'develop' of https://github.com/palantir/blueprint into …
bvandercar-vt Jul 12, 2024
110923a
Merge branch 'develop' of https://github.com/palantir/blueprint into …
bvandercar-vt Jul 16, 2024
2254a18
Merge branch 'develop' of https://github.com/palantir/blueprint into …
bvandercar-vt Jul 23, 2024
bfaa12f
Merge branch 'develop' into bvandercar/tooltip-role
bvandercar-vt Jul 23, 2024
7c7083d
Merge branch 'develop' of https://github.com/palantir/blueprint into …
bvandercar-vt Aug 7, 2024
5c7812a
Merge branch 'develop' of https://github.com/palantir/blueprint into …
bvandercar-vt Aug 8, 2024
2de1166
Merge branch 'develop' into bvandercar/tooltip-role
bvandercar-vt Aug 8, 2024
1d5157e
fix merge
bvandercar-vt Aug 8, 2024
7ff1a3b
use content role if passed
bvandercar-vt Aug 12, 2024
cfd20f6
reformat
bvandercar-vt Aug 12, 2024
1d8abfe
dont use ternary
bvandercar-vt Aug 12, 2024
76f22f9
refactor ts types
bvandercar-vt Aug 12, 2024
2ca0b39
revert test change
bvandercar-vt Aug 12, 2024
4646017
rename const
bvandercar-vt Aug 12, 2024
0336796
ensure aria-describedby is unique
bvandercar-vt Aug 12, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/core/src/common/utils/reactUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,8 @@ import { isEmptyString } from "./jsUtils";
export function isReactNodeEmpty(node?: React.ReactNode, skipArray = false): boolean {
return (
node == null ||
node === "" ||
node === false ||
isEmptyString(node) ||
bvandercar-vt marked this conversation as resolved.
Show resolved Hide resolved
(!skipArray &&
Array.isArray(node) &&
// only recurse one level through arrays, for performance
Expand Down
12 changes: 3 additions & 9 deletions packages/core/src/components/popover/popover.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -74,11 +74,6 @@ export interface PopoverProps<TProps extends DefaultPopoverTargetHTMLProps = Def
/** HTML props for the backdrop element. Can be combined with `backdropClassName`. */
backdropProps?: React.HTMLProps<HTMLDivElement>;

/**
* The content displayed inside the popover.
*/
content?: string | React.JSX.Element;
bvandercar-vt marked this conversation as resolved.
Show resolved Hide resolved

/**
* The kind of interaction that triggers the display of the popover.
*
Expand Down Expand Up @@ -252,8 +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() === "");
if (isContentEmpty) {
if (Utils.isReactNodeEmpty(content)) {
// need to do this check in render(), because `isOpen` is derived from
// state, and state can't necessarily be accessed in validateProps.
if (!disabled && isOpen !== false && !Utils.isNodeEnv("production")) {
Expand Down Expand Up @@ -301,7 +295,7 @@ export class Popover<
}
}

protected validateProps(props: PopoverProps<T> & { children?: React.ReactNode }) {
bvandercar-vt marked this conversation as resolved.
Show resolved Hide resolved
protected validateProps(props: PopoverProps<T>) {
if (props.isOpen == null && props.onInteraction != null) {
console.warn(Errors.POPOVER_WARN_UNCONTROLLED_ONINTERACTION);
}
Expand Down Expand Up @@ -417,7 +411,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;
Expand Down
17 changes: 15 additions & 2 deletions packages/core/src/components/popover/popoverSharedProps.ts
Original file line number Diff line number Diff line change
Expand Up @@ -86,10 +86,20 @@ export type PopoverClickTargetHandlers<TProps extends DefaultPopoverTargetHTMLPr
* @template TProps HTML props interface for target element,
* defaults to props for HTMLElement in IPopoverProps and ITooltipProps
*/
export interface PopoverSharedProps<TProps extends DefaultPopoverTargetHTMLProps> extends OverlayableProps, Props {
export interface PopoverSharedProps<
TProps extends DefaultPopoverTargetHTMLProps,
// eslint-disable-next-line @typescript-eslint/ban-types
AdditionalTargetRendererProps = {},
> 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.
Expand Down Expand Up @@ -238,7 +248,10 @@ export interface PopoverSharedProps<TProps extends DefaultPopoverTargetHTMLProps
// improvement would be better implemented if we added another type param to Popover, something like
// Popover<TProps, "click" | "hover">. Instead of discriminating, we union the different possible event handlers
// that may be passed (they are all optional properties anyway).
props: PopoverTargetProps & PopoverHoverTargetHandlers<TProps> & PopoverClickTargetHandlers<TProps>,
props: PopoverTargetProps &
PopoverHoverTargetHandlers<TProps> &
PopoverClickTargetHandlers<TProps> &
AdditionalTargetRendererProps,
) => React.JSX.Element;

/**
Expand Down
52 changes: 29 additions & 23 deletions packages/core/src/components/tooltip/tooltip.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -17,22 +17,18 @@
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";
import type { DefaultPopoverTargetHTMLProps, PopoverSharedProps } from "../popover/popoverSharedProps";
import { TooltipContext, type TooltipContextState, TooltipProvider } from "../popover/tooltipContext";

export interface TooltipProps<TProps extends DefaultPopoverTargetHTMLProps = DefaultPopoverTargetHTMLProps>
extends Omit<PopoverSharedProps<TProps>, "shouldReturnFocusOnClose">,
extends Omit<PopoverSharedProps<TProps, { tooltipId: string }>, "shouldReturnFocusOnClose">,
IntentProps {
/**
* The content that will be displayed inside of the tooltip.
*/
content: React.JSX.Element | string;
evansjohnson marked this conversation as resolved.
Show resolved Hide resolved

/**
* Whether to use a compact appearance, which reduces the visual padding around
* tooltip content.
Expand All @@ -42,19 +38,11 @@ export interface TooltipProps<TProps extends DefaultPopoverTargetHTMLProps = Def
compact?: boolean;

/**
* The amount of time in milliseconds the tooltip should remain open after
* the user hovers off the trigger. The timer is canceled if the user mouses
* over the target before it expires.
*
evansjohnson marked this conversation as resolved.
Show resolved Hide resolved
* @default 0
*/
hoverCloseDelay?: number;

/**
* The amount of time in milliseconds the tooltip should wait before opening
* after the user hovers over the trigger. The timer is canceled if the user
* mouses away from the target before it expires.
*
* @default 100
*/
hoverOpenDelay?: number;
Expand All @@ -68,12 +56,6 @@ export interface TooltipProps<TProps extends DefaultPopoverTargetHTMLProps = Def
interactionKind?: typeof PopoverInteractionKind.HOVER | typeof PopoverInteractionKind.HOVER_TARGET_ONLY;

/**
* Indicates how long (in milliseconds) the tooltip's appear/disappear
* transition takes. This is used by React `CSSTransition` to know when a
* transition completes and must match the duration of the animation in CSS.
* Only set this prop if you override Blueprint's default transitions with
* new transitions of a different length.
*
* @default 100
*/
transitionDuration?: number;
Expand Down Expand Up @@ -114,13 +96,29 @@ export class Tooltip<
this.popoverRef.current?.reposition();
}

protected validateProps(props: TooltipProps<T>) {
const childrenCount = React.Children.count(props.children);
if (childrenCount > 1) {
console.warn(Errors.POPOVER_WARN_TOO_MANY_CHILDREN);
bvandercar-vt marked this conversation as resolved.
Show resolved Hide resolved
}
// 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,
bvandercar-vt marked this conversation as resolved.
Show resolved Hide resolved
});

return (
<Popover
modifiers={{
Expand All @@ -134,6 +132,14 @@ export class Tooltip<
},
}}
{...restProps}
// eslint-disable-next-line react/jsx-no-bind
bvandercar-vt marked this conversation as resolved.
Show resolved Hide resolved
renderTarget={renderTarget ? props => renderTarget({ ...props, tooltipId }) : undefined}
content={
// want Popover to warn if empty, so don't wrap in element if empty.
Utils.isReactNodeEmpty(content)
? content
: Utils.ensureElement(content, undefined, { role: "tooltip", id: tooltipId })
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we can do this because a consumer may have already defined an id on the content which this would override

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Highly doubt that would ever be the case, can't imagine what other scenarios they'd be doing this for. There are many cases, such as in Popover, where we apply a bunch of props without caring what the consumer may have already applied.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should account for that case: be12eb3

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@evansjohnson the other alternative solution here would be we add a prop contentProps to Popover and apply it to the div that's already wrapping the content. Then we could apply these props via that. Up to you.

Copy link
Contributor

Choose a reason for hiding this comment

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

what other scenarios they'd be doing this for

basically to fix this issue in consumer land is what I'm thinking about, or possibly adding an id used to target a tooltip in tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Refactored, please re review, I think this concern has been solved

}
autoFocus={false}
canEscapeKeyClose={false}
disabled={ctxState.forceDisabled ?? disabled}
Expand All @@ -143,7 +149,7 @@ export class Tooltip<
portalContainer={this.props.portalContainer}
ref={this.popoverRef}
>
{children}
{childTarget}
</Popover>
);
};
Expand Down
2 changes: 1 addition & 1 deletion packages/core/test/popover/popoverTests.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ describe("<Popover>", () => {
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(<Popover renderTarget={() => <span>"boom"</span>}>pow</Popover>);
assert.isTrue(warnSpy.calledWith(Errors.POPOVER_WARN_DOUBLE_TARGET));
});
Expand Down
54 changes: 52 additions & 2 deletions packages/core/test/tooltip/tooltipTests.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -20,15 +20,65 @@ import * as React from "react";
import { spy, stub } from "sinon";

import { Classes } from "../../src/common";
import * as Errors from "../../src/common/errors";
import { Button, Overlay2 } from "../../src/components";
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("<Tooltip>", () => {
describe("validation", () => {
evansjohnson marked this conversation as resolved.
Show resolved Hide resolved
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(<Tooltip content={<p>Text</p>} usePortal={false} />);
assert.isTrue(warnSpy.calledWith(Errors.POPOVER_REQUIRES_TARGET));
});

it("warns if given > 1 target elements", () => {
mount(
<Tooltip>
<Button />
<Button />
</Tooltip>,
);
assert.isTrue(warnSpy.calledWith(Errors.POPOVER_WARN_TOO_MANY_CHILDREN));
});

it("warns if given children and renderTarget prop", () => {
mount(<Tooltip renderTarget={() => <span>"boom"</span>}>pow</Tooltip>);
assert.isTrue(warnSpy.calledWith(Errors.POPOVER_WARN_DOUBLE_TARGET));
});

it("warns if given targetProps and renderTarget", () => {
mount(<Tooltip targetProps={{ role: "none" }} renderTarget={() => <span>"boom"</span>} />);
assert.isTrue(warnSpy.calledWith(Errors.POPOVER_WARN_TARGET_PROPS_WITH_RENDER_TARGET));
});

it("warns if attempting to open with empty content", () => {
renderTooltip({ content: undefined, isOpen: true });
assert.isTrue(warnSpy.calledWith(Errors.POPOVER_WARN_EMPTY_CONTENT));
});

it("warns and disables if given empty content", () => {
const tooltip = renderTooltip({ content: undefined, isOpen: true });

assert.isFalse(tooltip.find(Overlay2).exists(), "not open for undefined content");
assert.equal(warnSpy.callCount, 2);

tooltip.setProps({ content: " " });
assert.isFalse(tooltip.find(Overlay2).exists(), "not open for white-space string content");
assert.equal(warnSpy.callCount, 3);
});
});

describe("rendering", () => {
it("propogates class names correctly", () => {
const tooltip = renderTooltip({
Expand Down Expand Up @@ -159,7 +209,7 @@ describe("<Tooltip>", () => {
function renderTooltip(props?: Partial<TooltipProps>) {
return mount<TooltipProps>(
<Tooltip content={<p>Text</p>} hoverOpenDelay={0} {...props} usePortal={false}>
<Button id={TEST_TARGET_ID} text="target" />
<Button text="target" />
</Tooltip>,
);
}
Expand Down