[core] fix(Breadcrumb,Button,AnchorButton): improve onClick type (#5945)

packages/core/src/components/breadcrumbs/breadcrumb.tsx

@@ -24,7 +24,7 @@ import { Icon } from "../icon/icon";
 // eslint-disable-next-line deprecation/deprecation
 export type BreadcrumbProps = IBreadcrumbProps;
 /** @deprecated use BreadcrumbProps */
-export interface IBreadcrumbProps extends ActionProps, LinkProps {
+export interface IBreadcrumbProps extends ActionProps<HTMLAnchorElement>, LinkProps {
     children?: React.ReactNode;
 
     /** Whether this breadcrumb is the current breadcrumb. */
@@ -68,6 +68,7 @@ export const Breadcrumb: React.FC<BreadcrumbProps> = props => {
             className={classes}
             href={props.href}
             onClick={props.disabled ? undefined : props.onClick}
+            onFocus={props.disabled ? undefined : props.onFocus}
             tabIndex={props.disabled ? undefined : 0}
             target={props.target}
         >

packages/core/src/components/button/abstractButton.tsx

@@ -30,11 +30,21 @@ import {
 import { Icon, IconName, IconSize } from "../icon/icon";
 import { Spinner } from "../spinner/spinner";
 
-// eslint-disable-next-line deprecation/deprecation
-export type ButtonProps<E extends HTMLButtonElement | HTMLAnchorElement = HTMLButtonElement> = IButtonProps<E>;
 /** @deprecated use ButtonProps */
-export interface IButtonProps<E extends HTMLButtonElement | HTMLAnchorElement = HTMLButtonElement>
-    extends ActionProps,
+export type IButtonProps<E extends HTMLButtonElement | HTMLAnchorElement = HTMLButtonElement> = ButtonProps<E>;
+
+/**
+ * Props interface for both the Button and AnchorButton components.
+ *
+ * Note that it is useful for the props for the two components to be assignable to each other, which we can allow
+ * by omitting the `elementRef` prop as `DialogStepButton` does. This is mostly for backwards compatibility, but it is
+ * a feature we like to preserve because the components are so similar and distinguishing between them in their event
+ * handlers is usually unnecessary. For this reason, we extend `ActionProps<HTMLElement>` rather than `ActionProps<E>`.
+ *
+ * @see {@link ActionProps}
+ */
+export interface ButtonProps<E extends HTMLButtonElement | HTMLAnchorElement = HTMLButtonElement>
+    extends ActionProps<HTMLElement>,
         // eslint-disable-next-line deprecation/deprecation
         IElementRefProps<E> {
     /**
@@ -156,6 +166,7 @@ export abstract class AbstractButton<E extends HTMLButtonElement | HTMLAnchorEle
             disabled,
             onBlur: this.handleBlur,
             onClick: disabled ? undefined : this.props.onClick,
+            onFocus: disabled ? undefined : this.props.onFocus,
             onKeyDown: this.handleKeyDown,
             onKeyUp: this.handleKeyUp,
             tabIndex: disabled ? -1 : tabIndex,

packages/core/src/components/button/button.md

@@ -56,7 +56,7 @@ The two button components each support arbitrary HTML props for their underlying
 DOM element (`<button>` and `<a>` respectively). Specifying an HTML prop will
 override the component's default for it, such as `role` on `<AnchorButton>`.
 
-@interface IButtonProps
+@interface ButtonProps
 
 @## CSS
 

packages/core/test/alert/alertTests.tsx

@@ -19,7 +19,7 @@ import { mount, shallow, ShallowWrapper } from "enzyme";
 import * as React from "react";
 import { SinonStub, spy, stub } from "sinon";
 
-import { Alert, Button, Classes, IAlertProps, IButtonProps, Icon, Intent } from "../../src";
+import { Alert, AlertProps, Button, ButtonProps, Classes, Icon, Intent } from "../../src";
 import * as Errors from "../../src/common/errors";
 import { findInPortal } from "../utils";
 
@@ -85,7 +85,7 @@ describe("<Alert>", () => {
     describe("confirm button", () => {
         const onConfirm = spy();
         const onClose = spy();
-        let wrapper: ShallowWrapper<IAlertProps, any>;
+        let wrapper: ShallowWrapper<AlertProps, any>;
 
         beforeEach(() => {
             onConfirm.resetHistory();
@@ -126,8 +126,8 @@ describe("<Alert>", () => {
     describe("cancel button", () => {
         const onCancel = spy();
         const onClose = spy();
-        let wrapper: ShallowWrapper<IAlertProps, any>;
-        let cancelButton: ShallowWrapper<IButtonProps, any>;
+        let wrapper: ShallowWrapper<AlertProps, any>;
+        let cancelButton: ShallowWrapper<ButtonProps, any>;
 
         beforeEach(() => {
             onCancel.resetHistory();
@@ -167,7 +167,7 @@ describe("<Alert>", () => {
         });
 
         it("canEscapeKeyCancel enables escape key", () => {
-            const alert = mount<IAlertProps>(
+            const alert = mount<AlertProps>(
                 <Alert isOpen={true} cancelButtonText="Cancel" confirmButtonText="Delete" onCancel={onCancel}>
                     <p>Are you sure you want to delete this file?</p>
                     <p>There is no going back.</p>
@@ -186,7 +186,7 @@ describe("<Alert>", () => {
         });
 
         it("canOutsideClickCancel enables outside click", () => {
-            const alert = mount<IAlertProps>(
+            const alert = mount<AlertProps>(
                 <Alert isOpen={true} cancelButtonText="Cancel" confirmButtonText="Delete" onCancel={onCancel}>
                     <p>Are you sure you want to delete this file?</p>
                     <p>There is no going back.</p>
@@ -206,9 +206,9 @@ describe("<Alert>", () => {
     });
 
     describe("load state", () => {
-        let wrapper: ShallowWrapper<IAlertProps, any>;
-        let findCancelButton: () => ShallowWrapper<IButtonProps, any>;
-        let findSubmitButton: () => ShallowWrapper<IButtonProps, any>;
+        let wrapper: ShallowWrapper<AlertProps, any>;
+        let findCancelButton: () => ShallowWrapper<ButtonProps, any>;
+        let findSubmitButton: () => ShallowWrapper<ButtonProps, any>;
 
         beforeEach(() => {
             wrapper = shallow(

packages/core/test/buttons/abstractButtonTests.tsx

@@ -0,0 +1,42 @@
+/*
+ * Copyright 2023 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 { shallow } from "enzyme";
+import * as React from "react";
+
+import { AnchorButton, Button, ButtonProps } from "../../src";
+
+describe("ButtonProps", () => {
+    describe("(without elementRef) should be assignable to", () => {
+        const buttonProps: Omit<ButtonProps, "elementRef"> = {
+            active: true,
+            alignText: "left",
+            fill: true,
+            onClick: (_event: React.MouseEvent<HTMLElement>) => {
+                /* no-op */
+            },
+            outlined: true,
+        };
+
+        it("<Button> component", () => {
+            shallow(<Button {...buttonProps} />);
+        });
+
+        it("<AnchorButton> component", () => {
+            shallow(<AnchorButton {...buttonProps} />);
+        });
+    });
+});

packages/core/test/buttons/buttonTests.tsx

@@ -19,7 +19,7 @@ import { mount, shallow } from "enzyme";
 import * as React from "react";
 import { spy } from "sinon";
 
-import { AnchorButton, Button, Classes, IButtonProps, Icon, Spinner } from "../../src";
+import { AnchorButton, Button, ButtonProps, Classes, Icon, Spinner } from "../../src";
 import * as Keys from "../../src/common/keys";
 
 describe("Buttons:", () => {
@@ -193,14 +193,14 @@ function buttonTestSuite(component: React.ComponentClass<any>, tagName: string)
             });
         }
 
-        function button(props: IButtonProps, useMount = false, ...children: React.ReactNode[]) {
+        function button(props: ButtonProps, useMount = false, ...children: React.ReactNode[]) {
             const element = React.createElement(component, props, ...children);
             return useMount ? mount(element) : shallow(element);
         }
 
         function checkClickTriggeredOnKeyUp(
             done: Mocha.Done,
-            buttonProps: Partial<IButtonProps>,
+            buttonProps: Partial<ButtonProps>,
             keyEventProps: Partial<React.KeyboardEvent<any>>,
         ) {
             const wrapper = button(buttonProps, true);
@@ -222,7 +222,7 @@ function buttonTestSuite(component: React.ComponentClass<any>, tagName: string)
         function checkKeyEventCallbackInvoked(callbackPropName: string, eventName: string, keyCode: number) {
             const callback = spy();
 
-            // IButtonProps doesn't include onKeyDown or onKeyUp in its
+            // ButtonProps doesn't include onKeyDown or onKeyUp in its
             // definition, even though Buttons support those props. Casting as
             // `any` gets around that for the purpose of these tests.
             const wrapper = button({ [callbackPropName]: callback } as any);

packages/core/test/menu/menuItemTests.tsx

@@ -39,11 +39,14 @@ describe("MenuItem", () => {
     });
 
     it("supports HTML props", () => {
-        const func = () => false;
-        const item = shallow(<MenuItem text="text" onClick={func} onKeyDown={func} onMouseMove={func} />).find("a");
-        assert.strictEqual(item.prop("onClick"), func);
-        assert.strictEqual(item.prop("onKeyDown"), func);
-        assert.strictEqual(item.prop("onMouseMove"), func);
+        const mouseHandler = (_event: React.MouseEvent<HTMLElement>) => false;
+        const keyHandler = (_event: React.KeyboardEvent<HTMLElement>) => false;
+        const item = shallow(
+            <MenuItem text="text" onClick={mouseHandler} onKeyDown={keyHandler} onMouseMove={mouseHandler} />,
+        ).find("a");
+        assert.strictEqual(item.prop("onClick"), mouseHandler);
+        assert.strictEqual(item.prop("onKeyDown"), keyHandler);
+        assert.strictEqual(item.prop("onMouseMove"), mouseHandler);
     });
 
     it("children appear in submenu", () => {