[Dialog] fix eager closing (#2756)

* replace Dialog's broken canOutsideClickClose with pointer-events * add type="button" to close button * remove obsolete test
palantir · Aug 3, 2018 · 1c1675b · 1c1675b · blueprint-bot · Aug 3, 2018
1 parent f3a45fb
commit 1c1675b
Show file tree

Hide file tree

Showing 3 changed files with 9 additions and 29 deletions.
diff --git a/packages/core/src/components/dialog/_dialog.scss b/packages/core/src/components/dialog/_dialog.scss
@@ -57,14 +57,8 @@ $dialog-padding: $pt-grid-size * 2 !default;
   justify-content: center;
   width: 100%;
   min-height: 100%;
+  pointer-events: none;
   user-select: none;
-
-  // LEGACY: override old (<= 1.24.0) dialog styles when inside a container to respect flex layout
-  .#{$ns}-dialog {
-    // stylelint-disable-next-line declaration-no-important
-    position: static !important;
-    transform: none;
-  }
 }
 
 .#{$ns}-dialog {
@@ -76,6 +70,7 @@ $dialog-padding: $pt-grid-size * 2 !default;
   background: $light-gray4;
   width: $pt-grid-size * 50;
   padding-bottom: $pt-grid-size * 2;
+  pointer-events: all;
   user-select: text;
 
   &:focus {

diff --git a/packages/core/src/components/dialog/dialog.tsx b/packages/core/src/components/dialog/dialog.tsx
@@ -11,7 +11,6 @@ import { AbstractPureComponent } from "../../common/abstractPureComponent";
 import * as Classes from "../../common/classes";
 import * as Errors from "../../common/errors";
 import { DISPLAYNAME_PREFIX, IProps } from "../../common/props";
-import { safeInvoke } from "../../common/utils";
 import { H4 } from "../html/html";
 import { Icon, IconName } from "../icon/icon";
 import { IBackdropProps, IOverlayableProps, Overlay } from "../overlay/overlay";
@@ -73,7 +72,7 @@ export class Dialog extends AbstractPureComponent<IDialogProps, {}> {
     public render() {
         return (
             <Overlay {...this.props} className={Classes.OVERLAY_SCROLL_CONTAINER} hasBackdrop={true}>
-                <div className={Classes.DIALOG_CONTAINER} onMouseDown={this.handleContainerMouseDown}>
+                <div className={Classes.DIALOG_CONTAINER}>
                     <div className={classNames(Classes.DIALOG, this.props.className)} style={this.props.style}>
                         {this.maybeRenderHeader()}
                         {this.props.children}
@@ -99,7 +98,12 @@ export class Dialog extends AbstractPureComponent<IDialogProps, {}> {
         // this gives us a behavior as if the default value were `true`
         if (this.props.isCloseButtonShown !== false) {
             return (
-                <button aria-label="Close" className={Classes.DIALOG_CLOSE_BUTTON} onClick={this.props.onClose}>
+                <button
+                    aria-label="Close"
+                    className={Classes.DIALOG_CLOSE_BUTTON}
+                    onClick={this.props.onClose}
+                    type="button"
+                >
                     <Icon icon="small-cross" iconSize={Icon.SIZE_LARGE} />
                 </button>
             );
@@ -121,12 +125,4 @@ export class Dialog extends AbstractPureComponent<IDialogProps, {}> {
             </div>
         );
     }
-
-    private handleContainerMouseDown = (evt: React.MouseEvent<HTMLDivElement>) => {
-        // quick re-implementation of canOutsideClickClose because DIALOG_CONTAINER covers the backdrop
-        const isClickOutsideDialog = (evt.target as HTMLElement).closest(`.${Classes.DIALOG}`) == null;
-        if (isClickOutsideDialog && this.props.canOutsideClickClose) {
-            safeInvoke(this.props.onClose, evt);
-        }
-    };
 }
diff --git a/packages/core/test/dialog/dialogTests.tsx b/packages/core/test/dialog/dialogTests.tsx
@@ -42,17 +42,6 @@ describe("<Dialog>", () => {
         assert.isTrue(onClose.calledOnce);
     });
 
-    it("attempts to close when dialog container element is moused down", () => {
-        const onClose = spy();
-        const dialog = mount(
-            <Dialog isOpen={true} onClose={onClose} usePortal={false}>
-                {createDialogContents()}
-            </Dialog>,
-        );
-        dialog.find(`.${Classes.DIALOG_CONTAINER}`).simulate("mousedown");
-        assert.isTrue(onClose.calledOnce);
-    });
-
     it("doesn't close when canOutsideClickClose=false and overlay backdrop element is moused down", () => {
         const onClose = spy();
         const dialog = mount(