Skip to content

Commit

Permalink
Do not set tabIndex when no popover will show (#6851)
Browse files Browse the repository at this point in the history
  • Loading branch information
@benrucker
benrucker authored Jun 28, 2024
1 parent 6e0ce0e commit 9ee655b
Show file tree
Hide file tree
Showing 2 changed files with 124 additions and 33 deletions.
15 changes: 10 additions & 5 deletions packages/core/src/components/popover/popover.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -249,11 +249,10 @@ export class Popover<
}

public render() {
const { disabled, content, placement, position = "auto", positioningStrategy } = this.props;
const { disabled, placement, position = "auto", positioningStrategy } = this.props;
const { isOpen } = this.state;

const isContentEmpty = content == null || (typeof content === "string" && content.trim() === "");
if (isContentEmpty) {
if (this.getIsContentEmpty()) {
// 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 @@ -344,7 +343,7 @@ export class Popover<
public reposition = () => this.popperScheduleUpdate?.();

private renderTarget = ({ ref: popperChildRef }: ReferenceChildrenProps) => {
const { children, className, fill, openOnTargetFocus, renderTarget } = this.props;
const { children, className, disabled, fill, openOnTargetFocus, renderTarget } = this.props;
const { isOpen } = this.state;
const isControlled = this.isControlled();
const isHoverInteractionKind = this.isHoverInteractionKind();
Expand Down Expand Up @@ -375,7 +374,8 @@ export class Popover<
onKeyDown: this.handleKeyDown,
};
// Ensure target is focusable if relevant prop enabled
const targetTabIndex = openOnTargetFocus && isHoverInteractionKind ? 0 : undefined;
const targetTabIndex =
!this.getIsContentEmpty() && !disabled && openOnTargetFocus && isHoverInteractionKind ? 0 : undefined;
const ownTargetProps = {
// N.B. this.props.className is passed along to renderTarget even though the user would have access to it.
// If, instead, renderTarget is undefined and the target is provided as a child, this.props.className is
Expand Down Expand Up @@ -788,6 +788,11 @@ export class Popover<
private isElementInPopover(element: Element) {
return this.getPopoverElement()?.contains(element) ?? false;
}

private getIsContentEmpty() {
const { content } = this.props;
return content == null || (typeof content === "string" && content.trim() === "");
}
}

function isEscapeKeypressEvent(e?: Event) {
Expand Down
142 changes: 114 additions & 28 deletions packages/core/test/popover/popoverTests.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,9 @@ import { PopoverArrow } from "../../src/components/popover/popoverArrow";
import { PopupKind } from "../../src/components/popover/popupKind";
import { Tooltip } from "../../src/components/tooltip/tooltip";

const BUTTON_WITH_TEST_ID = <Button data-testid="target-button" text="Target" />;
const BUTTON_ID_SELECTOR = "[data-testid='target-button']";

describe("<Popover>", () => {
let testsContainerElement: HTMLElement;
let wrapper: PopoverWrapper | undefined;
Expand Down Expand Up @@ -325,6 +328,14 @@ describe("<Popover>", () => {
});
});

it("does not add tabindex to target's child node when disabled=true", () => {
assertPopoverTargetTabIndex(false, {
disabled: true,
interactionKind: "hover",
openOnTargetFocus: true,
});
});

it("opens popover on target focus when interactionKind is HOVER", () => {
assertPopoverOpenStateForInteractionKind("hover", true);
});
Expand Down Expand Up @@ -419,17 +430,6 @@ describe("<Popover>", () => {
targetElement.simulate("focus");
assert.equal(wrapper.state("isOpen"), isOpen);
}

function assertPopoverTargetTabIndex(shouldTabIndexExist: boolean, popoverProps: Partial<PopoverProps>) {
wrapper = renderPopover({ ...popoverProps, usePortal: true });
const targetElement = wrapper.find("[data-testid='target-button']").hostNodes().getDOMNode();

if (shouldTabIndexExist) {
assert.equal(targetElement.getAttribute("tabindex"), "0");
} else {
assert.isNull(targetElement.getAttribute("tabindex"));
}
}
});

describe("in controlled mode", () => {
Expand Down Expand Up @@ -668,15 +668,14 @@ describe("<Popover>", () => {
});

describe("when composed with <Tooltip>", () => {
let root: ReactWrapper<any, any>;
let root: PopoverWrapper;
beforeEach(() => {
root = mount(
<Popover content="popover" hoverOpenDelay={0} hoverCloseDelay={0} usePortal={false}>
<Tooltip content="tooltip" hoverOpenDelay={0} hoverCloseDelay={0} usePortal={false}>
<Button text="Target" />
</Tooltip>
</Popover>,
{ attachTo: testsContainerElement },
root = renderPopover(
{ hoverOpenDelay: 0, hoverCloseDelay: 0, usePortal: false },
"popover",
<Tooltip content="tooltip" hoverOpenDelay={0} hoverCloseDelay={0} usePortal={false}>
{BUTTON_WITH_TEST_ID}
</Tooltip>,
);
});
afterEach(() => root.detach());
Expand All @@ -690,6 +689,78 @@ describe("<Popover>", () => {
root.find(`.${Classes.POPOVER_TARGET}`).first().simulate("click");
assert.lengthOf(root.find(`.${Classes.POPOVER}`), 1);
});

it("the target is focusable", () => {
assertTargetElementTabIndex(true, root.last().find(BUTTON_ID_SELECTOR).hostNodes().getDOMNode());
});

describe("when disabled=true", () => {
beforeEach(() => {
root.setProps({ disabled: true });
});

it("shows tooltip on hover", () => {
root.find(`.${Classes.POPOVER_TARGET}`).last().simulate("mouseenter");
assert.lengthOf(root.find(`.${Classes.TOOLTIP}`), 1);
});

it("does not show popover on click", () => {
root.find(`.${Classes.POPOVER_TARGET}`).last().simulate("click");
assert.lengthOf(root.find(`.${Classes.POPOVER}`), 0);
});

it("the target is focusable", () => {
assertTargetElementTabIndex(true, root.last().find(BUTTON_ID_SELECTOR).hostNodes().getDOMNode());
});
});
});

describe("when composed with a disabled <Tooltip>", () => {
let root: PopoverWrapper;
beforeEach(() => {
root = renderPopover(
{ hoverOpenDelay: 0, hoverCloseDelay: 0, usePortal: false },
"popover",
<Tooltip content="tooltip" disabled={true} hoverOpenDelay={0} hoverCloseDelay={0} usePortal={false}>
{BUTTON_WITH_TEST_ID}
</Tooltip>,
);
});
afterEach(() => root.detach());

it("does not show tooltip on hover", () => {
root.find(`.${Classes.POPOVER_TARGET}`).last().simulate("mouseenter");
assert.lengthOf(root.find(`.${Classes.TOOLTIP}`), 0);
});

it("shows popover on click", () => {
root.find(`.${Classes.POPOVER_TARGET}`).first().simulate("click");
assert.lengthOf(root.find(`.${Classes.POPOVER}`), 1);
});

it("the target is not focusable", () => {
assertTargetElementTabIndex(false, root.last().find(BUTTON_ID_SELECTOR).hostNodes().getDOMNode());
});

describe("when disabled=true", () => {
beforeEach(() => {
root.setProps({ disabled: true });
});

it("does not show tooltip on hover", () => {
root.find(`.${Classes.POPOVER_TARGET}`).last().simulate("mouseenter");
assert.lengthOf(root.find(`.${Classes.TOOLTIP}`), 0);
});

it("does not show popover on click", () => {
root.find(`.${Classes.POPOVER_TARGET}`).last().simulate("click");
assert.lengthOf(root.find(`.${Classes.POPOVER}`), 0);
});

it("the target is not focusable", () => {
assertTargetElementTabIndex(false, root.last().find(BUTTON_ID_SELECTOR).hostNodes().getDOMNode());
});
});
});

describe("Popper.js integration", () => {
Expand All @@ -710,7 +781,7 @@ describe("<Popover>", () => {

it("matches target width via custom modifier", () => {
wrapper = renderPopover({ matchTargetWidth: true, isOpen: true, placement: "bottom" });
const targetElement = wrapper.find("[data-testid='target-button']").hostNodes().getDOMNode();
const targetElement = wrapper.find(BUTTON_ID_SELECTOR).hostNodes().getDOMNode();
const popoverElement = wrapper.find(`.${Classes.POPOVER}`).hostNodes().getDOMNode();
assert.closeTo(
popoverElement.clientWidth,
Expand Down Expand Up @@ -816,7 +887,7 @@ describe("<Popover>", () => {
describe("Enter key down opens click interaction popover", () => {
it("when autoFocus={true}", done => {
wrapper = renderPopover({ autoFocus: true });
const button = wrapper.find("[data-testid='target-button']").hostNodes();
const button = wrapper.find(BUTTON_ID_SELECTOR).hostNodes();
(button.getDOMNode() as HTMLElement).focus();
button.simulate("keyDown", SPACE_KEYSTROKE);
// Wait for focus to change
Expand All @@ -833,7 +904,7 @@ describe("<Popover>", () => {

it("when autoFocus={false}", done => {
wrapper = renderPopover({ autoFocus: false });
const button = wrapper.find("[data-testid='target-button']").hostNodes();
const button = wrapper.find(BUTTON_ID_SELECTOR).hostNodes();
(button.getDOMNode() as HTMLElement).focus();
button.simulate("keyDown", SPACE_KEYSTROKE);

Expand Down Expand Up @@ -875,6 +946,20 @@ describe("<Popover>", () => {
});
});

function assertPopoverTargetTabIndex(shouldTabIndexExist: boolean, popoverProps: Partial<PopoverProps>) {
wrapper = renderPopover({ ...popoverProps, usePortal: true });
const targetElement = wrapper?.find(BUTTON_ID_SELECTOR).hostNodes().getDOMNode();
assertTargetElementTabIndex(shouldTabIndexExist, targetElement);
}

function assertTargetElementTabIndex(shouldTabIndexExist: boolean, targetElement: Element | undefined) {
if (shouldTabIndexExist) {
assert.equal(targetElement?.getAttribute("tabindex"), "0");
} else {
assert.isNull(targetElement?.getAttribute("tabindex"));
}
}

interface PopoverWrapper extends ReactWrapper<PopoverProps, PopoverState> {
popoverElement: HTMLElement;
targetElement: HTMLElement;
Expand All @@ -890,7 +975,11 @@ describe("<Popover>", () => {
then(next: (wrap: PopoverWrapper) => void, done: Mocha.Done): this;
}

function renderPopover(props: Partial<PopoverProps> = {}, content?: any) {
function renderPopover(
props: Partial<PopoverProps> = {},
content?: any,
children: React.JSX.Element = BUTTON_WITH_TEST_ID,
) {
const contentElement = (
<div tabIndex={0} className="test-content">
Text {content}
Expand All @@ -899,18 +988,15 @@ describe("<Popover>", () => {

wrapper = mount(
<Popover usePortal={false} {...props} hoverCloseDelay={0} hoverOpenDelay={0} content={contentElement}>
<Button data-testid="target-button" text="Target" />
{children}
</Popover>,
{ attachTo: testsContainerElement },
) as PopoverWrapper;

const instance = wrapper.instance() as Popover<React.HTMLProps<HTMLButtonElement>>;
wrapper.popoverElement = instance.popoverElement!;
wrapper.targetElement = instance.targetRef.current!;
wrapper.targetButton = wrapper
.find("[data-testid='target-button']")
.hostNodes()
.getDOMNode<HTMLButtonElement>();
wrapper.targetButton = wrapper.find(BUTTON_ID_SELECTOR).hostNodes().getDOMNode<HTMLButtonElement>();
wrapper.assertFindClass = (className: string, expected = true, msg = className) => {
const actual = wrapper!.findClass(className);
if (expected) {
Expand Down

1 comment on commit 9ee655b

@svc-palantir-github
Copy link

Choose a reason for hiding this comment

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

Do not set `tabIndex` when no popover will show (#6851)

Build artifact links for this commit: documentation | landing | table | demo

This is an automated comment from the deploy-preview CircleCI job.

Please sign in to comment.