From 679bdbefa998fa06a045b63f853e126cf940335d Mon Sep 17 00:00:00 2001 From: Cee Chen Date: Sat, 30 Sep 2023 11:34:21 -0700 Subject: [PATCH 01/12] Update EuiComboBox to dogfood EuiInputPopover + rip out custom EuiPortal and EuiPopoverPanel usage - there's still a lot more logic to rip out which EuiInputPopover already handles, this is just the minimum needed to maintain working behavior --- src/components/combo_box/combo_box.tsx | 174 +++++++++--------- .../_combo_box_options_list.scss | 20 +- .../combo_box_options_list.tsx | 137 ++++++-------- 3 files changed, 154 insertions(+), 177 deletions(-) diff --git a/src/components/combo_box/combo_box.tsx b/src/components/combo_box/combo_box.tsx index 4d6e431b4dc..9c8d3e73fa1 100644 --- a/src/components/combo_box/combo_box.tsx +++ b/src/components/combo_box/combo_box.tsx @@ -22,7 +22,7 @@ import classNames from 'classnames'; import { findPopoverPosition, htmlIdGenerator, keys } from '../../services'; import { getElementZIndex } from '../../services/popover'; import { CommonProps } from '../common'; -import { EuiPortal } from '../portal'; +import { EuiInputPopover } from '../popover'; import { EuiI18n } from '../i18n'; import { EuiFormControlLayoutProps } from '../form'; import { EuiFilterSelectItemClass } from '../filter_group/filter_select_item'; @@ -966,50 +966,46 @@ export class EuiComboBox extends Component< : undefined; optionsList = ( - - - {(listboxAriaLabel: string) => ( - - )} - - + + {(listboxAriaLabel: string) => ( + + )} + ); } @@ -1029,46 +1025,58 @@ export class EuiComboBox extends Component< onBlur={this.onContainerBlur} ref={this.comboBoxRefCallback} > - 0} - id={inputId} - inputRef={this.searchInputRefCallback} - isDisabled={isDisabled} - isListOpen={isListOpen} - noIcon={!!noSuggestions} - onChange={this.onSearchChange} - onClear={ - isClearable && !isDisabled ? this.clearSelectedOptions : undefined + panelPaddingSize="none" + disableFocusTrap={true} + input={ + 0} + id={inputId} + inputRef={this.searchInputRefCallback} + isDisabled={isDisabled} + isListOpen={isListOpen} + noIcon={!!noSuggestions} + onChange={this.onSearchChange} + onClear={ + isClearable && !isDisabled + ? this.clearSelectedOptions + : undefined + } + onClick={this.onComboBoxClick} + onCloseListClick={this.onCloseListClick} + onFocus={this.onComboBoxFocus} + onOpenListClick={this.onOpenListClick} + onRemoveOption={this.onRemoveOption} + placeholder={placeholder} + rootId={this.rootId} + searchValue={searchValue} + selectedOptions={selectedOptions} + singleSelection={singleSelection} + toggleButtonRef={this.toggleButtonRefCallback} + updatePosition={this.updatePosition} + value={value} + append={singleSelection ? append : undefined} + prepend={singleSelection ? prepend : undefined} + isLoading={isLoading} + isInvalid={markAsInvalid} + autoFocus={autoFocus} + aria-label={ariaLabel} + aria-labelledby={ariaLabelledby} + /> } - onClick={this.onComboBoxClick} - onCloseListClick={this.onCloseListClick} - onFocus={this.onComboBoxFocus} - onOpenListClick={this.onOpenListClick} - onRemoveOption={this.onRemoveOption} - placeholder={placeholder} - rootId={this.rootId} - searchValue={searchValue} - selectedOptions={selectedOptions} - singleSelection={singleSelection} - toggleButtonRef={this.toggleButtonRefCallback} - updatePosition={this.updatePosition} - value={value} - append={singleSelection ? append : undefined} - prepend={singleSelection ? prepend : undefined} - isLoading={isLoading} - isInvalid={markAsInvalid} - autoFocus={autoFocus} - aria-label={ariaLabel} - aria-labelledby={ariaLabelledby} - /> - {optionsList} + > + {optionsList} + ); } diff --git a/src/components/combo_box/combo_box_options_list/_combo_box_options_list.scss b/src/components/combo_box/combo_box_options_list/_combo_box_options_list.scss index 46da053fed4..78a3f4a5f7f 100644 --- a/src/components/combo_box/combo_box_options_list/_combo_box_options_list.scss +++ b/src/components/combo_box/combo_box_options_list/_combo_box_options_list.scss @@ -2,13 +2,13 @@ * 1. Using specificity to override panel shadow * 2. Prevent really long input from overflowing the container. */ + .euiComboBoxOptionsList { - // Remove transforms from popover panel - transform: none !important; // stylelint-disable-line declaration-no-important - top: 0; + max-height: 200px; // Also used/set in the JS file + overflow: hidden; - .euiFilterSelectItem__content { - margin-block: 0 !important; // stylelint-disable-line declaration-no-important + > div { // Targets the element for FixedSizeList that doesn't have a selector + @include euiScrollBar; } /* Kibana FTR affordance - without this, Selenium complains about the overlaid @@ -26,13 +26,3 @@ text-align: center; word-wrap: break-word; } - -.euiComboBoxOptionsList__rowWrap { - padding: 0; - max-height: 200px; // Also used/set in the JS file - overflow: hidden; - - > div { // Targets the element for FixedSizeList that doesn't have a selector - @include euiScrollBar; - } -} diff --git a/src/components/combo_box/combo_box_options_list/combo_box_options_list.tsx b/src/components/combo_box/combo_box_options_list/combo_box_options_list.tsx index 217e19e4d18..bd4e5263317 100644 --- a/src/components/combo_box/combo_box_options_list/combo_box_options_list.tsx +++ b/src/components/combo_box/combo_box_options_list/combo_box_options_list.tsx @@ -6,12 +6,7 @@ * Side Public License, v 1. */ -import React, { - Component, - ComponentProps, - ReactNode, - RefCallback, -} from 'react'; +import React, { Component, ReactNode, RefCallback } from 'react'; import classNames from 'classnames'; import { FixedSizeList, @@ -34,7 +29,6 @@ import { import { htmlIdGenerator } from '../../../services'; import { CommonProps } from '../../common'; import { EuiBadge } from '../../badge'; -import { EuiPopoverPanel } from '../../popover/popover_panel'; import { EuiTextTruncate } from '../../text_truncate'; import type { _EuiComboBoxProps } from '../combo_box'; @@ -47,60 +41,59 @@ import { UpdatePositionHandler, } from '../types'; -export type EuiComboBoxOptionsListProps = CommonProps & - ComponentProps & { - activeOptionIndex?: number; - areAllOptionsSelected?: boolean; - listboxAriaLabel: string; - /** - * Creates a custom text option. You can use `{searchValue}` inside your string to better customize your text. - * It won't show if there's no onCreateOption. - */ - customOptionText?: string; - fullWidth?: boolean; - getSelectedOptionForSearchValue?: (params: { - isCaseSensitive?: boolean; - searchValue: string; - selectedOptions: any[]; - }) => EuiComboBoxOptionOption | undefined; +export type EuiComboBoxOptionsListProps = CommonProps & { + activeOptionIndex?: number; + areAllOptionsSelected?: boolean; + listboxAriaLabel: string; + /** + * Creates a custom text option. You can use `{searchValue}` inside your string to better customize your text. + * It won't show if there's no onCreateOption. + */ + customOptionText?: string; + fullWidth?: boolean; + getSelectedOptionForSearchValue?: (params: { isCaseSensitive?: boolean; - isLoading?: boolean; - listRef: RefCallback; - matchingOptions: Array>; - onCloseList: (event: Event) => void; - onCreateOption?: ( - searchValue: string, - options: Array> - ) => boolean | void; - onOptionClick?: OptionHandler; - onOptionEnterKey?: OptionHandler; - onScroll?: ListProps['onScroll']; - optionRef: ( - index: number, - node: RefInstance - ) => void; - /** - * Array of EuiComboBoxOptionOption objects. See #EuiComboBoxOptionOption - */ - options: Array>; - position?: EuiComboBoxOptionsListPosition; - renderOption?: ( - option: EuiComboBoxOptionOption, - searchValue: string, - OPTION_CONTENT_CLASSNAME: string - ) => ReactNode; - rootId: ReturnType; - rowHeight: number; - scrollToIndex?: number; searchValue: string; - selectedOptions: Array>; - updatePosition: UpdatePositionHandler; - width: number; - singleSelection?: boolean | EuiComboBoxSingleSelectionShape; - delimiter?: string; - zIndex?: number; - truncationProps?: _EuiComboBoxProps['truncationProps']; - }; + selectedOptions: any[]; + }) => EuiComboBoxOptionOption | undefined; + isCaseSensitive?: boolean; + isLoading?: boolean; + listRef: RefCallback; + matchingOptions: Array>; + onCloseList: (event: Event) => void; + onCreateOption?: ( + searchValue: string, + options: Array> + ) => boolean | void; + onOptionClick?: OptionHandler; + onOptionEnterKey?: OptionHandler; + onScroll?: ListProps['onScroll']; + optionRef: ( + index: number, + node: RefInstance + ) => void; + /** + * Array of EuiComboBoxOptionOption objects. See #EuiComboBoxOptionOption + */ + options: Array>; + position?: EuiComboBoxOptionsListPosition; + renderOption?: ( + option: EuiComboBoxOptionOption, + searchValue: string, + OPTION_CONTENT_CLASSNAME: string + ) => ReactNode; + rootId: ReturnType; + rowHeight: number; + scrollToIndex?: number; + searchValue: string; + selectedOptions: Array>; + updatePosition: UpdatePositionHandler; + width: number; + singleSelection?: boolean | EuiComboBoxSingleSelectionShape; + delimiter?: string; + zIndex?: number; + truncationProps?: _EuiComboBoxProps['truncationProps']; +}; const hitEnterBadge = ( extends Component< matchingOptions.length < 7 ? matchingOptions.length : 7; const height = numVisibleOptions * (rowHeight + 1); // Add one for the border - // bounded by max-height of euiComboBoxOptionsList__rowWrap + // bounded by max-height of .euiComboBoxOptionsList const boundedHeight = height > 200 ? 200 : height; const optionsList = ( @@ -552,29 +545,15 @@ export class EuiComboBoxOptionsList extends Component< ); - /** - * Reusing the EuiPopover__panel classes to help with consistency/maintenance. - * But this should really be converted to user the popover component. - */ - const classes = classNames('euiComboBoxOptionsList'); - return ( - -
- {emptyState || optionsList} -
- + {emptyState || optionsList} + ); } } From e396a68a9c257b28ea085aef5a650ba4833aed85 Mon Sep 17 00:00:00 2001 From: Cee Chen Date: Sat, 30 Sep 2023 12:25:39 -0700 Subject: [PATCH 02/12] Remove now unnecessary update position logic - underlying popover component takes care of that + remove `euiBody-hasPortalContent` logic (if underlying EuiPopover/EuiPortal components don't need it, combobox shouldn't either) - note: type exported from index was not being used by any consumer-facing component, so I don't think it's a breaking change to remove it - TODO: pass resize-observer `width` from EuiInputPopover to combobox options component --- src/components/combo_box/combo_box.tsx | 63 +------------------ .../combo_box_input/combo_box_input.tsx | 23 ++----- .../combo_box_options_list.tsx | 36 +---------- src/components/combo_box/index.ts | 1 - src/components/combo_box/types.ts | 5 -- 5 files changed, 7 insertions(+), 121 deletions(-) diff --git a/src/components/combo_box/combo_box.tsx b/src/components/combo_box/combo_box.tsx index 9c8d3e73fa1..dd9d8be83f1 100644 --- a/src/components/combo_box/combo_box.tsx +++ b/src/components/combo_box/combo_box.tsx @@ -19,7 +19,7 @@ import React, { } from 'react'; import classNames from 'classnames'; -import { findPopoverPosition, htmlIdGenerator, keys } from '../../services'; +import { htmlIdGenerator, keys } from '../../services'; import { getElementZIndex } from '../../services/popover'; import { CommonProps } from '../common'; import { EuiInputPopover } from '../popover'; @@ -41,11 +41,9 @@ import { } from './combo_box_input/combo_box_input'; import { EuiComboBoxOptionsListProps } from './combo_box_options_list/combo_box_options_list'; import { - UpdatePositionHandler, OptionHandler, RefInstance, EuiComboBoxOptionOption, - EuiComboBoxOptionsListPosition, EuiComboBoxSingleSelectionShape, } from './types'; import { EuiComboBoxOptionsList } from './combo_box_options_list'; @@ -194,7 +192,6 @@ interface EuiComboBoxState { hasFocus: boolean; isListOpen: boolean; listElement?: RefInstance; - listPosition: EuiComboBoxOptionsListPosition; listZIndex: number | undefined; matchingOptions: Array>; searchValue: string; @@ -225,7 +222,6 @@ export class EuiComboBox extends Component< hasFocus: false, isListOpen: false, listElement: null, - listPosition: 'bottom', listZIndex: undefined, matchingOptions: getMatchingOptions({ options: this.props.options, @@ -311,59 +307,6 @@ export class EuiComboBox extends Component< }); }; - updatePosition: UpdatePositionHandler = ( - listElement = this.state.listElement - ) => { - if (!this._isMounted) { - return; - } - - if (!this.state.isListOpen) { - return; - } - - if (!listElement) { - return; - } - - // it's possible that updateListPosition is called when listElement is becoming visible, but isn't yet - const listElementBounds = listElement.getBoundingClientRect(); - if (listElementBounds.width === 0 || listElementBounds.height === 0) { - return; - } - - if (!this.comboBoxRefInstance) { - return; - } - - const comboBoxBounds = this.comboBoxRefInstance.getBoundingClientRect(); - - const { position, top } = findPopoverPosition({ - allowCrossAxis: false, - anchor: this.comboBoxRefInstance, - popover: listElement, - position: 'bottom', - }) as { position: 'bottom'; top: number }; - - if (this.listRefInstance) { - this.listRefInstance.style.top = `${top}px`; - // listElement doesn't have its width set until after updating the position - // which means the popover service won't know about the correct width - // however, we already know where to position the element - this.listRefInstance.style.left = `${ - comboBoxBounds.left + window.pageXOffset - }px`; - this.listRefInstance.style.width = `${comboBoxBounds.width}px`; - } - - // Cache for future calls. - this.setState({ - listElement, - listPosition: position, - width: comboBoxBounds.width, - }); - }; - incrementActiveOptionIndex = (amount: number) => { // If there are no options available, do nothing. if (!this.state.matchingOptions.length) { @@ -927,7 +870,6 @@ export class EuiComboBox extends Component< activeOptionIndex, hasFocus, isListOpen, - listPosition, searchValue, width, matchingOptions, @@ -989,7 +931,6 @@ export class EuiComboBox extends Component< onScroll={this.onOptionListScroll} optionRef={this.optionRefCallback} options={options} - position={listPosition} singleSelection={singleSelection} renderOption={renderOption} rootId={this.rootId} @@ -997,7 +938,6 @@ export class EuiComboBox extends Component< scrollToIndex={activeOptionIndex} searchValue={searchValue} selectedOptions={selectedOptions} - updatePosition={this.updatePosition} width={width} delimiter={delimiter} getSelectedOptionForSearchValue={getSelectedOptionForSearchValue} @@ -1063,7 +1003,6 @@ export class EuiComboBox extends Component< selectedOptions={selectedOptions} singleSelection={singleSelection} toggleButtonRef={this.toggleButtonRefCallback} - updatePosition={this.updatePosition} value={value} append={singleSelection ? append : undefined} prepend={singleSelection ? prepend : undefined} diff --git a/src/components/combo_box/combo_box_input/combo_box_input.tsx b/src/components/combo_box/combo_box_input/combo_box_input.tsx index ead4a60c328..6b6f6f928c0 100644 --- a/src/components/combo_box/combo_box_input/combo_box_input.tsx +++ b/src/components/combo_box/combo_box_input/combo_box_input.tsx @@ -29,7 +29,6 @@ import { EuiComboBoxOptionOption, EuiComboBoxSingleSelectionShape, OptionHandler, - UpdatePositionHandler, } from '../types'; export interface EuiComboBoxInputProps extends CommonProps { @@ -56,7 +55,6 @@ export interface EuiComboBoxInputProps extends CommonProps { selectedOptions: Array>; singleSelection?: boolean | EuiComboBoxSingleSelectionShape; toggleButtonRef?: RefCallback; - updatePosition: UpdatePositionHandler; value?: string; prepend?: EuiFormControlLayoutProps['prepend']; append?: EuiFormControlLayoutProps['append']; @@ -99,12 +97,11 @@ export class EuiComboBoxInput extends Component< this.setState({ inputWidth }); }; - updatePosition = () => { - // Wait a beat for the DOM to update, since we depend on DOM elements' bounds. - requestAnimationFrame(() => { - this.props.updatePosition(); - }); - }; + componentDidUpdate(prevProps: EuiComboBoxInputProps) { + if (prevProps.searchValue !== this.props.searchValue) { + this.updateInputSize(this.props.searchValue); + } + } onFocus: FocusEventHandler = (event) => { this.props.onFocus(event); @@ -145,16 +142,6 @@ export class EuiComboBoxInput extends Component< } }; - componentDidUpdate(prevProps: EuiComboBoxInputProps) { - if (prevProps.searchValue !== this.props.searchValue) { - this.updateInputSize(this.props.searchValue); - - // We need to update the position of everything if the user enters enough input to change - // the size of the input. - this.updatePosition(); - } - } - render() { const { compressed, diff --git a/src/components/combo_box/combo_box_options_list/combo_box_options_list.tsx b/src/components/combo_box/combo_box_options_list/combo_box_options_list.tsx index bd4e5263317..12998825e15 100644 --- a/src/components/combo_box/combo_box_options_list/combo_box_options_list.tsx +++ b/src/components/combo_box/combo_box_options_list/combo_box_options_list.tsx @@ -34,11 +34,9 @@ import { EuiTextTruncate } from '../../text_truncate'; import type { _EuiComboBoxProps } from '../combo_box'; import { EuiComboBoxOptionOption, - EuiComboBoxOptionsListPosition, EuiComboBoxSingleSelectionShape, OptionHandler, RefInstance, - UpdatePositionHandler, } from '../types'; export type EuiComboBoxOptionsListProps = CommonProps & { @@ -76,7 +74,6 @@ export type EuiComboBoxOptionsListProps = CommonProps & { * Array of EuiComboBoxOptionOption objects. See #EuiComboBoxOptionOption */ options: Array>; - position?: EuiComboBoxOptionsListPosition; renderOption?: ( option: EuiComboBoxOptionOption, searchValue: string, @@ -87,7 +84,6 @@ export type EuiComboBoxOptionsListProps = CommonProps & { scrollToIndex?: number; searchValue: string; selectedOptions: Array>; - updatePosition: UpdatePositionHandler; width: number; singleSelection?: boolean | EuiComboBoxSingleSelectionShape; delimiter?: string; @@ -117,22 +113,7 @@ export class EuiComboBoxOptionsList extends Component< isCaseSensitive: false, }; - updatePosition = () => { - // Wait a beat for the DOM to update, since we depend on DOM elements' bounds. - requestAnimationFrame(() => { - this.props.updatePosition(this.listRefInstance); - }); - }; - componentDidMount() { - // Wait a frame, otherwise moving focus from one combo box to another will result in the class - // being removed from the body. - requestAnimationFrame(() => { - document.body.classList.add('euiBody-hasPortalContent'); - }); - this.updatePosition(); - window.addEventListener('resize', this.updatePosition); - // Firefox will trigger a scroll event in many common situations when the options list div is appended // to the DOM; in testing it was always within 100ms, but setting a timeout here for 500ms to be safe setTimeout(() => { @@ -144,17 +125,6 @@ export class EuiComboBoxOptionsList extends Component< } componentDidUpdate(prevProps: EuiComboBoxOptionsListProps) { - const { options, selectedOptions, searchValue } = prevProps; - - // We don't compare matchingOptions because that will result in a loop. - if ( - searchValue !== this.props.searchValue || - options !== this.props.options || - selectedOptions !== this.props.selectedOptions - ) { - this.updatePosition(); - } - if ( this.listRef && typeof this.props.activeOptionIndex !== 'undefined' && @@ -165,8 +135,6 @@ export class EuiComboBoxOptionsList extends Component< } componentWillUnmount() { - document.body.classList.remove('euiBody-hasPortalContent'); - window.removeEventListener('resize', this.updatePosition); window.removeEventListener('scroll', this.closeListOnScroll, { capture: true, }); @@ -378,7 +346,6 @@ export class EuiComboBoxOptionsList extends Component< onScroll, optionRef, options, - position = 'bottom', renderOption, rootId, rowHeight, @@ -386,7 +353,6 @@ export class EuiComboBoxOptionsList extends Component< searchValue, selectedOptions, singleSelection, - updatePosition, width, delimiter, zIndex, @@ -539,7 +505,7 @@ export class EuiComboBoxOptionsList extends Component< itemData={matchingOptions} ref={this.setListRef} innerRef={this.setListBoxRef} - width={width} + width={width} // TODO: Inherit width from EuiInputPopover instead of EuiComboBox > {this.ListRow} diff --git a/src/components/combo_box/index.ts b/src/components/combo_box/index.ts index e80488c60ac..d85ab3bf70f 100644 --- a/src/components/combo_box/index.ts +++ b/src/components/combo_box/index.ts @@ -12,6 +12,5 @@ export * from './combo_box_input'; export * from './combo_box_options_list'; export type { EuiComboBoxOptionOption, - EuiComboBoxOptionsListPosition, EuiComboBoxSingleSelectionShape, } from './types'; diff --git a/src/components/combo_box/types.ts b/src/components/combo_box/types.ts index 0049587d0e7..45cf3838975 100644 --- a/src/components/combo_box/types.ts +++ b/src/components/combo_box/types.ts @@ -26,15 +26,10 @@ export interface EuiComboBoxOptionOption< truncationProps?: _EuiComboBoxProps['truncationProps']; } -export type UpdatePositionHandler = ( - listElement?: RefInstance -) => void; export type OptionHandler = (option: EuiComboBoxOptionOption) => void; export type RefInstance = T | null; -export type EuiComboBoxOptionsListPosition = 'top' | 'bottom'; - export interface EuiComboBoxSingleSelectionShape { asPlainText?: boolean; } From 2d85f3c8f4b71e45ea2152d87a512072c5a51053 Mon Sep 17 00:00:00 2001 From: Cee Chen Date: Sat, 30 Sep 2023 12:58:26 -0700 Subject: [PATCH 03/12] Remove now-unnecessary z-index logic - EuiPopover already handles this automatically under the hood + add regression test for this, since this had a issue filed in Kibana at some point (3551) --- src/components/combo_box/combo_box.spec.tsx | 30 +++++++++++++++++++ src/components/combo_box/combo_box.tsx | 17 +---------- .../combo_box_options_list.tsx | 3 -- 3 files changed, 31 insertions(+), 19 deletions(-) diff --git a/src/components/combo_box/combo_box.spec.tsx b/src/components/combo_box/combo_box.spec.tsx index 4fb066cbd0c..3e92a400ff8 100644 --- a/src/components/combo_box/combo_box.spec.tsx +++ b/src/components/combo_box/combo_box.spec.tsx @@ -11,6 +11,9 @@ /// import React, { useState } from 'react'; + +import { EuiFlyout, EuiPopover, EuiButton } from '../index'; + import { EuiComboBox } from './index'; // CI doesn't have access to the Inter font, so we need to manually include it @@ -276,4 +279,31 @@ describe('EuiComboBox', () => { cy.get('[data-test-subj=comboBoxOptionsList]').should('have.length', 1); }); }); + + describe('z-index regression testing', () => { + it('displays the dropdown list above any inherited z-indices from parents', () => { + cy.mount( + {}} size="s"> + {}} + button={Toggle popover} + > + + + + ); + cy.wait(500); // Let the flyout finish animating in + cy.get('[data-test-subj=comboBoxSearchInput]').click(); + + cy.get('[data-test-subj="comboBoxOptionsList"]') + .parents('[data-popover-panel]') + .should('have.css', 'z-index', '5000'); + + // Should be able to click the first option without an error + // about the popover or flyout blocking the click + cy.get('[role=option]').click('top'); + }); + }); }); diff --git a/src/components/combo_box/combo_box.tsx b/src/components/combo_box/combo_box.tsx index dd9d8be83f1..b5ace401114 100644 --- a/src/components/combo_box/combo_box.tsx +++ b/src/components/combo_box/combo_box.tsx @@ -20,7 +20,6 @@ import React, { import classNames from 'classnames'; import { htmlIdGenerator, keys } from '../../services'; -import { getElementZIndex } from '../../services/popover'; import { CommonProps } from '../common'; import { EuiInputPopover } from '../popover'; import { EuiI18n } from '../i18n'; @@ -192,7 +191,6 @@ interface EuiComboBoxState { hasFocus: boolean; isListOpen: boolean; listElement?: RefInstance; - listZIndex: number | undefined; matchingOptions: Array>; searchValue: string; width: number; @@ -222,7 +220,6 @@ export class EuiComboBox extends Component< hasFocus: false, isListOpen: false, listElement: null, - listZIndex: undefined, matchingOptions: getMatchingOptions({ options: this.props.options, selectedOptions: this.props.selectedOptions, @@ -260,14 +257,6 @@ export class EuiComboBox extends Component< listRefInstance: RefInstance = null; listRefCallback: RefCallback = (ref) => { - if (this.comboBoxRefInstance) { - // find the zIndex of the combobox relative to the page body - // and use that to depth-position the list box - // adds an extra `100` to provide some defense around neighboring elements' positioning - const listZIndex = - getElementZIndex(this.comboBoxRefInstance, document.body) + 100; - this.setState({ listZIndex }); - } this.listRefInstance = ref; }; @@ -301,10 +290,7 @@ export class EuiComboBox extends Component< } this.clearActiveOption(); - this.setState({ - listZIndex: undefined, - isListOpen: false, - }); + this.setState({ isListOpen: false }); }; incrementActiveOptionIndex = (amount: number) => { @@ -914,7 +900,6 @@ export class EuiComboBox extends Component< > {(listboxAriaLabel: string) => ( = CommonProps & { width: number; singleSelection?: boolean | EuiComboBoxSingleSelectionShape; delimiter?: string; - zIndex?: number; truncationProps?: _EuiComboBoxProps['truncationProps']; }; @@ -355,8 +354,6 @@ export class EuiComboBoxOptionsList extends Component< singleSelection, width, delimiter, - zIndex, - style, truncationProps, listboxAriaLabel, ...rest From 7e0cdc2de946e1c3f43be46400093672d0eeebb6 Mon Sep 17 00:00:00 2001 From: Cee Chen Date: Sat, 30 Sep 2023 22:59:19 -0700 Subject: [PATCH 04/12] Move `closeOnScroll` logic to EuiInputPopover - TODO: should `EuiSuperSelect` use this new prop as well? --- src-docs/src/views/popover/input_popover.tsx | 1 + src/components/combo_box/combo_box.tsx | 9 +--- .../combo_box_options_list.tsx | 29 ----------- src/components/popover/input_popover.spec.tsx | 52 +++++++++++++++++++ src/components/popover/input_popover.tsx | 43 +++++++++++++++ 5 files changed, 98 insertions(+), 36 deletions(-) diff --git a/src-docs/src/views/popover/input_popover.tsx b/src-docs/src/views/popover/input_popover.tsx index 1e87b9ee1a3..1b2fd51d14d 100644 --- a/src-docs/src/views/popover/input_popover.tsx +++ b/src-docs/src/views/popover/input_popover.tsx @@ -21,6 +21,7 @@ export default () => { setIsPopoverOpen(false)} + closeOnScroll={true} input={ setIsPopoverOpen(true)} diff --git a/src/components/combo_box/combo_box.tsx b/src/components/combo_box/combo_box.tsx index b5ace401114..2b79d3a0b68 100644 --- a/src/components/combo_box/combo_box.tsx +++ b/src/components/combo_box/combo_box.tsx @@ -282,13 +282,7 @@ export class EuiComboBox extends Component< }); }; - closeList = (event?: Event) => { - if (event && event.target === this.searchInputRefInstance) { - // really long search values / custom entries triggers a scroll event on the input - // which the EuiComboBoxOptionsList passes through here - return; - } - + closeList = () => { this.clearActiveOption(); this.setState({ isListOpen: false }); }; @@ -956,6 +950,7 @@ export class EuiComboBox extends Component< fullWidth={fullWidth} panelPaddingSize="none" disableFocusTrap={true} + closeOnScroll={true} input={ extends Component< isCaseSensitive: false, }; - componentDidMount() { - // Firefox will trigger a scroll event in many common situations when the options list div is appended - // to the DOM; in testing it was always within 100ms, but setting a timeout here for 500ms to be safe - setTimeout(() => { - window.addEventListener('scroll', this.closeListOnScroll, { - passive: true, // for better performance as we won't call preventDefault - capture: true, // scroll events don't bubble, they must be captured instead - }); - }, 500); - } - componentDidUpdate(prevProps: EuiComboBoxOptionsListProps) { if ( this.listRef && @@ -133,24 +122,6 @@ export class EuiComboBoxOptionsList extends Component< } } - componentWillUnmount() { - window.removeEventListener('scroll', this.closeListOnScroll, { - capture: true, - }); - } - - closeListOnScroll = (event: Event) => { - // Close the list when a scroll event happens, but not if the scroll happened in the options list. - // This mirrors Firefox's approach of auto-closing `select` elements onscroll. - if ( - this.listRefInstance && - event.target && - this.listRefInstance.contains(event.target as Node) === false - ) { - this.props.onCloseList(event); - } - }; - listRefCallback: RefCallback = (ref) => { this.props.listRef(ref); this.listRefInstance = ref; diff --git a/src/components/popover/input_popover.spec.tsx b/src/components/popover/input_popover.spec.tsx index 80fde317f43..2e00958b3fa 100644 --- a/src/components/popover/input_popover.spec.tsx +++ b/src/components/popover/input_popover.spec.tsx @@ -200,4 +200,56 @@ describe('EuiPopover', () => { }); }); }); + + describe('closeOnScroll', () => { + const ScrollAllTheThings = () => { + const [isOpen, setIsOpen] = useState(true); + + return ( +
+ setIsOpen(false)} + input={ + setIsOpen(true)} + rows={1} + defaultValue={`hello\nworld`} + /> + } + > +
+
Popover content
+
+ +
+ ); + }; + + it('closes the popover when the user scrolls outside of the component', () => { + cy.mount(); + cy.wait(500); // Wait for the setTimeout in the useEffect + + // Scrolling the input or popover should not close the popover + cy.get('[data-test-subj="inputWithScroll"]').scrollTo('bottom'); + cy.get('[data-popover-panel]').should('exist'); + + cy.get('[data-test-subj="popoverWithScroll"]').scrollTo('bottom'); + cy.wait(500); // Wait a tick for false positives + cy.get('[data-popover-panel]').should('exist'); + + // Scrolling anywhere else should close the popover + cy.scrollTo('bottom'); + cy.get('[data-popover-panel]').should('not.exist'); + + // Popover should be able to re-opened after close + cy.get('[data-test-subj="inputWithScroll"]').click(); + cy.get('[data-popover-panel]').should('exist'); + }); + }); }); diff --git a/src/components/popover/input_popover.tsx b/src/components/popover/input_popover.tsx index 1d61c2a708e..cec436efffd 100644 --- a/src/components/popover/input_popover.tsx +++ b/src/components/popover/input_popover.tsx @@ -36,6 +36,10 @@ export interface _EuiInputPopoverProps */ anchorPosition?: 'downLeft' | 'downRight' | 'downCenter'; disableFocusTrap?: boolean; + /** + * Allows automatically closing the input popover on page scroll + */ + closeOnScroll?: boolean; fullWidth?: boolean; input: EuiPopoverProps['button']; inputRef?: EuiPopoverProps['buttonRef']; @@ -56,6 +60,7 @@ export const EuiInputPopover: FunctionComponent = ({ children, className, closePopover, + closeOnScroll = false, disableFocusTrap = false, focusTrapProps, input, @@ -140,6 +145,44 @@ export const EuiInputPopover: FunctionComponent = ({ [disableFocusTrap, closePopover, panelPropsOnKeyDown] ); + /** + * Optional close on scroll behavior + */ + + useEffect(() => { + // When the popover opens, add a scroll listener to the page (& remove it after) + if (closeOnScroll && panelEl) { + // Close the popover, but only if the scroll event occurs outside the input or the popover itself + const closePopoverOnScroll = (event: Event) => { + if (!panelEl || !inputEl || !event.target) return; + const scrollTarget = event.target as Node; + + if ( + panelEl.contains(scrollTarget) === false && + inputEl.contains(scrollTarget) === false + ) { + closePopover(); + } + }; + + // Firefox will trigger a scroll event in many common situations when the options list div is appended + // to the DOM; in testing it was always within 100ms, but setting a timeout here for 500ms to be safe + const timeoutId = setTimeout(() => { + window.addEventListener('scroll', closePopoverOnScroll, { + passive: true, // for better performance as we won't call preventDefault + capture: true, // scroll events don't bubble, they must be captured instead + }); + }, 500); + + return () => { + window.removeEventListener('scroll', closePopoverOnScroll, { + capture: true, + }); + clearTimeout(timeoutId); + }; + } + }, [closeOnScroll, closePopover, panelEl, inputEl]); + return ( Date: Sat, 30 Sep 2023 23:10:20 -0700 Subject: [PATCH 05/12] Remove now-unnecessary width state - EuiInputPopover is already managing this via resize observer --- src/components/combo_box/combo_box.tsx | 11 ----------- .../combo_box_options_list/combo_box_options_list.tsx | 10 ++++++---- src/components/popover/input_popover.tsx | 8 +++++++- 3 files changed, 13 insertions(+), 16 deletions(-) diff --git a/src/components/combo_box/combo_box.tsx b/src/components/combo_box/combo_box.tsx index 2b79d3a0b68..4bf04484d9e 100644 --- a/src/components/combo_box/combo_box.tsx +++ b/src/components/combo_box/combo_box.tsx @@ -193,7 +193,6 @@ interface EuiComboBoxState { listElement?: RefInstance; matchingOptions: Array>; searchValue: string; - width: number; } const initialSearchValue = ''; @@ -230,7 +229,6 @@ export class EuiComboBox extends Component< sortMatchesBy: this.props.sortMatchesBy, }), searchValue: initialSearchValue, - width: 0, }; _isMounted = false; @@ -240,13 +238,6 @@ export class EuiComboBox extends Component< comboBoxRefInstance: RefInstance = null; comboBoxRefCallback: RefCallback = (ref) => { this.comboBoxRefInstance = ref; - - if (this.comboBoxRefInstance) { - const comboBoxBounds = this.comboBoxRefInstance.getBoundingClientRect(); - this.setState({ - width: comboBoxBounds.width, - }); - } }; searchInputRefInstance: RefInstance = null; @@ -851,7 +842,6 @@ export class EuiComboBox extends Component< hasFocus, isListOpen, searchValue, - width, matchingOptions, } = this.state; @@ -917,7 +907,6 @@ export class EuiComboBox extends Component< scrollToIndex={activeOptionIndex} searchValue={searchValue} selectedOptions={selectedOptions} - width={width} delimiter={delimiter} getSelectedOptionForSearchValue={getSelectedOptionForSearchValue} listboxAriaLabel={listboxAriaLabel} diff --git a/src/components/combo_box/combo_box_options_list/combo_box_options_list.tsx b/src/components/combo_box/combo_box_options_list/combo_box_options_list.tsx index ec538865fe7..66a883d59be 100644 --- a/src/components/combo_box/combo_box_options_list/combo_box_options_list.tsx +++ b/src/components/combo_box/combo_box_options_list/combo_box_options_list.tsx @@ -6,7 +6,7 @@ * Side Public License, v 1. */ -import React, { Component, ReactNode, RefCallback } from 'react'; +import React, { Component, ContextType, ReactNode, RefCallback } from 'react'; import classNames from 'classnames'; import { FixedSizeList, @@ -30,6 +30,7 @@ import { htmlIdGenerator } from '../../../services'; import { CommonProps } from '../../common'; import { EuiBadge } from '../../badge'; import { EuiTextTruncate } from '../../text_truncate'; +import { EuiInputPopoverWidthContext } from '../../popover/input_popover'; import type { _EuiComboBoxProps } from '../combo_box'; import { @@ -84,7 +85,6 @@ export type EuiComboBoxOptionsListProps = CommonProps & { scrollToIndex?: number; searchValue: string; selectedOptions: Array>; - width: number; singleSelection?: boolean | EuiComboBoxSingleSelectionShape; delimiter?: string; truncationProps?: _EuiComboBoxProps['truncationProps']; @@ -106,6 +106,9 @@ export class EuiComboBoxOptionsList extends Component< listRef: FixedSizeList | null = null; listBoxRef: HTMLUListElement | null = null; + static contextType = EuiInputPopoverWidthContext; + declare context: ContextType; + static defaultProps = { 'data-test-subj': '', rowHeight: 29, // row height of default option renderer @@ -323,7 +326,6 @@ export class EuiComboBoxOptionsList extends Component< searchValue, selectedOptions, singleSelection, - width, delimiter, truncationProps, listboxAriaLabel, @@ -473,7 +475,7 @@ export class EuiComboBoxOptionsList extends Component< itemData={matchingOptions} ref={this.setListRef} innerRef={this.setListBoxRef} - width={width} // TODO: Inherit width from EuiInputPopover instead of EuiComboBox + width={this.context} > {this.ListRow} diff --git a/src/components/popover/input_popover.tsx b/src/components/popover/input_popover.tsx index cec436efffd..6fc3c41aa8f 100644 --- a/src/components/popover/input_popover.tsx +++ b/src/components/popover/input_popover.tsx @@ -15,6 +15,7 @@ import React, { useCallback, useMemo, useRef, + createContext, } from 'react'; import { css } from '@emotion/react'; import classnames from 'classnames'; @@ -56,6 +57,9 @@ export type EuiInputPopoverProps = CommonProps & HTMLAttributes & _EuiInputPopoverProps; +// Used by child components that want to know the parent popover width +export const EuiInputPopoverWidthContext = createContext(0); + export const EuiInputPopover: FunctionComponent = ({ children, className, @@ -202,7 +206,9 @@ export const EuiInputPopover: FunctionComponent = ({ disabled={disableFocusTrap} {...focusTrapProps} > - {children} + + {children} + ); From bd8df8bcd0cb6f0bd2bd43b4d7eeb5ae3887bb18 Mon Sep 17 00:00:00 2001 From: Cee Chen Date: Sat, 30 Sep 2023 23:38:03 -0700 Subject: [PATCH 06/12] Clean up further unnecessary list refs - by passing more elegant props to virtualized `FixedSizeList` component --- src/components/combo_box/combo_box.tsx | 2 - .../_combo_box_options_list.scss | 2 +- .../combo_box_options_list.tsx | 37 ++++++++++--------- 3 files changed, 20 insertions(+), 21 deletions(-) diff --git a/src/components/combo_box/combo_box.tsx b/src/components/combo_box/combo_box.tsx index 4bf04484d9e..f9481837b9b 100644 --- a/src/components/combo_box/combo_box.tsx +++ b/src/components/combo_box/combo_box.tsx @@ -190,7 +190,6 @@ interface EuiComboBoxState { activeOptionIndex: number; hasFocus: boolean; isListOpen: boolean; - listElement?: RefInstance; matchingOptions: Array>; searchValue: string; } @@ -218,7 +217,6 @@ export class EuiComboBox extends Component< activeOptionIndex: -1, hasFocus: false, isListOpen: false, - listElement: null, matchingOptions: getMatchingOptions({ options: this.props.options, selectedOptions: this.props.selectedOptions, diff --git a/src/components/combo_box/combo_box_options_list/_combo_box_options_list.scss b/src/components/combo_box/combo_box_options_list/_combo_box_options_list.scss index 78a3f4a5f7f..9904605dcca 100644 --- a/src/components/combo_box/combo_box_options_list/_combo_box_options_list.scss +++ b/src/components/combo_box/combo_box_options_list/_combo_box_options_list.scss @@ -7,7 +7,7 @@ max-height: 200px; // Also used/set in the JS file overflow: hidden; - > div { // Targets the element for FixedSizeList that doesn't have a selector + &__virtualization { @include euiScrollBar; } diff --git a/src/components/combo_box/combo_box_options_list/combo_box_options_list.tsx b/src/components/combo_box/combo_box_options_list/combo_box_options_list.tsx index 66a883d59be..2bd9408b179 100644 --- a/src/components/combo_box/combo_box_options_list/combo_box_options_list.tsx +++ b/src/components/combo_box/combo_box_options_list/combo_box_options_list.tsx @@ -10,6 +10,7 @@ import React, { Component, ContextType, ReactNode, RefCallback } from 'react'; import classNames from 'classnames'; import { FixedSizeList, + FixedSizeListProps, ListProps, ListChildComponentProps, } from 'react-window'; @@ -102,9 +103,7 @@ const hitEnterBadge = ( export class EuiComboBoxOptionsList extends Component< EuiComboBoxOptionsListProps > { - listRefInstance: RefInstance = null; listRef: FixedSizeList | null = null; - listBoxRef: HTMLUListElement | null = null; static contextType = EuiInputPopoverWidthContext; declare context: ContextType; @@ -125,24 +124,25 @@ export class EuiComboBoxOptionsList extends Component< } } - listRefCallback: RefCallback = (ref) => { - this.props.listRef(ref); - this.listRefInstance = ref; - }; - setListRef = (ref: FixedSizeList | null) => { this.listRef = ref; }; - setListBoxRef = (ref: HTMLUListElement | null) => { - this.listBoxRef = ref; - - if (ref) { - ref.setAttribute('aria-label', this.props.listboxAriaLabel); - ref.setAttribute('id', this.props.rootId('listbox')); - ref.setAttribute('role', 'listbox'); - ref.setAttribute('tabIndex', '0'); - } + ListInnerElement: FixedSizeListProps['innerElementType'] = ({ + children, + ...rest + }) => { + return ( +
+ {children} +
+ ); }; ListRow = ({ data, index, style }: ListChildComponentProps) => { @@ -468,13 +468,14 @@ export class EuiComboBoxOptionsList extends Component< const optionsList = ( {this.ListRow} @@ -485,7 +486,7 @@ export class EuiComboBoxOptionsList extends Component<
{emptyState || optionsList} From 8897ff42f6fa29261ecdf87007af49b799f10d67 Mon Sep 17 00:00:00 2001 From: Cee Chen Date: Sat, 30 Sep 2023 23:57:32 -0700 Subject: [PATCH 07/12] Remove even more unused/vestigial code --- src/components/combo_box/combo_box.tsx | 24 +------------------ .../combo_box_input/combo_box_input.tsx | 2 -- 2 files changed, 1 insertion(+), 25 deletions(-) diff --git a/src/components/combo_box/combo_box.tsx b/src/components/combo_box/combo_box.tsx index f9481837b9b..9ec721ed8ed 100644 --- a/src/components/combo_box/combo_box.tsx +++ b/src/components/combo_box/combo_box.tsx @@ -229,7 +229,6 @@ export class EuiComboBox extends Component< searchValue: initialSearchValue, }; - _isMounted = false; rootId = htmlIdGenerator(); // Refs @@ -249,14 +248,6 @@ export class EuiComboBox extends Component< this.listRefInstance = ref; }; - toggleButtonRefInstance: RefInstance = - null; - toggleButtonRefCallback: RefCallback = ( - ref - ) => { - this.toggleButtonRefInstance = ref; - }; - optionsRefInstances: Array> = []; optionRefCallback: EuiComboBoxOptionsListProps['optionRef'] = ( index, @@ -668,10 +659,6 @@ export class EuiComboBox extends Component< } }; - onCloseListClick = () => { - this.closeList(); - }; - onSearchChange: NonNullable['onChange']> = ( searchValue ) => { @@ -690,10 +677,6 @@ export class EuiComboBox extends Component< } }; - componentDidMount() { - this._isMounted = true; - } - static getDerivedStateFromProps( nextProps: _EuiComboBoxProps, prevState: EuiComboBoxState @@ -795,10 +778,6 @@ export class EuiComboBox extends Component< ); } - componentWillUnmount() { - this._isMounted = false; - } - render() { const { 'data-test-subj': dataTestSubj, @@ -960,7 +939,7 @@ export class EuiComboBox extends Component< : undefined } onClick={this.onComboBoxClick} - onCloseListClick={this.onCloseListClick} + onCloseListClick={this.closeList} onFocus={this.onComboBoxFocus} onOpenListClick={this.onOpenListClick} onRemoveOption={this.onRemoveOption} @@ -969,7 +948,6 @@ export class EuiComboBox extends Component< searchValue={searchValue} selectedOptions={selectedOptions} singleSelection={singleSelection} - toggleButtonRef={this.toggleButtonRefCallback} value={value} append={singleSelection ? append : undefined} prepend={singleSelection ? prepend : undefined} diff --git a/src/components/combo_box/combo_box_input/combo_box_input.tsx b/src/components/combo_box/combo_box_input/combo_box_input.tsx index 6b6f6f928c0..004112c7e8e 100644 --- a/src/components/combo_box/combo_box_input/combo_box_input.tsx +++ b/src/components/combo_box/combo_box_input/combo_box_input.tsx @@ -163,7 +163,6 @@ export class EuiComboBoxInput extends Component< searchValue, selectedOptions, singleSelection: singleSelectionProp, - toggleButtonRef, value, prepend, append, @@ -276,7 +275,6 @@ export class EuiComboBoxInput extends Component< 'data-test-subj': 'comboBoxToggleListButton', disabled: isDisabled, onClick: isListOpen && !isDisabled ? onCloseListClick : onOpenListClick, - ref: toggleButtonRef, side: 'right', tabIndex: -1, type: 'arrowDown', From 3ac62a39fea07c353d50b7a25712e4b7aec247c1 Mon Sep 17 00:00:00 2001 From: Cee Chen Date: Sun, 1 Oct 2023 09:48:45 -0700 Subject: [PATCH 08/12] [nit/cleanup] optional chaining syntax --- src/components/combo_box/combo_box.tsx | 51 +++++++------------------- 1 file changed, 13 insertions(+), 38 deletions(-) diff --git a/src/components/combo_box/combo_box.tsx b/src/components/combo_box/combo_box.tsx index 9ec721ed8ed..c981b2b08aa 100644 --- a/src/components/combo_box/combo_box.tsx +++ b/src/components/combo_box/combo_box.tsx @@ -240,7 +240,7 @@ export class EuiComboBox extends Component< searchInputRefInstance: RefInstance = null; searchInputRefCallback: RefCallback = (ref) => { this.searchInputRefInstance = ref; - if (this.props.inputRef) this.props.inputRef(ref); + this.props.inputRef?.(ref); }; listRefInstance: RefInstance = null; @@ -429,12 +429,8 @@ export class EuiComboBox extends Component< }; onComboBoxFocus: FocusEventHandler = (event) => { - if (this.props.onFocus) { - this.props.onFocus(event); - } - + this.props.onFocus?.(event); this.openList(); - this.setState({ hasFocus: true }); }; @@ -464,11 +460,8 @@ export class EuiComboBox extends Component< this.comboBoxRefInstance.contains(relatedTarget); if (!focusedInOptionsList && !focusedInInput) { + this.props.onBlur?.(event); this.closeList(); - - if (this.props.onBlur) { - this.props.onBlur(event); - } this.setState({ hasFocus: false }); // If the user tabs away or changes focus to another element, take whatever input they've @@ -540,9 +533,7 @@ export class EuiComboBox extends Component< break; default: - if (this.props.onKeyDown) { - this.props.onKeyDown(event); - } + this.props.onKeyDown?.(event); } }; @@ -572,17 +563,13 @@ export class EuiComboBox extends Component< ? [addedOption] : selectedOptions.concat(addedOption); - if (onChange) { - onChange(changeOptions); - } + onChange?.(changeOptions); this.clearSearchValue(); this.clearActiveOption(); if (!isContainerBlur) { - if (this.searchInputRefInstance) { - this.searchInputRefInstance.focus(); - } + this.searchInputRefInstance?.focus(); } if (singleSelection) { @@ -596,24 +583,17 @@ export class EuiComboBox extends Component< onRemoveOption: OptionHandler = (removedOption) => { const { onChange, selectedOptions } = this.props; - if (onChange) { - onChange(selectedOptions.filter((option) => option !== removedOption)); - } + onChange?.(selectedOptions.filter((option) => option !== removedOption)); this.clearActiveOption(); }; clearSelectedOptions = () => { - const { onChange } = this.props; - if (onChange) { - onChange([]); - } + this.props.onChange?.([]); // Clicking the clear button will also cause it to disappear. This would result in focus // shifting unexpectedly to the body element so we set it to the input which is more reasonable, - if (this.searchInputRefInstance) { - this.searchInputRefInstance.focus(); - } + this.searchInputRefInstance?.focus(); if (!this.state.isListOpen) { this.openList(); @@ -622,9 +602,7 @@ export class EuiComboBox extends Component< onComboBoxClick = () => { // When the user clicks anywhere on the box, enter the interaction state. - if (this.searchInputRefInstance) { - this.searchInputRefInstance.focus(); - } + this.searchInputRefInstance?.focus(); // If the user does this from a state in which an option has focus, then we need to reset it or clear it. if ( @@ -645,18 +623,15 @@ export class EuiComboBox extends Component< }; onOpenListClick = () => { - if (this.searchInputRefInstance) { - this.searchInputRefInstance.focus(); - } + this.searchInputRefInstance?.focus(); + if (!this.state.isListOpen) { this.openList(); } }; onOptionListScroll = () => { - if (this.searchInputRefInstance) { - this.searchInputRefInstance.focus(); - } + this.searchInputRefInstance?.focus(); }; onSearchChange: NonNullable['onChange']> = ( From f374ee3f1bc4cae339644f95299c4fd87a5811fc Mon Sep 17 00:00:00 2001 From: Cee Chen Date: Sun, 1 Oct 2023 09:51:01 -0700 Subject: [PATCH 09/12] [SPICY POTATO] Remove logic that apparently only exists for React 16.3 - no idea if this breaks production because there are apparently no tests for this, but it looks unnecessary - we should be ready to roll this individual commit back if necessary / if Kibana breaks --- src/components/combo_box/combo_box.tsx | 76 ------------------- .../combo_box_options_list.tsx | 9 --- 2 files changed, 85 deletions(-) diff --git a/src/components/combo_box/combo_box.tsx b/src/components/combo_box/combo_box.tsx index c981b2b08aa..0ffc1126050 100644 --- a/src/components/combo_box/combo_box.tsx +++ b/src/components/combo_box/combo_box.tsx @@ -24,7 +24,6 @@ import { CommonProps } from '../common'; import { EuiInputPopover } from '../popover'; import { EuiI18n } from '../i18n'; import { EuiFormControlLayoutProps } from '../form'; -import { EuiFilterSelectItemClass } from '../filter_group/filter_select_item'; import type { EuiTextTruncateProps } from '../text_truncate'; import { @@ -248,14 +247,6 @@ export class EuiComboBox extends Component< this.listRefInstance = ref; }; - optionsRefInstances: Array> = []; - optionRefCallback: EuiComboBoxOptionsListProps['optionRef'] = ( - index, - ref - ) => { - this.optionsRefInstances[index] = ref; - }; - openList = () => { this.setState({ isListOpen: true, @@ -687,72 +678,6 @@ export class EuiComboBox extends Component< return stateUpdate; } - updateMatchingOptionsIfDifferent = ( - newMatchingOptions: Array> - ) => { - const { matchingOptions, activeOptionIndex } = this.state; - const { singleSelection, selectedOptions } = this.props; - - let areOptionsDifferent = false; - - if (matchingOptions.length !== newMatchingOptions.length) { - areOptionsDifferent = true; - } else { - for (let i = 0; i < matchingOptions.length; i++) { - if (matchingOptions[i].label !== newMatchingOptions[i].label) { - areOptionsDifferent = true; - break; - } - } - } - - if (areOptionsDifferent) { - this.optionsRefInstances = []; - let nextActiveOptionIndex = activeOptionIndex; - // ensure that the currently selected single option is active if it is in the matchingOptions - if (Boolean(singleSelection) && selectedOptions.length === 1) { - if (newMatchingOptions.includes(selectedOptions[0])) { - nextActiveOptionIndex = newMatchingOptions.indexOf( - selectedOptions[0] - ); - } - } - - this.setState({ - matchingOptions: newMatchingOptions, - activeOptionIndex: nextActiveOptionIndex, - }); - - if (!newMatchingOptions.length) { - // Prevent endless setState -> componentWillUpdate -> setState loop. - if (this.hasActiveOption()) { - this.clearActiveOption(); - } - } - } - }; - - componentDidUpdate() { - const { options, selectedOptions, singleSelection, sortMatchesBy } = - this.props; - const { searchValue } = this.state; - - // React 16.3 has a bug (fixed in 16.4) where getDerivedStateFromProps - // isn't called after a state change, and we track `searchValue` in state - // instead we need to react to a change in searchValue here - this.updateMatchingOptionsIfDifferent( - getMatchingOptions({ - options, - selectedOptions, - searchValue, - isCaseSensitive: this.props.isCaseSensitive, - isPreFiltered: this.props.async, - showPrevSelected: Boolean(singleSelection), - sortMatchesBy, - }) - ); - } - render() { const { 'data-test-subj': dataTestSubj, @@ -850,7 +775,6 @@ export class EuiComboBox extends Component< onOptionClick={this.onOptionClick} onOptionEnterKey={this.onOptionEnterKey} onScroll={this.onOptionListScroll} - optionRef={this.optionRefCallback} options={options} singleSelection={singleSelection} renderOption={renderOption} diff --git a/src/components/combo_box/combo_box_options_list/combo_box_options_list.tsx b/src/components/combo_box/combo_box_options_list/combo_box_options_list.tsx index 2bd9408b179..f06e5092d00 100644 --- a/src/components/combo_box/combo_box_options_list/combo_box_options_list.tsx +++ b/src/components/combo_box/combo_box_options_list/combo_box_options_list.tsx @@ -24,7 +24,6 @@ import { EuiComboBoxTitle } from './combo_box_title'; import { EuiI18n } from '../../i18n'; import { EuiFilterSelectItem, - EuiFilterSelectItemClass, FilterChecked, } from '../../filter_group/filter_select_item'; import { htmlIdGenerator } from '../../../services'; @@ -38,7 +37,6 @@ import { EuiComboBoxOptionOption, EuiComboBoxSingleSelectionShape, OptionHandler, - RefInstance, } from '../types'; export type EuiComboBoxOptionsListProps = CommonProps & { @@ -68,10 +66,6 @@ export type EuiComboBoxOptionsListProps = CommonProps & { onOptionClick?: OptionHandler; onOptionEnterKey?: OptionHandler; onScroll?: ListProps['onScroll']; - optionRef: ( - index: number, - node: RefInstance - ) => void; /** * Array of EuiComboBoxOptionOption objects. See #EuiComboBoxOptionOption */ @@ -161,7 +155,6 @@ export class EuiComboBoxOptionsList extends Component< singleSelection, selectedOptions, onOptionClick, - optionRef, activeOptionIndex, renderOption, searchValue, @@ -205,7 +198,6 @@ export class EuiComboBoxOptionsList extends Component< onOptionClick(option); } }} - ref={optionRef.bind(this, index)} isFocused={optionIsFocused} checked={checked} showIcons={singleSelection ? true : false} @@ -317,7 +309,6 @@ export class EuiComboBoxOptionsList extends Component< onOptionClick, onOptionEnterKey, onScroll, - optionRef, options, renderOption, rootId, From 2e36462c925459c7e6208880c3ae2315d97063a3 Mon Sep 17 00:00:00 2001 From: Cee Chen Date: Sun, 1 Oct 2023 11:16:57 -0700 Subject: [PATCH 10/12] T E S T S - friendship with enzyme is over, RTL is my new best friend - remove snapshots that weren't asserting anything meaningful - turn off whitespace changes, etc etc - I probably should have broken this up a bit more into separate PRs but tbh these tests aren't great and will almost certainly need to be rewritten from scratch once we convert EuiComboBox to EuiSelectable in any case, so f it --- .../__snapshots__/combo_box.test.tsx.snap | 1408 +++++------------ src/components/combo_box/combo_box.spec.tsx | 175 +- src/components/combo_box/combo_box.test.tsx | 934 ++++++----- src/test/rtl/component_helpers.d.ts | 2 + src/test/rtl/component_helpers.ts | 14 +- 5 files changed, 1002 insertions(+), 1531 deletions(-) diff --git a/src/components/combo_box/__snapshots__/combo_box.test.tsx.snap b/src/components/combo_box/__snapshots__/combo_box.test.tsx.snap index 6bf8b35fb61..a1e7f5f4028 100644 --- a/src/components/combo_box/__snapshots__/combo_box.test.tsx.snap +++ b/src/components/combo_box/__snapshots__/combo_box.test.tsx.snap @@ -1,1094 +1,430 @@ // Jest Snapshot v1, https://goo.gl/fbAQLP -exports[`EuiComboBox is rendered 1`] = ` +exports[`EuiComboBox renders 1`] = `
- -
-
- +
+ +
+
+ +
+
`; -exports[`props aria-label attribute is rendered 1`] = ` -
- -
-`; - -exports[`props aria-labelledby attribute is rendered 1`] = ` -
- -
-`; - -exports[`props autoFocus is rendered 1`] = ` -
- -
-`; - -exports[`props custom ID is rendered 1`] = ` -
- -
-`; - -exports[`props delimiter is rendered 1`] = ` -
- -
-`; - -exports[`props full width is rendered 1`] = ` -
- -
-`; - -exports[`props isClearable=false disallows user from clearing input when no options are selected 1`] = ` -
- -
-`; - -exports[`props isClearable=false disallows user from clearing input when options are selected 1`] = ` -
- -
-`; - -exports[`props isDisabled is rendered 1`] = ` -
- -
-`; - -exports[`props option.prepend & option.append renders in pills 1`] = ` -Array [ - - - - - - Pre - - - - 1 - - - - - , - - - - - 2 - - - - Post - - - - - - , -] -`; - -exports[`props option.prepend & option.append renders in single selection 1`] = ` - - - - Pre - - - - 1 - - -`; - -exports[`props option.prepend & option.append renders in the options dropdown 1`] = ` -
-
+exports[`EuiComboBox renders the options list dropdown 1`] = ` + +
- -
+
- - 2 - - - Post - - - - - - + aria-hidden="true" + class="euiFormControlLayoutCustomIcon__icon" + data-euiicon-type="arrowDown" + /> + +
+
+
+
- -`; - -exports[`props options list is rendered 1`] = ` -
-
- -
-
- -
-
-
-
+ data-focus-guard="true" + style="width: 1px; height: 0px; padding: 0px; overflow: hidden; position: fixed; top: 1px; left: 1px;" + tabindex="-1" + />
+
+
+
+
- -`; - -exports[`props selectedOptions are rendered 1`] = ` -
- -
-`; - -exports[`props singleSelection is rendered 1`] = ` -
- -
-`; - -exports[`props singleSelection prepend and append is rendered 1`] = ` -
- - - - - - -
-`; - -exports[`props singleSelection selects existing option when opened 1`] = ` -
- - - - - - -
+ `; diff --git a/src/components/combo_box/combo_box.spec.tsx b/src/components/combo_box/combo_box.spec.tsx index 3e92a400ff8..39d05d57c46 100644 --- a/src/components/combo_box/combo_box.spec.tsx +++ b/src/components/combo_box/combo_box.spec.tsx @@ -10,11 +10,15 @@ /// /// -import React, { useState } from 'react'; +import React, { FunctionComponent, useState } from 'react'; import { EuiFlyout, EuiPopover, EuiButton } from '../index'; -import { EuiComboBox } from './index'; +import { + EuiComboBox, + type EuiComboBoxProps, + type EuiComboBoxOptionOption, +} from './index'; // CI doesn't have access to the Inter font, so we need to manually include it // for truncation font calculations to work correctly @@ -198,85 +202,144 @@ describe('EuiComboBox', () => { }); }); - describe('Backspace to delete last pill', () => { - const options = [ + describe('selection', () => { + const defaultOptions: Array> = [ { label: 'Item 1' }, { label: 'Item 2' }, { label: 'Item 3' }, ]; - - const TestComboBox = (...rest) => { - const [selectedOptions, setSelected] = useState([]); - const onChange = (selectedOptions) => { + const StatefulComboBox: FunctionComponent< + Partial> + > = ({ options = defaultOptions, ...rest }) => { + const [selectedOptions, setSelected] = useState([]); + const onChange = (selectedOptions: typeof options) => { setSelected(selectedOptions); }; return ( ); }; - it('does not delete the last pill if there is search text', () => { - cy.realMount(); - cy.get('[data-test-subj=comboBoxSearchInput]').realClick(); - cy.realPress('{downarrow}'); - cy.realPress('Enter'); - cy.realPress('{downarrow}'); - cy.realPress('Enter'); - cy.get('.euiComboBoxPill').should('have.length', 2); + describe('delimiter', () => { + it('selects the option when the delimiter option is typed into the search', () => { + cy.mount(); + cy.get('[data-test-subj="euiComboBoxPill"]').should('not.exist'); - cy.get('[data-test-subj=comboBoxSearchInput]').type('test'); - cy.get('[data-test-subj=comboBoxSearchInput]').realPress('Backspace'); + cy.get('[data-test-subj="comboBoxSearchInput"]').click(); + cy.realType('Item 1,'); - cy.get('[data-test-subj=comboBoxSearchInput]') - .invoke('val') - .should('equal', 'tes'); - cy.get('.euiComboBoxPill').should('have.length', 2); - }); + cy.get('[data-test-subj="euiComboBoxPill"]').should( + 'have.text', + 'Item 1' + ); + cy.get('[data-test-subj="comboBoxSearchInput"]').should( + 'have.value', + '' + ); + }); - it('does not delete the last pill if the input is not active when backspace is pressed', () => { - cy.realMount(); - cy.get('[data-test-subj=comboBoxSearchInput]').realClick(); - cy.realPress('{downarrow}'); - cy.realPress('Enter'); - cy.get('[data-test-subj=comboBoxSearchInput]').type('test'); - cy.realPress('Escape'); - cy.get('.euiComboBoxPill').should('have.length', 1); + it('does nothing if the item if already selected', () => { + cy.mount( + + ); + cy.get('[data-test-subj="euiComboBoxPill"]').should( + 'have.text', + 'Item 1' + ); - cy.realPress(['Shift', 'Tab']); // Should be focused on the first pill's X button - cy.realPress('Backspace'); - cy.get('.euiComboBoxPill').should('have.length', 1); + cy.get('[data-test-subj="comboBoxSearchInput"]').click(); + cy.realType('Item 1,'); - cy.repeatRealPress('Tab', 2); // Should be focused on the clear button - cy.realPress('Backspace'); - cy.get('.euiComboBoxPill').should('have.length', 1); - }); + cy.get('[data-test-subj="euiComboBoxPill"]').should('have.length', 1); + cy.get('[data-test-subj="comboBoxSearchInput"]').should( + 'have.value', + 'Item 1,' + ); + cy.contains("Item 1, doesn't match any options"); + }); - it('deletes the last pill added when backspace on the input is pressed ', () => { - cy.realMount(); - cy.get('[data-test-subj=comboBoxSearchInput]').realClick(); - cy.realPress('{downarrow}'); - cy.realPress('Enter'); - cy.realPress('{downarrow}'); - cy.realPress('Enter'); - cy.get('.euiComboBoxPill').should('have.length', 2); + it('still respects enter to select', () => { + cy.mount(); + cy.get('[data-test-subj="euiComboBoxPill"]').should('not.exist'); - cy.get('[data-test-subj=comboBoxSearchInput]').realPress('Backspace'); - cy.get('.euiComboBoxPill').should('have.length', 1); + cy.get('[data-test-subj="comboBoxSearchInput"]').click(); + cy.realType('Item 1'); + cy.realPress('Enter'); + + cy.get('[data-test-subj="euiComboBoxPill"]').should( + 'have.text', + 'Item 1' + ); + }); }); - it('opens up the selection list again after deleting the active single selection ', () => { - cy.realMount(); - cy.get('[data-test-subj=comboBoxSearchInput]').realClick(); - cy.realPress('{downarrow}'); - cy.realPress('Enter'); + describe('backspace to delete last pill', () => { + it('does not delete the last pill if there is search text', () => { + cy.realMount(); + cy.get('[data-test-subj=comboBoxSearchInput]').realClick(); + cy.realPress('{downarrow}'); + cy.realPress('Enter'); + cy.realPress('{downarrow}'); + cy.realPress('Enter'); + cy.get('.euiComboBoxPill').should('have.length', 2); + + cy.get('[data-test-subj=comboBoxSearchInput]').type('test'); + cy.get('[data-test-subj=comboBoxSearchInput]').realPress('Backspace'); + + cy.get('[data-test-subj=comboBoxSearchInput]') + .invoke('val') + .should('equal', 'tes'); + cy.get('.euiComboBoxPill').should('have.length', 2); + }); + + it('does not delete the last pill if the input is not active when backspace is pressed', () => { + cy.realMount(); + cy.get('[data-test-subj=comboBoxSearchInput]').realClick(); + cy.realPress('{downarrow}'); + cy.realPress('Enter'); + cy.get('[data-test-subj=comboBoxSearchInput]').type('test'); + cy.realPress('Escape'); + cy.get('.euiComboBoxPill').should('have.length', 1); + + cy.realPress(['Shift', 'Tab']); // Should be focused on the first pill's X button + cy.realPress('Backspace'); + cy.get('.euiComboBoxPill').should('have.length', 1); + + cy.repeatRealPress('Tab', 2); // Should be focused on the clear button + cy.realPress('Backspace'); + cy.get('.euiComboBoxPill').should('have.length', 1); + }); - cy.realPress('Backspace'); - cy.get('[data-test-subj=comboBoxOptionsList]').should('have.length', 1); + it('deletes the last pill added when backspace on the input is pressed ', () => { + cy.realMount(); + cy.get('[data-test-subj=comboBoxSearchInput]').realClick(); + cy.realPress('{downarrow}'); + cy.realPress('Enter'); + cy.realPress('{downarrow}'); + cy.realPress('Enter'); + cy.get('.euiComboBoxPill').should('have.length', 2); + + cy.get('[data-test-subj=comboBoxSearchInput]').realPress('Backspace'); + cy.get('.euiComboBoxPill').should('have.length', 1); + }); + + it('opens up the selection list again after deleting the active single selection ', () => { + cy.realMount(); + cy.get('[data-test-subj=comboBoxSearchInput]').realClick(); + cy.realPress('{downarrow}'); + cy.realPress('Enter'); + + cy.realPress('Backspace'); + cy.get('[data-test-subj=comboBoxOptionsList]').should('have.length', 1); + }); }); }); diff --git a/src/components/combo_box/combo_box.test.tsx b/src/components/combo_box/combo_box.test.tsx index e2e0a6c1bae..e6204b282a0 100644 --- a/src/components/combo_box/combo_box.test.tsx +++ b/src/components/combo_box/combo_box.test.tsx @@ -6,62 +6,37 @@ * Side Public License, v 1. */ -import React, { ReactNode } from 'react'; -import { act, fireEvent } from '@testing-library/react'; -import { shallow, mount } from 'enzyme'; -import { render } from '../../test/rtl'; +import React from 'react'; +import { fireEvent } from '@testing-library/react'; +import { render, showEuiComboBoxOptions } from '../../test/rtl'; import { shouldRenderCustomStyles } from '../../test/internal'; -import { - requiredProps, - findTestSubject, - takeMountedSnapshot, -} from '../../test'; -import { comboBoxKeys } from '../../services'; - -import { EuiComboBox, EuiComboBoxProps } from './combo_box'; -import type { EuiComboBoxOptionOption } from './types'; +import { requiredProps } from '../../test'; -jest.mock('../portal', () => ({ - EuiPortal: ({ children }: { children: ReactNode }) => children, -})); +import { keys } from '../../services'; +import { EuiComboBox } from './combo_box'; +import type { EuiComboBoxOptionOption } from './types'; -interface TitanOption { - 'data-test-subj'?: 'titanOption'; +interface Options { + 'data-test-subj'?: string; label: string; } -const options: TitanOption[] = [ +const options: Options[] = [ { 'data-test-subj': 'titanOption', label: 'Titan', }, - { - label: 'Enceladus', - }, - { - label: 'Mimas', - }, - { - label: 'Dione', - }, - { - label: 'Iapetus', - }, - { - label: 'Phoebe', - }, - { - label: 'Rhea', - }, + { label: 'Enceladus' }, + { label: 'Mimas' }, + { label: 'Dione' }, + { label: 'Iapetus' }, + { label: 'Phoebe' }, + { label: 'Rhea' }, { label: "Pandora is one of Saturn's moons, named for a Titaness of Greek mythology", }, - { - label: 'Tethys', - }, - { - label: 'Hyperion', - }, + { label: 'Tethys' }, + { label: 'Hyperion' }, ]; describe('EuiComboBox', () => { @@ -74,20 +49,17 @@ describe('EuiComboBox', () => { { skip: { parentTest: true }, childProps: ['truncationProps', 'options[0]'], - renderCallback: async ({ getByTestSubject, findAllByTestSubject }) => { - fireEvent.click(getByTestSubject('comboBoxToggleListButton')); - await findAllByTestSubject('truncatedText'); - }, + renderCallback: showEuiComboBoxOptions, } ); - test('is rendered', () => { + it('renders', () => { const { container } = render(); expect(container.firstChild).toMatchSnapshot(); }); - test('supports thousands of options in an options group', () => { + it('supports thousands of options in an options group', () => { // tests for a regression: RangeError: Maximum call stack size exceeded // https://mathiasbynens.be/demo/javascript-argument-count const options: EuiComboBoxOptionOption[] = [{ label: 'test', options: [] }]; @@ -95,508 +67,594 @@ describe('EuiComboBox', () => { options[0].options?.push({ label: `option ${i}` }); } - mount(); + render(); }); -}); -describe('props', () => { - test('options list is rendered', () => { - const component = mount( + it('renders the options list dropdown', async () => { + const { baseElement } = render( ); + await showEuiComboBoxOptions(); - act(() => { - component.setState({ isListOpen: true }); - }); - expect(takeMountedSnapshot(component)).toMatchSnapshot(); + expect(baseElement).toMatchSnapshot(); }); - test('selectedOptions are rendered', () => { - const component = shallow( + it('renders selectedOptions as pills', () => { + const { getAllByTestSubject } = render( ); + const selections = getAllByTestSubject('euiComboBoxPill'); - expect(component).toMatchSnapshot(); + expect(selections).toHaveLength(2); + expect(selections[0]).toHaveTextContent(options[2].label); + expect(selections[1]).toHaveTextContent(options[4].label); }); - test('custom ID is rendered', () => { - const component = shallow( - - ); - - expect(component).toMatchSnapshot(); - }); - - describe('option.prepend & option.append', () => { - const options = [ - { label: '1', prepend: Pre }, - { label: '2', append: Post }, - ]; + describe('props', () => { + describe('option.prepend & option.append', () => { + const options = [ + { label: '1', prepend: Pre }, + { label: '2', append: Post }, + ]; + + it('renders in pills', () => { + const { getByTestSubject, getAllByTestSubject } = render( + + ); + + expect(getByTestSubject('prepend')).toBeInTheDocument(); + expect(getByTestSubject('append')).toBeInTheDocument(); + + expect(getAllByTestSubject('euiComboBoxPill')[0]) + .toMatchInlineSnapshot(` + + + + + + Pre + + + + 1 + + + + + + `); + }); - test('renders in pills', () => { - const { getByTestSubject, getAllByTestSubject } = render( - - ); + test('renders in the options dropdown', async () => { + const { getByTestSubject, getAllByRole } = render( + + ); + await showEuiComboBoxOptions(); + + const dropdown = getByTestSubject('comboBoxOptionsList'); + expect( + dropdown.querySelector('.euiComboBoxOption__prepend') + ).toBeInTheDocument(); + expect( + dropdown.querySelector('.euiComboBoxOption__append') + ).toBeInTheDocument(); + + expect(getAllByRole('option')[0]).toMatchInlineSnapshot(` + + `); + }); - expect(getByTestSubject('prepend')).toBeInTheDocument(); - expect(getByTestSubject('append')).toBeInTheDocument(); - expect(getAllByTestSubject('euiComboBoxPill')).toMatchSnapshot(); + test('renders in single selection', () => { + const { getByTestSubject } = render( + + ); + + expect(getByTestSubject('euiComboBoxPill')).toMatchInlineSnapshot(` + + + + Pre + + + + 1 + + + `); + }); }); - test('renders in the options dropdown', () => { - const component = mount(); + describe('singleSelection', () => { + it('does not show or allow selecting more than one option', async () => { + const onChange = jest.fn(); + + const { getAllByTestSubject } = render( + + ); + const selections = getAllByTestSubject('euiComboBoxPill'); + + expect(selections).toHaveLength(1); + expect(selections[0]).toHaveTextContent('Mimas'); + }); - act(() => { - component.setState({ isListOpen: true }); + it('selects existing option when opened', async () => { + const { getByTestSubject } = render( + + ); + await showEuiComboBoxOptions(); + + const dropdown = getByTestSubject('comboBoxOptionsList'); + const checkedOption = '[data-euiicon-type="check"]'; + + // Should only have 1 checked item + expect(dropdown.querySelectorAll(checkedOption)).toHaveLength(1); + + // The first option should be rendered and have the check + const option = dropdown.querySelector('[data-test-subj="titanOption"]'); + expect(option).toBeTruthy(); + expect(option!.querySelector(checkedOption)).toBeTruthy(); }); - const dropdown = component.find( - 'div[data-test-subj="comboBoxOptionsList"]' - ); - expect(dropdown.find('.euiComboBoxOption__prepend')).toHaveLength(1); - expect(dropdown.find('.euiComboBoxOption__append')).toHaveLength(1); - expect(dropdown.render()).toMatchSnapshot(); + it('renders prepend and append in form layout', () => { + const { container } = render( + + ); + + expect( + container.querySelector('.euiFormControlLayout__prepend') + ).toBeInTheDocument(); + expect( + container.querySelector('.euiFormControlLayout__append') + ).toBeInTheDocument(); + }); + + it('renders as plain text', () => { + const { getByTestSubject } = render( + + ); + + expect(getByTestSubject('euiComboBoxPill').className).toContain( + 'euiComboBoxPill--plainText' + ); + }); }); - test('renders in single selection', () => { - const { getByTestSubject } = render( + test('isDisabled', () => { + const { container, queryByTestSubject, queryByTitle } = render( ); - expect(getByTestSubject('euiComboBoxPill')).toMatchSnapshot(); - }); - }); + expect(container.firstElementChild!.className).toContain('-isDisabled'); + expect(queryByTestSubject('comboBoxSearchInput')).toBeDisabled(); - describe('isClearable=false disallows user from clearing input', () => { - test('when no options are selected', () => { - const component = shallow( - - ); - - expect(component).toMatchSnapshot(); + expect(queryByTestSubject('comboBoxClearButton')).toBeFalsy(); + expect(queryByTestSubject('comboBoxToggleListButton')).toBeFalsy(); + expect( + queryByTitle('Remove Titan from selection in this group') + ).toBeFalsy(); }); - test('when options are selected', () => { - const component = shallow( + test('fullWidth', () => { + // TODO: Should likely be a visual screenshot test + const { container } = render( ); - expect(component).toMatchSnapshot(); + expect(container.innerHTML).toContain('euiFormControlLayout--fullWidth'); + expect(container.innerHTML).toContain('euiComboBox--fullWidth'); + expect(container.innerHTML).toContain( + 'euiComboBox__inputWrap--fullWidth' + ); }); - }); - describe('singleSelection', () => { - test('is rendered', () => { - const component = shallow( + test('autoFocus', () => { + const { getByTestSubject } = render( ); - expect(component).toMatchSnapshot(); - }); - test('selects existing option when opened', () => { - const component = shallow( - + expect(document.activeElement).toBe( + getByTestSubject('comboBoxSearchInput') ); - - component.setState({ isListOpen: true }); - expect(component).toMatchSnapshot(); }); - test('prepend and append is rendered', () => { - const component = shallow( + + test('aria-label / aria-labelledby renders on the input, not on the wrapper', () => { + const { getByTestSubject } = render( ); + const input = getByTestSubject('comboBoxSearchInput'); - component.setState({ isListOpen: true }); - expect(component).toMatchSnapshot(); + expect(input).toHaveAttribute('aria-label', 'Test label'); + expect(input).toHaveAttribute('aria-labelledby', 'test-heading-id'); }); - }); - - test('isDisabled is rendered', () => { - const component = shallow( - - ); - expect(component).toMatchSnapshot(); - }); - - test('full width is rendered', () => { - const component = shallow( - - ); + test('inputRef', () => { + const inputRefCallback = jest.fn(); - expect(component).toMatchSnapshot(); - }); - - test('delimiter is rendered', () => { - const component = shallow( - - ); + const { getByRole } = render( + + ); + expect(inputRefCallback).toHaveBeenCalledTimes(1); - expect(component).toMatchSnapshot(); + expect(getByRole('combobox')).toBe(inputRefCallback.mock.calls[0][0]); + }); }); - test('autoFocus is rendered', () => { - const component = shallow( + it('does not show multiple checkmarks with duplicate labels', async () => { + const options = [ + { label: 'Titan', key: 'titan1' }, + { label: 'Titan', key: 'titan2' }, + { label: 'Tethys' }, + ]; + const { baseElement } = render( ); + await showEuiComboBoxOptions(); - expect(component).toMatchSnapshot(); - }); - - test('aria-label attribute is rendered', () => { - const component = shallow( - + const dropdownOptions = baseElement.querySelectorAll( + '.euiFilterSelectItem' ); - - expect(component).toMatchSnapshot(); + expect( + dropdownOptions[0]!.querySelector('[data-euiicon-type="check"]') + ).toBeFalsy(); + expect( + dropdownOptions[1]!.querySelector('[data-euiicon-type="check"]') + ).toBeTruthy(); }); - test('aria-labelledby attribute is rendered', () => { - const component = shallow( - - ); - - expect(component).toMatchSnapshot(); - }); -}); + describe('behavior', () => { + describe('hitting "Enter"', () => { + it('calls the onCreateOption callback when there is input', () => { + const onCreateOptionHandler = jest.fn(); + + const { getByTestSubject } = render( + + ); + const input = getByTestSubject('comboBoxSearchInput'); + + fireEvent.change(input, { target: { value: 'foo' } }); + fireEvent.keyDown(input, { key: 'Enter' }); + + expect(onCreateOptionHandler).toHaveBeenCalledTimes(1); + expect(onCreateOptionHandler).toHaveBeenCalledWith('foo', options); + }); -test('does not show multiple checkmarks with duplicate labels', () => { - const options = [ - { - label: 'Titan', - key: 'titan1', - }, - { - label: 'Titan', - key: 'titan2', - }, - { - label: 'Tethys', - }, - ]; - const { baseElement, getByTestSubject } = render( - - ); - fireEvent.focus(getByTestSubject('comboBoxSearchInput')); - - const dropdownOptions = baseElement.querySelectorAll('.euiFilterSelectItem'); - expect( - dropdownOptions[0]!.querySelector('[data-euiicon-type="check"]') - ).toBeFalsy(); - expect( - dropdownOptions[1]!.querySelector('[data-euiicon-type="check"]') - ).toBeTruthy(); -}); + it('does not call onCreateOption when there is no input', () => { + const onCreateOptionHandler = jest.fn(); -describe('behavior', () => { - describe('hitting "Enter"', () => { - test('calls the onCreateOption callback when there is input', () => { - const onCreateOptionHandler = jest.fn(); + const { getByTestSubject } = render( + + ); + const input = getByTestSubject('comboBoxSearchInput'); - const component = mount( - - ); - - act(() => { - component.setState({ searchValue: 'foo' }); - }); + fireEvent.keyDown(input, { key: 'Enter' }); - act(() => { - const searchInput = findTestSubject(component, 'comboBoxSearchInput'); - searchInput.simulate('focus'); - searchInput.simulate('keyDown', { key: comboBoxKeys.ENTER }); + expect(onCreateOptionHandler).not.toHaveBeenCalled(); }); - - expect(onCreateOptionHandler).toHaveBeenCalledTimes(1); - expect(onCreateOptionHandler).toHaveBeenNthCalledWith(1, 'foo', options); }); - test("doesn't the onCreateOption callback when there is no input", () => { - const onCreateOptionHandler = jest.fn(); + describe('tabbing off the search input', () => { + it("closes the options list if the user isn't navigating the options", async () => { + const keyDownBubbled = jest.fn(); - const component = mount( - - ); + const { getByTestSubject } = render( +
+ +
+ ); + await showEuiComboBoxOptions(); - const searchInput = findTestSubject(component, 'comboBoxSearchInput'); - searchInput.simulate('focus'); - searchInput.simulate('keyDown', { key: comboBoxKeys.ENTER }); - expect(onCreateOptionHandler).not.toHaveBeenCalled(); - }); - }); + const mockEvent = { key: keys.TAB, shiftKey: true }; + fireEvent.keyDown(getByTestSubject('comboBoxSearchInput'), mockEvent); - describe('tabbing', () => { - test("off the search input closes the options list if the user isn't navigating the options", () => { - const onKeyDownWrapper = jest.fn(); - const component = mount( -
- -
- ); + // If the TAB keydown bubbled up to the wrapper, then a browser DOM would shift the focus + expect(keyDownBubbled).toHaveBeenCalledWith( + expect.objectContaining(mockEvent) + ); + }); - const searchInput = findTestSubject(component, 'comboBoxSearchInput'); - searchInput.simulate('focus'); + it('calls onCreateOption', () => { + const onCreateOptionHandler = jest.fn(); - // Focusing the input should open the options list. - expect(findTestSubject(component, 'comboBoxOptionsList')).toBeDefined(); + const { getByTestSubject } = render( + + ); + const input = getByTestSubject('comboBoxSearchInput'); - // Tab backwards to take focus off the combo box. - searchInput.simulate('keyDown', { - key: comboBoxKeys.TAB, - shiftKey: true, + fireEvent.change(input, { target: { value: 'foo' } }); + fireEvent.blur(input); + + expect(onCreateOptionHandler).toHaveBeenCalledTimes(1); + expect(onCreateOptionHandler).toHaveBeenCalledWith('foo', options); }); - // If the TAB keydown propagated to the wrapper, then a browser DOM would shift the focus - expect(onKeyDownWrapper).toHaveBeenCalledTimes(1); - }); + it('does nothing if the user is navigating the options', async () => { + const keyDownBubbled = jest.fn(); - test('off the search input calls onCreateOption', () => { - const onCreateOptionHandler = jest.fn(); + const { getByTestSubject } = render( +
+ +
+ ); + await showEuiComboBoxOptions(); - const component = mount( - - ); - - const searchInput = findTestSubject(component, 'comboBoxSearchInput'); + // Navigate to an option then tab off + const input = getByTestSubject('comboBoxSearchInput'); + fireEvent.keyDown(input, { key: keys.ARROW_DOWN }); + fireEvent.keyDown(input, { key: keys.TAB }); - act(() => { - component.setState({ searchValue: 'foo' }); - searchInput.simulate('focus'); + // If the TAB keydown did not bubble to the wrapper, then the tab event was prevented + expect(keyDownBubbled).not.toHaveBeenCalled(); }); - - searchInput.simulate('blur'); - - expect(onCreateOptionHandler).toHaveBeenCalledTimes(1); - expect(onCreateOptionHandler).toHaveBeenNthCalledWith(1, 'foo', options); }); - test('off the search input does nothing if the user is navigating the options', () => { - const onKeyDownWrapper = jest.fn(); - const component = mount( -
+ describe('clear button', () => { + it('renders when options are selected', () => { + const { getByTestSubject } = render( -
- ); - - const searchInput = findTestSubject(component, 'comboBoxSearchInput'); - searchInput.simulate('focus'); - - // Focusing the input should open the options list. - expect(findTestSubject(component, 'comboBoxOptionsList')).toBeDefined(); + ); - // Navigate to an option. - searchInput.simulate('keyDown', { key: comboBoxKeys.ARROW_DOWN }); - - // Tab backwards to take focus off the combo box. - searchInput.simulate('keyDown', { - key: comboBoxKeys.TAB, - shiftKey: true, + expect(getByTestSubject('comboBoxClearButton')).toBeInTheDocument(); }); - // If the TAB keydown did not bubble to the wrapper, then the tab event was prevented - expect(onKeyDownWrapper.mock.calls.length).toBe(0); - }); - }); - - describe('clear button', () => { - test('calls onChange callback with empty array', () => { - const onChangeHandler = jest.fn(); - const component = mount( - - ); - - findTestSubject(component, 'comboBoxClearButton').simulate('click'); - expect(onChangeHandler).toHaveBeenCalledTimes(1); - expect(onChangeHandler).toHaveBeenNthCalledWith(1, []); - }); + it('does not render when no options are selected', () => { + const { queryByTestSubject } = render( + + ); - test('focuses the input', () => { - const component = mount( - {}} - /> - ); + expect(queryByTestSubject('comboBoxClearButton')).toBeFalsy(); + }); - findTestSubject(component, 'comboBoxClearButton').simulate('click'); - expect( - findTestSubject(component, 'comboBoxSearchInput').getDOMNode() - ).toBe(document.activeElement); - }); - }); + it('does not render when isClearable is false', () => { + const { queryByTestSubject } = render( + + ); - describe('sortMatchesBy', () => { - const sortMatchesByOptions = [ - { - label: 'Something is Disabled', - }, - ...options, - ]; - test('options "none"', () => { - const component = mount< - EuiComboBox, - EuiComboBoxProps, - { matchingOptions: TitanOption[] } - >(); - - findTestSubject(component, 'comboBoxSearchInput').simulate('change', { - target: { value: 'di' }, + expect(queryByTestSubject('comboBoxClearButton')).toBeFalsy(); }); - expect(component.state('matchingOptions')[0].label).toBe( - 'Something is Disabled' - ); - }); + it('calls the onChange callback with empty array', () => { + const onChangeHandler = jest.fn(); - test('options "startsWith"', () => { - const component = mount< - EuiComboBox, - EuiComboBoxProps, - { matchingOptions: TitanOption[] } - >( - - ); + const { getByTestSubject } = render( + + ); + fireEvent.click(getByTestSubject('comboBoxClearButton')); - findTestSubject(component, 'comboBoxSearchInput').simulate('change', { - target: { value: 'di' }, + expect(onChangeHandler).toHaveBeenCalledTimes(1); + expect(onChangeHandler).toHaveBeenCalledWith([]); }); - expect(component.state('matchingOptions')[0].label).toBe('Dione'); + it('focuses the input', () => { + const { getByTestSubject } = render( + {}} + /> + ); + fireEvent.click(getByTestSubject('comboBoxClearButton')); + + expect(document.activeElement).toBe( + getByTestSubject('comboBoxSearchInput') + ); + }); }); - }); - describe('isCaseSensitive', () => { - const isCaseSensitiveOptions = [ - { - label: 'Case sensitivity', - }, - ]; - - test('options "false"', () => { - const component = mount< - EuiComboBox, - EuiComboBoxProps, - { matchingOptions: TitanOption[] } - >( - - ); - - findTestSubject(component, 'comboBoxSearchInput').simulate('change', { - target: { value: 'case' }, + describe('sortMatchesBy', () => { + const onSearchChange = jest.fn(); + const sortMatchesByOptions = [ + { label: 'Something is Disabled' }, + ...options, + ]; + + test('"none"', () => { + const { getByTestSubject, getAllByRole } = render( + + ); + fireEvent.change(getByTestSubject('comboBoxSearchInput'), { + target: { value: 'di' }, + }); + + const foundOptions = getAllByRole('option'); + expect(foundOptions).toHaveLength(2); + expect(foundOptions[0]).toHaveTextContent('Something is Disabled'); + expect(foundOptions[1]).toHaveTextContent('Dione'); }); - expect(component.state('matchingOptions')[0].label).toBe( - 'Case sensitivity' - ); + test('"startsWith"', () => { + const { getByTestSubject, getAllByRole } = render( + + ); + fireEvent.change(getByTestSubject('comboBoxSearchInput'), { + target: { value: 'di' }, + }); + + const foundOptions = getAllByRole('option'); + expect(foundOptions).toHaveLength(2); + expect(foundOptions[0]).toHaveTextContent('Dione'); + expect(foundOptions[1]).toHaveTextContent('Something is Disabled'); + }); }); - test('options "true"', () => { - const component = mount< - EuiComboBox, - EuiComboBoxProps, - { matchingOptions: TitanOption[] } - >( - - ); - - findTestSubject(component, 'comboBoxSearchInput').simulate('change', { - target: { value: 'case' }, + describe('isCaseSensitive', () => { + const isCaseSensitiveOptions = [{ label: 'Case sensitivity' }]; + + test('false', () => { + const { getByTestSubject, queryAllByRole } = render( + + ); + fireEvent.change(getByTestSubject('comboBoxSearchInput'), { + target: { value: 'case' }, + }); + + expect(queryAllByRole('option')).toHaveLength(1); }); - expect(component.state('matchingOptions').length).toBe(0); + test('true', () => { + const { getByTestSubject, queryAllByRole } = render( + + ); + const input = getByTestSubject('comboBoxSearchInput'); - findTestSubject(component, 'comboBoxSearchInput').simulate('change', { - target: { value: 'Case' }, - }); + fireEvent.change(input, { target: { value: 'case' } }); + expect(queryAllByRole('option')).toHaveLength(0); - expect(component.state('matchingOptions')[0].label).toBe( - 'Case sensitivity' - ); + fireEvent.change(input, { target: { value: 'Case' } }); + expect(queryAllByRole('option')).toHaveLength(1); + }); }); }); - - it('calls the inputRef prop with the input element', () => { - const inputRefCallback = jest.fn(); - - const component = mount< - EuiComboBox, - EuiComboBoxProps, - { matchingOptions: TitanOption[] } - >(); - - expect(inputRefCallback).toHaveBeenCalledTimes(1); - expect(component.find('input[role="combobox"]').getDOMNode()).toBe( - inputRefCallback.mock.calls[0][0] - ); - }); }); diff --git a/src/test/rtl/component_helpers.d.ts b/src/test/rtl/component_helpers.d.ts index 678cf0ba3dc..13dc28dcb6c 100644 --- a/src/test/rtl/component_helpers.d.ts +++ b/src/test/rtl/component_helpers.d.ts @@ -8,3 +8,5 @@ export declare const waitForEuiPopoverClose: () => Promise; export declare const waitForEuiToolTipVisible: () => Promise; export declare const waitForEuiToolTipHidden: () => Promise; + +export declare const showEuiComboBoxOptions: () => Promise; diff --git a/src/test/rtl/component_helpers.ts b/src/test/rtl/component_helpers.ts index aa37e1dde78..66611572cd6 100644 --- a/src/test/rtl/component_helpers.ts +++ b/src/test/rtl/component_helpers.ts @@ -7,7 +7,8 @@ */ import '@testing-library/jest-dom'; -import { waitFor } from '@testing-library/react'; +import { waitFor, fireEvent } from '@testing-library/react'; +import { screen } from './custom_render'; /** * Ensure the EuiPopover being tested is open/closed before continuing @@ -43,3 +44,14 @@ export const waitForEuiToolTipHidden = async () => const tooltip = document.querySelector('.euiToolTipPopover'); expect(tooltip).toBeNull(); }); + +/** + * Doot doo + */ +export const showEuiComboBoxOptions = async () => { + fireEvent.click(screen.getByTestSubject('comboBoxToggleListButton')); + await waitForEuiPopoverOpen(); + await waitFor(() => { + expect(screen.getByRole('listbox')).toBeInTheDocument(); + }); +}; From d50c5f86d18da0159f9286106f36c9f0c41e5fca Mon Sep 17 00:00:00 2001 From: Cee Chen Date: Sun, 1 Oct 2023 11:52:07 -0700 Subject: [PATCH 11/12] =?UTF-8?q?=F0=9F=8E=BA=20Add=20`inputPopoverProps`?= =?UTF-8?q?=20support=20=F0=9F=8E=BA?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- src/components/combo_box/combo_box.spec.tsx | 34 +++++++++++++++++++-- src/components/combo_box/combo_box.tsx | 15 +++++++-- 2 files changed, 44 insertions(+), 5 deletions(-) diff --git a/src/components/combo_box/combo_box.spec.tsx b/src/components/combo_box/combo_box.spec.tsx index 39d05d57c46..c9f45a66836 100644 --- a/src/components/combo_box/combo_box.spec.tsx +++ b/src/components/combo_box/combo_box.spec.tsx @@ -34,7 +34,7 @@ before(() => { }); describe('EuiComboBox', () => { - describe('Focus management', () => { + describe('focus management', () => { it('keeps focus on the input box when clicking a disabled item', () => { cy.realMount( { }); }); + describe('inputPopoverProps', () => { + it('allows setting a minimum popover width', () => { + cy.mount( + {}} + data-test-subj="combobox" + inputPopoverProps={{ + panelMinWidth: 300, + anchorPosition: 'downCenter', + }} + style={{ margin: '0 auto' }} + /> + ); + cy.get('[data-test-subj="comboBoxInput"]').click(); + + cy.get('[data-popover-panel]') + .should('have.css', 'inline-size', '400px') + .should('have.css', 'left', '50px'); + + cy.get('[data-test-subj="combobox"]').then( + ($el) => ($el[0].style.width = '200px') + ); + + cy.get('[data-popover-panel]') + .should('have.css', 'inline-size', '300px') + .should('have.css', 'left', '100px'); + }); + }); + describe('truncation', () => { const sharedProps = { style: { width: 200 }, @@ -201,7 +232,6 @@ describe('EuiComboBox', () => { }); }); }); - describe('selection', () => { const defaultOptions: Array> = [ { label: 'Item 1' }, diff --git a/src/components/combo_box/combo_box.tsx b/src/components/combo_box/combo_box.tsx index 0ffc1126050..ffc5678c75f 100644 --- a/src/components/combo_box/combo_box.tsx +++ b/src/components/combo_box/combo_box.tsx @@ -21,7 +21,7 @@ import classNames from 'classnames'; import { htmlIdGenerator, keys } from '../../services'; import { CommonProps } from '../common'; -import { EuiInputPopover } from '../popover'; +import { EuiInputPopover, EuiInputPopoverProps } from '../popover'; import { EuiI18n } from '../i18n'; import { EuiFormControlLayoutProps } from '../form'; import type { EuiTextTruncateProps } from '../text_truncate'; @@ -162,6 +162,13 @@ export interface _EuiComboBoxProps * text will always take precedence. */ truncationProps?: Partial>; + /** + * Allows customizing the underlying EuiInputPopover component + * (except for props that control state). + */ + inputPopoverProps?: Partial< + Omit + >; } /** @@ -710,6 +717,7 @@ export class EuiComboBox extends Component< append, autoFocus, truncationProps, + inputPopoverProps, 'aria-label': ariaLabel, 'aria-labelledby': ariaLabelledby, ...rest @@ -810,12 +818,13 @@ export class EuiComboBox extends Component< ref={this.comboBoxRefCallback} > Date: Mon, 2 Oct 2023 15:16:20 -0700 Subject: [PATCH 12/12] changelog --- upcoming_changelogs/7246.md | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 upcoming_changelogs/7246.md diff --git a/upcoming_changelogs/7246.md b/upcoming_changelogs/7246.md new file mode 100644 index 00000000000..98cd7e9792f --- /dev/null +++ b/upcoming_changelogs/7246.md @@ -0,0 +1,2 @@ +- Updated `EuiComboBox` to use `EuiInputPopover` under the hood +- Added `inputPopoverProps` to `EuiComboBox`, which allows customizing the underlying popover