From a997399b60229c12e5b768025d2452464f5667a9 Mon Sep 17 00:00:00 2001 From: Chandler Prall Date: Tue, 18 Sep 2018 12:41:28 -0600 Subject: [PATCH 1/4] Force popover contents to stay in the same position side once open, unless window resizes --- src/components/popover/popover.js | 46 +++++++++++++------ src/services/popover/popover_positioning.js | 27 ++++++----- .../popover/popover_positioning.test.js | 26 +++++++++++ 3 files changed, 74 insertions(+), 25 deletions(-) diff --git a/src/components/popover/popover.js b/src/components/popover/popover.js index 12dd244dc43..741369b0a59 100644 --- a/src/components/popover/popover.js +++ b/src/components/popover/popover.js @@ -115,6 +115,7 @@ export class EuiPopover extends Component { popoverStyles: DEFAULT_POPOVER_STYLES, arrowStyles: {}, arrowPosition: null, + openPosition: null, }; } @@ -154,7 +155,7 @@ export class EuiPopover extends Component { } if (this.props.repositionOnScroll) { - window.addEventListener('scroll', this.positionPopover); + window.addEventListener('scroll', this.positionPopoverFixed); } this.updateFocus(); @@ -176,9 +177,9 @@ export class EuiPopover extends Component { // update scroll listener if (prevProps.repositionOnScroll !== this.props.repositionOnScroll) { if (this.props.repositionOnScroll) { - window.addEventListener('scroll', this.positionPopover); + window.addEventListener('scroll', this.positionPopoverFixed); } else { - window.removeEventListener('scroll', this.positionPopover); + window.removeEventListener('scroll', this.positionPopoverFixed); } } @@ -197,7 +198,7 @@ export class EuiPopover extends Component { } componentWillUnmount() { - window.removeEventListener('scroll', this.positionPopover); + window.removeEventListener('scroll', this.positionPopoverFixed); clearTimeout(this.closingTransitionTimeout); } @@ -223,14 +224,14 @@ export class EuiPopover extends Component { }, 0 ); - this.positionPopover(); + this.positionPopoverFixed(); if (waitDuration > 0) { const startTime = Date.now(); const endTime = startTime + waitDuration; const onFrame = () => { - this.positionPopover(); + this.positionPopoverFixed(); if (endTime > Date.now()) { requestAnimationFrame(onFrame); @@ -241,12 +242,20 @@ export class EuiPopover extends Component { } } - positionPopover = () => { + positionPopover = allowEnforcePosition => { if (this.button == null || this.panel == null) return; - const { top, left, position, arrow } = findPopoverPosition({ + let position = getPopoverPositionFromAnchorPosition(this.props.anchorPosition); + let forcePosition = null; + if (allowEnforcePosition && this.state.openPosition != null) { + position = this.state.openPosition; + forcePosition = true; + } + + const { top, left, position: foundPosition, arrow } = findPopoverPosition({ container: this.props.container, - position: getPopoverPositionFromAnchorPosition(this.props.anchorPosition), + position, + forcePosition, align: getPopoverAlignFromAnchorPosition(this.props.anchorPosition), anchor: this.button, popover: this.panel, @@ -270,9 +279,17 @@ export class EuiPopover extends Component { }; const arrowStyles = this.props.hasArrow ? arrow : null; - const arrowPosition = position; + const arrowPosition = foundPosition; + + this.setState({ popoverStyles, arrowStyles, arrowPosition, openPosition: foundPosition }); + } + + positionPopoverFixed = () => { + this.positionPopover(true); + } - this.setState({ popoverStyles, arrowStyles, arrowPosition }); + positionPopoverFluid = () => { + this.positionPopover(false); } panelRef = node => { @@ -284,12 +301,13 @@ export class EuiPopover extends Component { popoverStyles: DEFAULT_POPOVER_STYLES, arrowStyles: {}, arrowPosition: null, + openPosition: null, }); - window.removeEventListener('resize', this.positionPopover); + window.removeEventListener('resize', this.positionPopoverFluid); } else { // panel is coming into existence - this.positionPopover(); - window.addEventListener('resize', this.positionPopover); + this.positionPopoverFluid(); + window.addEventListener('resize', this.positionPopoverFluid); } }; diff --git a/src/services/popover/popover_positioning.js b/src/services/popover/popover_positioning.js index 9ca983c5a9a..ef2893f1a4b 100644 --- a/src/services/popover/popover_positioning.js +++ b/src/services/popover/popover_positioning.js @@ -34,6 +34,7 @@ const positionSubstitutes = { * @param anchor {HTMLElement|React.Component} Element to anchor the popover to * @param popover {HTMLElement|React.Component} Element containing the popover content * @param position {string} Position the user wants. One of ["top", "right", "bottom", "left"] + * @param [forcePosition] {boolean} If true, use only the provided `position` value and don't try any other position * @param [align] {string} Cross-axis alignment. One of ["top", "right", "bottom", "left"] * @param [buffer=16] {number} Minimum distance between the popover and the bounding container * @param [offset=0] {number} Distance between the popover and the anchor @@ -49,6 +50,7 @@ export function findPopoverPosition({ popover, align, position, + forcePosition, buffer = 16, offset = 0, allowCrossAxis = true, @@ -95,17 +97,20 @@ export function findPopoverPosition({ * if position = "right" the order is right, left, top, bottom */ - const iterationPositions = [ - position, // Try the user-desired position first. - positionComplements[position], // Try the complementary position. - ]; - const iterationAlignments = [align, align]; // keep user-defined alignment in the original and complementary positions - if (allowCrossAxis) { - iterationPositions.push( - positionSubstitutes[position], // Switch to the cross axis. - positionComplements[positionSubstitutes[position]] // Try the complementary position on the cross axis. - ); - iterationAlignments.push(null, null); // discard desired alignment on cross-axis + const iterationPositions = [position]; // Try the user-desired position first. + const iterationAlignments = [align]; // keep user-defined alignment in the original positions. + + if (forcePosition !== true) { + iterationPositions.push(positionComplements[position]); // Try the complementary position. + iterationAlignments.push(align); // keep user-defined alignment in the complementary position. + + if (allowCrossAxis) { + iterationPositions.push( + positionSubstitutes[position], // Switch to the cross axis. + positionComplements[positionSubstitutes[position]] // Try the complementary position on the cross axis. + ); + iterationAlignments.push(null, null); // discard desired alignment on cross-axis + } } const { diff --git a/src/services/popover/popover_positioning.test.js b/src/services/popover/popover_positioning.test.js index 1401a145bb0..6ed6c225b81 100644 --- a/src/services/popover/popover_positioning.test.js +++ b/src/services/popover/popover_positioning.test.js @@ -499,6 +499,32 @@ describe('popover_positioning', () => { left: 85 }); }); + + it('respects forcePosition value', () => { + const anchor = document.createElement('div'); + anchor.getBoundingClientRect = () => makeBB(100, 150, 120, 50); + + const popover = document.createElement('div'); + popover.getBoundingClientRect = () => makeBB(0, 30, 50, 0); + + // give the container limited space on both left and right, forcing to top + const container = document.createElement('div'); + container.getBoundingClientRect = () => makeBB(0, 160, 768, 40); + + expect(findPopoverPosition({ + position: 'right', + forcePosition: true, + anchor, + popover, + container, + offset: 5 + })).toEqual({ + fit: 0, + position: 'right', + top: 85, + left: 155 + }); + }); }); describe('placement falls back to second complementary position', () => { From a07f2e437396c13de7b32e85c92da2680d061fb4 Mon Sep 17 00:00:00 2001 From: Chandler Prall Date: Tue, 18 Sep 2018 12:50:17 -0600 Subject: [PATCH 2/4] changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index aa93664bbaf..55b4cced1dd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,7 @@ - Fixed issue with unselected tabs and aria-controls attribute in EuiTabbedContent - Added `tag` icon ([#1188](https://github.com/elastic/eui/pull/1188)) - Replaced `logging` app icon ([#1194](https://github.com/elastic/eui/pull/1194)) +- Force `EuiPopover` contents to stick to its initial position when the content changes ([#1199](https://github.com/elastic/eui/pull/1199)) **Bug fixes** From 9ea47188e3addbd9c166205a757c807204bc16aa Mon Sep 17 00:00:00 2001 From: Chandler Prall Date: Tue, 25 Sep 2018 12:15:25 -0600 Subject: [PATCH 3/4] Make EuiPopover wait until any initial css transitions on its content have finished before locking the position; Fix popover positioning logic to disregard invalid alignments when forcing a position --- src/components/popover/popover.js | 54 ++++++++++++++++----- src/services/popover/popover_positioning.js | 7 +++ 2 files changed, 49 insertions(+), 12 deletions(-) diff --git a/src/components/popover/popover.js b/src/components/popover/popover.js index ed825afd204..fa168cddbd5 100644 --- a/src/components/popover/popover.js +++ b/src/components/popover/popover.js @@ -83,6 +83,20 @@ function getElementFromInitialFocus(initialFocus) { return initialFocus; } +function getTransitionTimings(element) { + const computedStyle = window.getComputedStyle(element); + + const computedDuration = computedStyle.getPropertyValue('transition-duration'); + let durationMatch = computedDuration.match(GROUP_NUMERIC); + durationMatch = durationMatch ? parseFloat(durationMatch[1]) * 1000 : 0; + + const computedDelay = computedStyle.getPropertyValue('transition-delay'); + let delayMatch = computedDelay.match(GROUP_NUMERIC); + delayMatch = delayMatch ? parseFloat(delayMatch[1]) * 1000 : 0; + + return { durationMatch, delayMatch }; +} + export class EuiPopover extends Component { static getDerivedStateFromProps(nextProps, prevState) { if (prevState.prevProps.isOpen && !nextProps.isOpen) { @@ -122,7 +136,8 @@ export class EuiPopover extends Component { popoverStyles: DEFAULT_POPOVER_STYLES, arrowStyles: {}, arrowPosition: null, - openPosition: null, + openPosition: null, // once a stable position has been found, keep the contents on that side + isOpenStable: false, // wait for any initial opening transitions to finish before marking as stable }; } @@ -194,6 +209,29 @@ export class EuiPopover extends Component { isOpening: true, }); }); + + // for each child element of `this.panel`, find any transition duration we should wait for before stabilizing + const { durationMatch, delayMatch } = Array.prototype.slice.call(this.panel.children).reduce( + ({ durationMatch, delayMatch }, element) => { + const transitionTimings = getTransitionTimings(element); + + return { + durationMatch: Math.max(durationMatch, transitionTimings.durationMatch), + delayMatch: Math.max(delayMatch, transitionTimings.delayMatch), + }; + }, + { durationMatch: 0, delayMatch: 0 } + ); + + setTimeout( + () => { + this.setState( + { isOpenStable: true }, + this.positionPopoverFixed + ); + }, + (durationMatch + delayMatch) + ); } // update scroll listener @@ -229,16 +267,7 @@ export class EuiPopover extends Component { (waitDuration, record) => { // only check for CSS transition values for ELEMENT nodes if (record.target.nodeType === document.ELEMENT_NODE) { - const computedStyle = window.getComputedStyle(record.target); - - const computedDuration = computedStyle.getPropertyValue('transition-duration'); - let durationMatch = computedDuration.match(GROUP_NUMERIC); - durationMatch = durationMatch ? parseFloat(durationMatch[1]) * 1000 : 0; - - const computedDelay = computedStyle.getPropertyValue('transition-delay'); - let delayMatch = computedDelay.match(GROUP_NUMERIC); - delayMatch = delayMatch ? parseFloat(delayMatch[1]) * 1000 : 0; - + const { durationMatch, delayMatch } = getTransitionTimings(record.target); waitDuration = Math.max(waitDuration, durationMatch + delayMatch); } @@ -269,7 +298,7 @@ export class EuiPopover extends Component { let position = getPopoverPositionFromAnchorPosition(this.props.anchorPosition); let forcePosition = null; - if (allowEnforcePosition && this.state.openPosition != null) { + if (allowEnforcePosition && this.state.isOpenStable && this.state.openPosition != null) { position = this.state.openPosition; forcePosition = true; } @@ -324,6 +353,7 @@ export class EuiPopover extends Component { arrowStyles: {}, arrowPosition: null, openPosition: null, + isOpenStable: false, }); window.removeEventListener('resize', this.positionPopoverFluid); } else { diff --git a/src/services/popover/popover_positioning.js b/src/services/popover/popover_positioning.js index ef2893f1a4b..d19b17169f0 100644 --- a/src/services/popover/popover_positioning.js +++ b/src/services/popover/popover_positioning.js @@ -111,6 +111,13 @@ export function findPopoverPosition({ ); iterationAlignments.push(null, null); // discard desired alignment on cross-axis } + } else { + // position is forced, if it conficts with the alignment then reset align to `null` + // e.g. original placement request for `downLeft` is moved to the `left` side, future calls + // will position and align `left`, and `leftLeft` is not a valid placement + if (position === align || position === positionComplements[align]) { + iterationAlignments[0] = null; + } } const { From 8429296f075ad7e4b5112c2e64a30277239a5a72 Mon Sep 17 00:00:00 2001 From: Chandler Prall Date: Tue, 2 Oct 2018 13:21:31 -0600 Subject: [PATCH 4/4] Don't transition context menu height on initial rendering --- src/components/context_menu/context_menu.js | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/src/components/context_menu/context_menu.js b/src/components/context_menu/context_menu.js index 01f60d11ead..3547a67ec2c 100644 --- a/src/components/context_menu/context_menu.js +++ b/src/components/context_menu/context_menu.js @@ -103,7 +103,7 @@ export class EuiContextMenu extends Component { idToPanelMap: {}, idToPreviousPanelIdMap: {}, idAndItemIndexToPanelIdMap: {}, - idToRenderedItemsMap: {}, + idToRenderedItemsMap: this.mapIdsToRenderedItems(this.props.panels), height: undefined, outgoingPanelId: undefined, @@ -115,13 +115,11 @@ export class EuiContextMenu extends Component { }; } - componentDidMount() { - this.mapIdsToRenderedItems(this.props.panels); - } - componentDidUpdate(prevProps) { if (prevProps.panels !== this.props.panels) { - this.mapIdsToRenderedItems(this.props.panels); + this.setState({ // eslint-disable-line react/no-did-update-set-state + idToRenderedItemsMap: this.mapIdsToRenderedItems(this.props.panels), + }); } } @@ -205,7 +203,7 @@ export class EuiContextMenu extends Component { idToRenderedItemsMap[panel.id] = this.renderItems(panel.items); }); - this.setState({ idToRenderedItemsMap }); + return idToRenderedItemsMap; }; renderItems(items = []) {