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

[popover2] fix(ContextMenu2): Tooltip2 and autofocus interactions #4744

Merged
merged 4 commits into from
Jun 3, 2021
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
49 changes: 28 additions & 21 deletions packages/popover2/src/contextMenu2.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ import {
import * as Classes from "./classes";
import { Popover2Props, Popover2 } from "./popover2";
import { Popover2TargetProps } from "./popover2SharedProps";
import { Tooltip2Context, Tooltip2Provider } from "./tooltip2Context";

type Offset = {
left: number;
Expand Down Expand Up @@ -129,6 +130,9 @@ export const ContextMenu2: React.FC<ContextMenu2Props> = React.forwardRef<any, C
...restProps
} = props;

// ancestor Tooltip2Context state doesn't affect us since we don't care about parent ContextMenu2s, we only want to
// force disable parent Tooltip2s in certain cases through dispatching actions
const [, popover2Dispatch] = React.useContext(Tooltip2Context);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
const [, popover2Dispatch] = React.useContext(Tooltip2Context);
const [, tooltip2Dispatch] = React.useContext(Tooltip2Context);

should be renamed here and below...

// click target offset relative to the viewport (e.clientX/clientY), since the target will be rendered in a Portal
const [targetOffset, setTargetOffset] = React.useState<Offset | undefined>(undefined);
// hold a reference to the click mouse event to pass to content/child render functions
Expand All @@ -141,6 +145,7 @@ export const ContextMenu2: React.FC<ContextMenu2Props> = React.forwardRef<any, C
// for this component (that will lead to unpredictable behavior).
React.useEffect(() => {
setIsOpen(false);
popover2Dispatch({ type: "RESET_DISABLED_STATE" });
}, [disabled]);

const cancelContextMenu = React.useCallback((e: React.SyntheticEvent<HTMLDivElement>) => e.preventDefault(), []);
Expand All @@ -149,6 +154,7 @@ export const ContextMenu2: React.FC<ContextMenu2Props> = React.forwardRef<any, C
if (!nextOpenState) {
setIsOpen(false);
setMouseEvent(undefined);
popover2Dispatch({ type: "RESET_DISABLED_STATE" });
}
}, []);

Expand Down Expand Up @@ -176,7 +182,6 @@ export const ContextMenu2: React.FC<ContextMenu2Props> = React.forwardRef<any, C
menu === undefined ? undefined : (
<Popover2
{...popoverProps}
autoFocus={false}
content={
// this prevents right-clicking inside our context menu
<div onContextMenu={cancelContextMenu}>{menu}</div>
Expand Down Expand Up @@ -218,6 +223,7 @@ export const ContextMenu2: React.FC<ContextMenu2Props> = React.forwardRef<any, C
setMouseEvent(e);
setTargetOffset({ left: e.clientX, top: e.clientY });
setIsOpen(true);
popover2Dispatch({ type: "FORCE_DISABLED_STATE" });
}

onContextMenu?.(e);
Expand All @@ -227,26 +233,27 @@ export const ContextMenu2: React.FC<ContextMenu2Props> = React.forwardRef<any, C

const containerClassName = classNames(className, Classes.CONTEXT_MENU2);

if (CoreUtils.isFunction(children)) {
return children({
className: containerClassName,
contentProps,
onContextMenu: handleContextMenu,
popover: maybePopover,
});
} else {
return React.createElement(
tagName,
{
className: containerClassName,
onContextMenu: handleContextMenu,
ref: userRef,
...restProps,
},
maybePopover,
children,
);
}
const child = CoreUtils.isFunction(children)
? children({
className: containerClassName,
contentProps,
onContextMenu: handleContextMenu,
popover: maybePopover,
})
: React.createElement(
tagName,
{
className: containerClassName,
onContextMenu: handleContextMenu,
ref: userRef,
...restProps,
},
maybePopover,
children,
);

// force descendant Tooltip2s to be disabled when this context menu is open
return <Tooltip2Provider initialState={{ forceDisabled: isOpen }}>{child}</Tooltip2Provider>;
});
ContextMenu2.displayName = "Blueprint.ContextMenu2";

Expand Down
29 changes: 21 additions & 8 deletions packages/popover2/src/tooltip2.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import * as Classes from "./classes";
import { Popover2, Popover2InteractionKind } from "./popover2";
import { TOOLTIP_ARROW_SVG_SIZE } from "./popover2Arrow";
import { Popover2SharedProps } from "./popover2SharedProps";
import { Tooltip2Context, Tooltip2ContextState, Tooltip2Provider } from "./tooltip2Context";

// eslint-disable-next-line deprecation/deprecation
export type Tooltip2Props<TProps = React.HTMLProps<HTMLElement>> = ITooltip2Props<TProps>;
Expand Down Expand Up @@ -87,7 +88,24 @@ export class Tooltip2<T> extends React.PureComponent<Tooltip2Props<T>> {
private popover: Popover2<T> | null = null;

public render() {
const { children, intent, popoverClassName, ...restProps } = this.props;
// if we have an ancestor Tooltip2Context, we should take its state into account in this render path,
// it was likely created by a parent ContextMenu2
return (
<Tooltip2Context.Consumer>
{([state]) => <Tooltip2Provider initialState={state}>{this.renderPopover}</Tooltip2Provider>}
Copy link
Contributor

Choose a reason for hiding this comment

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

what happens if you have nested tooltips? (acknowledging that it's not the best UX, but not ruling out that its possibility 😬 )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think it would be a possibility in terms of tooltips nested inside tooltip content, but nesting of tooltip targets could be possible (just like nesting of ContextMenu2 targets, as demonstrated in the ContextMenu2 docs example). I'll add a test for it

Copy link
Contributor

Choose a reason for hiding this comment

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

oops yes, i meant targets, thanks for clarifying!

</Tooltip2Context.Consumer>
);
}

public reposition() {
if (this.popover != null) {
this.popover.reposition();
}
}

// any descendant ContextMenu2s may update this ctxState
private renderPopover = (ctxState: Tooltip2ContextState) => {
const { children, disabled, intent, popoverClassName, ...restProps } = this.props;
const classes = classNames(
Classes.TOOLTIP2,
{ [CoreClasses.MINIMAL]: this.props.minimal },
Expand All @@ -111,6 +129,7 @@ export class Tooltip2<T> extends React.PureComponent<Tooltip2Props<T>> {
{...restProps}
autoFocus={false}
canEscapeKeyClose={false}
disabled={ctxState.forceDisabled ?? disabled}
enforceFocus={false}
lazy={true}
popoverClassName={classes}
Expand All @@ -120,11 +139,5 @@ export class Tooltip2<T> extends React.PureComponent<Tooltip2Props<T>> {
{children}
</Popover2>
);
}

public reposition() {
if (this.popover != null) {
this.popover.reposition();
}
}
};
}
54 changes: 54 additions & 0 deletions packages/popover2/src/tooltip2Context.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
/*
* Copyright 2021 Palantir Technologies, Inc. All rights reserved.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

import * as React from "react";

export interface Tooltip2ContextState {
forceDisabled?: boolean;
}

type Tooltip2Action = { type: "FORCE_DISABLED_STATE" } | { type: "RESET_DISABLED_STATE" };
const noOpDispatch: React.Dispatch<Tooltip2Action> = () => null;

export const Tooltip2Context = React.createContext<[Tooltip2ContextState, React.Dispatch<Tooltip2Action>]>([
{},
noOpDispatch,
]);

const tooltip2Reducer = (state: Tooltip2ContextState, action: Tooltip2Action) => {
switch (action.type) {
case "FORCE_DISABLED_STATE":
return { forceDisabled: true };
case "RESET_DISABLED_STATE":
return {};
default:
return state;
}
};

interface Tooltip2ProviderProps {
children: React.ReactNode | ((ctxState: Tooltip2ContextState) => React.ReactNode);
initialState?: Partial<Tooltip2ContextState>;
}

export const Tooltip2Provider = ({ children, initialState = {} }: Tooltip2ProviderProps) => {
const [state, dispatch] = React.useReducer(tooltip2Reducer, initialState);
return (
<Tooltip2Context.Provider value={[state, dispatch]}>
{typeof children === "function" ? children(state) : children}
</Tooltip2Context.Provider>
);
};
15 changes: 14 additions & 1 deletion packages/popover2/test/contextMenu2Tests.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ import classNames from "classnames";
import { mount, ReactWrapper } from "enzyme";
import * as React from "react";

import { Menu, MenuItem } from "@blueprintjs/core";
import { Classes as CoreClasses, Menu, MenuItem, Keys } from "@blueprintjs/core";

import { Classes, ContextMenu2, ContextMenu2ContentProps, ContextMenu2Props, Popover2, Tooltip2 } from "../src";

Expand Down Expand Up @@ -57,6 +57,19 @@ describe("ContextMenu2", () => {
assert.isTrue(ref.current?.classList.contains("test-container"));
});

it("closes popover on ESC key press", () => {
const ctxMenu = mountTestMenu();
openCtxMenu(ctxMenu);
ctxMenu
.find(`.${CoreClasses.OVERLAY_OPEN}`)
.hostNodes()
.simulate("keydown", {
nativeEvent: new KeyboardEvent("keydown"),
which: Keys.ESCAPE,
});
assert.isFalse(ctxMenu.find(Popover2).prop("isOpen"));
});

function mountTestMenu(props: Partial<ContextMenu2Props> = {}) {
return mount(
<ContextMenu2 content={MENU} popoverProps={{ transitionDuration: 0 }} {...props}>
Expand Down