From 82f35f3f938d372f7f4366048ef6e529b9df6040 Mon Sep 17 00:00:00 2001 From: Chris Lewis Date: Fri, 9 Dec 2016 14:28:36 -0800 Subject: [PATCH] Fix portal/popover position and scrolling issues (#326) --- packages/core/package.json | 2 +- packages/core/src/common/tetherUtils.ts | 18 ++++++++++++++++++ .../core/src/components/overlay/overlay.tsx | 13 +++++++------ .../core/src/components/portal/_portal.scss | 13 +++++++++++++ packages/core/test/overlay/overlayTests.tsx | 2 +- 5 files changed, 40 insertions(+), 8 deletions(-) diff --git a/packages/core/package.json b/packages/core/package.json index af704c77f11..ab815c2574f 100644 --- 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 index b9d229bd9f0..0f9f21f6c5d 100644 --- 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 index 41016171e92..30b5cb95c84 100644 --- 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 { 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) => { return React.cloneElement(child, { className: classNames(child.props.className, Classes.OVERLAY_CONTENT), + tabIndex: 0, }); }); @@ -188,8 +190,6 @@ export class Overlay extends React.Component { 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 { 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 index fe6b3e4d4f1..60629221a19 100644 --- 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 `` 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 index 0a58fd1fac3..e485b301410 100644 --- a/packages/core/test/overlay/overlayTests.tsx +++ b/packages/core/test/overlay/overlayTests.tsx @@ -164,7 +164,7 @@ describe("", () => { , { 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", () => {