Fix portal/popover position and scrolling issues (#326)

palantir · Dec 9, 2016 · 82f35f3 · 82f35f3 · blueprint-bot · Dec 9, 2016
1 parent 41b3ae8
commit 82f35f3
Show file tree

Hide file tree

Showing 5 changed files with 40 additions and 8 deletions.
diff --git a/packages/core/package.json b/packages/core/package.json
@@ -10,7 +10,7 @@
     "dom4": "^1.8",
     "normalize.css": "4.1.1",
     "pure-render-decorator": "^1.1",
-    "tether": "^1.2"
+    "tether": "^1.4"
   },
   "peerDependencies": {
     "react": "^15.0.1 || ^0.14",

diff --git a/packages/core/src/common/tetherUtils.ts b/packages/core/src/common/tetherUtils.ts
@@ -5,6 +5,14 @@
  * and https://github.com/palantir/blueprint/blob/master/PATENTS
  */
 
+// TODO: shim for new option added in Tether 1.4.0
+// https://github.com/DefinitelyTyped/DefinitelyTyped/pull/13142
+declare module "tether" {
+    interface ITetherOptions {
+        bodyElement?: HTMLElement;
+    }
+}
+
 import * as Tether from "tether";
 
 import { Position } from "./position";
@@ -14,6 +22,15 @@ const DEFAULT_CONSTRAINTS = {
     to: "scrollParent",
 };
 
+// per https://github.com/HubSpot/tether/pull/204, Tether now exposes a `bodyElement` option that,
+// when present, gets the tethered element injected into *it* instead of into the document body.
+// but both approaches still cause React to freak out, because it loses its handle on the DOM
+// element. thus, we pass a fake HTML bodyElement to Tether, with a no-op `appendChild` function
+// (the only function the library uses from bodyElement).
+const fakeHtmlElement = ({
+    appendChild : () => { /* No-op */ },
+} as any) as HTMLElement;
+
 export interface ITetherConstraint {
     attachment?: string;
     outOfBoundsClass?: string;
@@ -34,6 +51,7 @@ export function createTetherOptions(element: Element,
 
     const options: Tether.ITetherOptions = {
         attachment: getPopoverAttachment(position),
+        bodyElement: fakeHtmlElement,
         classPrefix: "pt-tether",
         constraints,
         element,

diff --git a/packages/core/src/components/overlay/overlay.tsx b/packages/core/src/components/overlay/overlay.tsx
@@ -160,10 +160,12 @@ export class Overlay extends React.Component<IOverlayProps, IOverlayState> {
         const { children, className, inline, isOpen, transitionDuration, transitionName } = this.props;
 
         // add a special class to each child that will automatically set the appropriate
-        // CSS position mode under the hood.
+        // CSS position mode under the hood. also, make the container focusable so we can
+        // trap focus inside it (via `persistentFocus()`).
         const decoratedChildren = React.Children.map(children, (child: React.ReactElement<any>) => {
             return React.cloneElement(child, {
                 className: classNames(child.props.className, Classes.OVERLAY_CONTENT),
+                tabIndex: 0,
             });
         });
 
@@ -188,8 +190,6 @@ export class Overlay extends React.Component<IOverlayProps, IOverlayState> {
         const elementProps = {
             className: mergedClassName,
             onKeyDown: this.handleKeyDown,
-            // make the container focusable so we can trap focus inside it (via `persistentFocus()`)
-            tabIndex: 0,
         };
 
         if (inline) {
@@ -298,11 +298,12 @@ export class Overlay extends React.Component<IOverlayProps, IOverlayState> {
         const isFocusOutsideModal = !containerElement.contains(document.activeElement);
         if (isFocusOutsideModal) {
             // element marked autofocus has higher priority than the other clowns
-            let autofocusElement = containerElement.query("[autofocus]") as HTMLElement;
+            const autofocusElement = containerElement.query("[autofocus]") as HTMLElement;
+            const wrapperElement = containerElement.query("[tabindex]") as HTMLElement;
             if (autofocusElement != null) {
                 autofocusElement.focus();
-            } else {
-                containerElement.focus();
+            } else if (wrapperElement != null) {
+                wrapperElement.focus();
             }
         }
     }

diff --git a/packages/core/src/components/portal/_portal.scss b/packages/core/src/components/portal/_portal.scss
@@ -36,3 +36,16 @@ The children of a `Portal` component are appended to the `<body>` element.
 
 Styleguide components.portal.js
 */
+
+.pt-portal {
+  // take the portal out of the document flow to prevent browsers from autoscrolling to the bottom
+  // of the document (where portals are appended) when programmatically focusing within a portal
+  // child element. also, don't use `fixed`, because then Tether'd elements won't reposition
+  // themselves properly as the document scrolls.
+  position: absolute;
+  // ensure content still offsets from the top of the document
+  top: 0;
+  // ensure content won't be horizontally scrunched
+  right: 0;
+  left: 0;
+}
diff --git a/packages/core/test/overlay/overlayTests.tsx b/packages/core/test/overlay/overlayTests.tsx
@@ -164,7 +164,7 @@ describe("<Overlay>", () => {
                 </Overlay>,
                 { attachTo: testsContainerElement },
             );
-            assert.equal(document.body.querySelector(".pt-overlay-open"), document.activeElement);
+            assert.equal(document.body.querySelector(".pt-overlay-backdrop"), document.activeElement);
         });
 
         it("does not bring focus to overlay if autoFocus=false", () => {