From df8cae6689df357f812149b159a33bc7bd71c492 Mon Sep 17 00:00:00 2001 From: Trevor Mehard Date: Mon, 20 Jan 2020 09:59:12 -0500 Subject: [PATCH 1/9] Make setActiveItem public so the parent components can reset the activeItem when necessary. When the Suggest Popover is closed and there is a selectedItem the activeItem should always be set to selectedItem. When the Popover is opened again the value of the component is reflected correctly. --- .../src/components/query-list/queryList.tsx | 2 +- .../select/src/components/select/suggest.tsx | 21 +++++++++++++++++++ 2 files changed, 22 insertions(+), 1 deletion(-) diff --git a/packages/select/src/components/query-list/queryList.tsx b/packages/select/src/components/query-list/queryList.tsx index 0f19dc094b6..29b2039a18e 100644 --- a/packages/select/src/components/query-list/queryList.tsx +++ b/packages/select/src/components/query-list/queryList.tsx @@ -486,7 +486,7 @@ export class QueryList extends AbstractComponent2, IQueryL return getFirstEnabledItem(this.state.filteredItems, this.props.itemDisabled, direction, startIndex); } - private setActiveItem(activeItem: T | ICreateNewItem | null) { + public setActiveItem(activeItem: T | ICreateNewItem | null) { this.expectedNextActiveItem = activeItem; if (this.props.activeItem === undefined) { // indicate that the active item may need to be scrolled into view after update. diff --git a/packages/select/src/components/select/suggest.tsx b/packages/select/src/components/select/suggest.tsx index ef4eee906a7..3445d11a852 100644 --- a/packages/select/src/components/select/suggest.tsx +++ b/packages/select/src/components/select/suggest.tsx @@ -179,6 +179,7 @@ export class Suggest extends React.PureComponent, ISuggestSt popoverClassName={classNames(Classes.SELECT_POPOVER, popoverProps.popoverClassName)} onOpening={this.handlePopoverOpening} onOpened={this.handlePopoverOpened} + onClosed={this.handlePopoverClosed} > extends React.PureComponent, ISuggestSt Utils.safeInvokeMember(this.props.popoverProps, "onOpened", node); }; + private handlePopoverClosed = (node: HTMLElement) => { + // The activeItem should always be the selectedItem when the Popover is first opened + // if the activeItem prop is not set. Set the activeItem on close so that there isn't + // a flash of the activeItem on screen. + const shouldResetActiveItemToSelectedItem = ( + this.props.activeItem === undefined && + this.state.selectedItem && + !this.props.resetOnSelect + ); + + if (this.queryList && shouldResetActiveItemToSelectedItem) { + // If the selectedItem prop is set then use it. + // If not fall back to component state. + const selectedItem = this.props.selectedItem || this.state.selectedItem; + this.queryList.setActiveItem(selectedItem); + } + + Utils.safeInvokeMember(this.props.popoverProps, "onClosed", node); + }; + private getTargetKeyDownHandler = ( handleQueryListKeyDown: React.EventHandler>, ) => { From 4c2a0d939da6de35bbaa816ff0c62ac40ee2c870 Mon Sep 17 00:00:00 2001 From: Trevor Mehard Date: Mon, 20 Jan 2020 16:00:52 -0500 Subject: [PATCH 2/9] Fixing lint errors --- .../src/components/query-list/queryList.tsx | 30 +++++++++---------- .../select/src/components/select/suggest.tsx | 7 ++--- 2 files changed, 17 insertions(+), 20 deletions(-) diff --git a/packages/select/src/components/query-list/queryList.tsx b/packages/select/src/components/query-list/queryList.tsx index 29b2039a18e..e7576d429ed 100644 --- a/packages/select/src/components/query-list/queryList.tsx +++ b/packages/select/src/components/query-list/queryList.tsx @@ -289,6 +289,21 @@ export class QueryList extends AbstractComponent2, IQueryL } } + public setActiveItem(activeItem: T | ICreateNewItem | null) { + this.expectedNextActiveItem = activeItem; + if (this.props.activeItem === undefined) { + // indicate that the active item may need to be scrolled into view after update. + this.shouldCheckActiveItemInViewport = true; + this.setState({ activeItem }); + } + + if (isCreateNewItem(activeItem)) { + Utils.safeInvoke(this.props.onActiveItemChange, null, true); + } else { + Utils.safeInvoke(this.props.onActiveItemChange, activeItem, false); + } + } + /** default `itemListRenderer` implementation */ private renderItemList = (listProps: IItemListRendererProps) => { const { initialContent, noResults } = this.props; @@ -486,21 +501,6 @@ export class QueryList extends AbstractComponent2, IQueryL return getFirstEnabledItem(this.state.filteredItems, this.props.itemDisabled, direction, startIndex); } - public setActiveItem(activeItem: T | ICreateNewItem | null) { - this.expectedNextActiveItem = activeItem; - if (this.props.activeItem === undefined) { - // indicate that the active item may need to be scrolled into view after update. - this.shouldCheckActiveItemInViewport = true; - this.setState({ activeItem }); - } - - if (isCreateNewItem(activeItem)) { - Utils.safeInvoke(this.props.onActiveItemChange, null, true); - } else { - Utils.safeInvoke(this.props.onActiveItemChange, activeItem, false); - } - } - private isCreateItemRendered(): boolean { return ( this.canCreateItems() && diff --git a/packages/select/src/components/select/suggest.tsx b/packages/select/src/components/select/suggest.tsx index 3445d11a852..0c8cb0ef029 100644 --- a/packages/select/src/components/select/suggest.tsx +++ b/packages/select/src/components/select/suggest.tsx @@ -289,11 +289,8 @@ export class Suggest extends React.PureComponent, ISuggestSt // The activeItem should always be the selectedItem when the Popover is first opened // if the activeItem prop is not set. Set the activeItem on close so that there isn't // a flash of the activeItem on screen. - const shouldResetActiveItemToSelectedItem = ( - this.props.activeItem === undefined && - this.state.selectedItem && - !this.props.resetOnSelect - ); + const shouldResetActiveItemToSelectedItem = + this.props.activeItem === undefined && this.state.selectedItem && !this.props.resetOnSelect; if (this.queryList && shouldResetActiveItemToSelectedItem) { // If the selectedItem prop is set then use it. From 7a8d0f3fbc28f746a50cf4187f113ec7cfd7699d Mon Sep 17 00:00:00 2001 From: Trevor Mehard Date: Tue, 4 Feb 2020 17:44:33 -0500 Subject: [PATCH 3/9] Adding explicit checks for null. Rename parameter to reflect DOM element. --- packages/select/src/components/select/suggest.tsx | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/packages/select/src/components/select/suggest.tsx b/packages/select/src/components/select/suggest.tsx index 0c8cb0ef029..864ee929e99 100644 --- a/packages/select/src/components/select/suggest.tsx +++ b/packages/select/src/components/select/suggest.tsx @@ -285,21 +285,21 @@ export class Suggest extends React.PureComponent, ISuggestSt Utils.safeInvokeMember(this.props.popoverProps, "onOpened", node); }; - private handlePopoverClosed = (node: HTMLElement) => { + private handlePopoverClosed = (element: HTMLElement) => { // The activeItem should always be the selectedItem when the Popover is first opened // if the activeItem prop is not set. Set the activeItem on close so that there isn't // a flash of the activeItem on screen. const shouldResetActiveItemToSelectedItem = - this.props.activeItem === undefined && this.state.selectedItem && !this.props.resetOnSelect; + this.props.activeItem === undefined && this.state.selectedItem !== null && !this.props.resetOnSelect; - if (this.queryList && shouldResetActiveItemToSelectedItem) { + if (this.queryList !== null && shouldResetActiveItemToSelectedItem) { // If the selectedItem prop is set then use it. // If not fall back to component state. - const selectedItem = this.props.selectedItem || this.state.selectedItem; + const selectedItem = this.props.selectedItem ?? this.state.selectedItem; this.queryList.setActiveItem(selectedItem); } - Utils.safeInvokeMember(this.props.popoverProps, "onClosed", node); + Utils.safeInvokeMember(this.props.popoverProps, "onClosed", element); }; private getTargetKeyDownHandler = ( From a4346e58207ae94e5408c20f9eedb1c180d1db15 Mon Sep 17 00:00:00 2001 From: Trevor Mehard Date: Sat, 8 Feb 2020 12:03:45 -0500 Subject: [PATCH 4/9] Adding test for active item reset for review --- packages/select/test/suggestTests.tsx | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/packages/select/test/suggestTests.tsx b/packages/select/test/suggestTests.tsx index bde12edff73..83dfddc25f8 100644 --- a/packages/select/test/suggestTests.tsx +++ b/packages/select/test/suggestTests.tsx @@ -99,6 +99,26 @@ describe("Suggest", () => { assert.strictEqual(scrollActiveItemIntoViewSpy.callCount, 1, "should call scrollActiveItemIntoView"); }); + it("sets active item to the selected item when the popover is closed", () => { + const ITEM_INDEX = 4; + const wrapper = suggest(); + const queryList = ((wrapper.instance() as Suggest) as any).queryList; // private ref + console.log("INITIAL STATE", queryList.state.activeItem, wrapper.state().selectedItem) + simulateFocus(wrapper); + assert.isTrue(wrapper.state().isOpen); + selectItem(wrapper, ITEM_INDEX); + assert.isFalse(wrapper.state().isOpen); + simulateFocus(wrapper); + assert.isTrue(wrapper.state().isOpen); + simulateKeyDown(wrapper, Keys.ARROW_DOWN); + console.log("AFTER KEY DOWN", queryList.state.activeItem, wrapper.state().selectedItem) + console.log("BEFORE BLUR", queryList.state.activeItem, wrapper.state().selectedItem) + simulateKeyDown(wrapper, Keys.ESCAPE); + assert.isFalse(wrapper.state().isOpen); + console.log("AFTER BLUR", queryList.state.activeItem, wrapper.state().selectedItem) + assert.deepEqual(queryList.state.activeItem, wrapper.state().selectedItem); + }); + function checkKeyDownDoesNotOpenPopover(wrapper: ReactWrapper, which: number) { simulateKeyDown(wrapper, which); assert.isFalse(wrapper.state().isOpen, "should not open popover"); From 4c3c5b235e91fde76efd1c3ca897d6db85a1b585 Mon Sep 17 00:00:00 2001 From: Adi Dahiya Date: Thu, 20 Feb 2020 03:10:42 -0500 Subject: [PATCH 5/9] fix tests and behavior --- .../src/components/query-list/queryList.tsx | 8 ++++- .../select/src/components/select/suggest.tsx | 32 +++++++++++-------- packages/select/test/suggestTests.tsx | 31 ++++++++++-------- 3 files changed, 44 insertions(+), 27 deletions(-) diff --git a/packages/select/src/components/query-list/queryList.tsx b/packages/select/src/components/query-list/queryList.tsx index e7576d429ed..3be262da16e 100644 --- a/packages/select/src/components/query-list/queryList.tsx +++ b/packages/select/src/components/query-list/queryList.tsx @@ -30,6 +30,12 @@ import { } from "../../common"; export interface IQueryListProps extends IListItemsProps { + /** + * Initial active item, useful if the parent component is controlling its selectedItem but + * not activeItem. + */ + initialActiveItem?: T; + /** * Callback invoked when user presses a key, after processing `QueryList`'s own key events * (up/down to navigate active item). This callback is passed to `renderer` and (along with @@ -171,7 +177,7 @@ export class QueryList extends AbstractComponent2, IQueryL activeItem: props.activeItem !== undefined ? props.activeItem - : getFirstEnabledItem(filteredItems, props.itemDisabled), + : props.initialActiveItem ?? getFirstEnabledItem(filteredItems, props.itemDisabled), createNewItem, filteredItems, query, diff --git a/packages/select/src/components/select/suggest.tsx b/packages/select/src/components/select/suggest.tsx index 864ee929e99..c7dcc886ef3 100644 --- a/packages/select/src/components/select/suggest.tsx +++ b/packages/select/src/components/select/suggest.tsx @@ -129,6 +129,7 @@ export class Suggest extends React.PureComponent, ISuggestSt return ( extends React.PureComponent, ISuggestSt this.setState({ selectedItem: this.props.selectedItem }); } + if (this.state.isOpen === false && prevState.isOpen === true) { + // just closed, likely by keyboard interaction + this.maybeResetActiveItemToSelectedItem(); + } + if (this.state.isOpen && !prevState.isOpen && this.queryList != null) { this.queryList.scrollActiveItemIntoView(); } @@ -286,19 +292,7 @@ export class Suggest extends React.PureComponent, ISuggestSt }; private handlePopoverClosed = (element: HTMLElement) => { - // The activeItem should always be the selectedItem when the Popover is first opened - // if the activeItem prop is not set. Set the activeItem on close so that there isn't - // a flash of the activeItem on screen. - const shouldResetActiveItemToSelectedItem = - this.props.activeItem === undefined && this.state.selectedItem !== null && !this.props.resetOnSelect; - - if (this.queryList !== null && shouldResetActiveItemToSelectedItem) { - // If the selectedItem prop is set then use it. - // If not fall back to component state. - const selectedItem = this.props.selectedItem ?? this.state.selectedItem; - this.queryList.setActiveItem(selectedItem); - } - + this.maybeResetActiveItemToSelectedItem(); Utils.safeInvokeMember(this.props.popoverProps, "onClosed", element); }; @@ -338,4 +332,16 @@ export class Suggest extends React.PureComponent, ISuggestSt Utils.safeInvokeMember(this.props.inputProps, "onKeyUp", evt); }; }; + + private maybeResetActiveItemToSelectedItem() { + // The activeItem should always be the selectedItem when the Popover is first opened + // if the activeItem prop is not set. Set the activeItem on close so that there isn't + // a flash of the activeItem on screen. + const shouldResetActiveItemToSelectedItem = + this.props.activeItem === undefined && this.state.selectedItem !== null && !this.props.resetOnSelect; + + if (this.queryList !== null && shouldResetActiveItemToSelectedItem) { + this.queryList.setActiveItem(this.props.selectedItem ?? this.state.selectedItem); + } + } } diff --git a/packages/select/test/suggestTests.tsx b/packages/select/test/suggestTests.tsx index 83dfddc25f8..83b5868f0d5 100644 --- a/packages/select/test/suggestTests.tsx +++ b/packages/select/test/suggestTests.tsx @@ -22,7 +22,7 @@ import * as sinon from "sinon"; import { IFilm, renderFilm, TOP_100_FILMS } from "../../docs-app/src/examples/select-examples/films"; import { ISuggestProps, ISuggestState, Suggest } from "../src/components/select/suggest"; -import { IItemRendererProps } from "../src/index"; +import { IItemRendererProps, QueryList } from "../src/index"; import { selectComponentSuite } from "./selectComponentSuite"; describe("Suggest", () => { @@ -100,22 +100,27 @@ describe("Suggest", () => { }); it("sets active item to the selected item when the popover is closed", () => { - const ITEM_INDEX = 4; - const wrapper = suggest(); - const queryList = ((wrapper.instance() as Suggest) as any).queryList; // private ref - console.log("INITIAL STATE", queryList.state.activeItem, wrapper.state().selectedItem) - simulateFocus(wrapper); - assert.isTrue(wrapper.state().isOpen); - selectItem(wrapper, ITEM_INDEX); - assert.isFalse(wrapper.state().isOpen); + const wrapper = suggest({ selectedItem: TOP_100_FILMS[10] }); + const queryList = ((wrapper.instance() as Suggest) as any).queryList as QueryList; // private ref + + assert.deepEqual( + queryList.state.activeItem, + wrapper.state().selectedItem, + "QueryList activeItem should be set to the controlled selectedItem if prop is provided", + ); + simulateFocus(wrapper); assert.isTrue(wrapper.state().isOpen); - simulateKeyDown(wrapper, Keys.ARROW_DOWN); - console.log("AFTER KEY DOWN", queryList.state.activeItem, wrapper.state().selectedItem) - console.log("BEFORE BLUR", queryList.state.activeItem, wrapper.state().selectedItem) + + const newActiveItem = TOP_100_FILMS[11]; + queryList.setActiveItem(newActiveItem); + assert.deepEqual(queryList.state.activeItem, newActiveItem); + simulateKeyDown(wrapper, Keys.ESCAPE); assert.isFalse(wrapper.state().isOpen); - console.log("AFTER BLUR", queryList.state.activeItem, wrapper.state().selectedItem) + + wrapper.update(); + wrapper.find(QueryList).update(); assert.deepEqual(queryList.state.activeItem, wrapper.state().selectedItem); }); From 13c4f9f1640c0166058125702b178fd4d91203c3 Mon Sep 17 00:00:00 2001 From: Adi Dahiya Date: Thu, 20 Feb 2020 03:14:14 -0500 Subject: [PATCH 6/9] remove unused popover handler --- packages/select/src/components/select/suggest.tsx | 6 ------ 1 file changed, 6 deletions(-) diff --git a/packages/select/src/components/select/suggest.tsx b/packages/select/src/components/select/suggest.tsx index c7dcc886ef3..d2877410bd6 100644 --- a/packages/select/src/components/select/suggest.tsx +++ b/packages/select/src/components/select/suggest.tsx @@ -185,7 +185,6 @@ export class Suggest extends React.PureComponent, ISuggestSt popoverClassName={classNames(Classes.SELECT_POPOVER, popoverProps.popoverClassName)} onOpening={this.handlePopoverOpening} onOpened={this.handlePopoverOpened} - onClosed={this.handlePopoverClosed} > extends React.PureComponent, ISuggestSt Utils.safeInvokeMember(this.props.popoverProps, "onOpened", node); }; - private handlePopoverClosed = (element: HTMLElement) => { - this.maybeResetActiveItemToSelectedItem(); - Utils.safeInvokeMember(this.props.popoverProps, "onClosed", element); - }; - private getTargetKeyDownHandler = ( handleQueryListKeyDown: React.EventHandler>, ) => { From 948bba70e98f418395292a9ccfbfe45a16b43406 Mon Sep 17 00:00:00 2001 From: Adi Dahiya Date: Tue, 25 Feb 2020 19:56:06 -0500 Subject: [PATCH 7/9] fix flash of content --- packages/select/src/components/select/suggest.tsx | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/packages/select/src/components/select/suggest.tsx b/packages/select/src/components/select/suggest.tsx index d2877410bd6..4217d574b83 100644 --- a/packages/select/src/components/select/suggest.tsx +++ b/packages/select/src/components/select/suggest.tsx @@ -125,7 +125,6 @@ export class Suggest extends React.PureComponent, ISuggestSt public render() { // omit props specific to this component, spread the rest. const { disabled, inputProps, popoverProps, ...restProps } = this.props; - return ( extends React.PureComponent, ISuggestSt if (this.state.isOpen === false && prevState.isOpen === true) { // just closed, likely by keyboard interaction - this.maybeResetActiveItemToSelectedItem(); + // wait until the transition ends so there isn't a flash of content in the popover + setTimeout(() => { + this.maybeResetActiveItemToSelectedItem(); + }, this.props.popoverProps?.transitionDuration ?? Popover.defaultProps.transitionDuration); } if (this.state.isOpen && !prevState.isOpen && this.queryList != null) { @@ -328,9 +330,6 @@ export class Suggest extends React.PureComponent, ISuggestSt }; private maybeResetActiveItemToSelectedItem() { - // The activeItem should always be the selectedItem when the Popover is first opened - // if the activeItem prop is not set. Set the activeItem on close so that there isn't - // a flash of the activeItem on screen. const shouldResetActiveItemToSelectedItem = this.props.activeItem === undefined && this.state.selectedItem !== null && !this.props.resetOnSelect; From 689fb7d6f92b15793e5fb129a48f37a0397c11f7 Mon Sep 17 00:00:00 2001 From: Adi Dahiya Date: Tue, 25 Feb 2020 20:15:00 -0500 Subject: [PATCH 8/9] fix test --- packages/select/test/suggestTests.tsx | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/packages/select/test/suggestTests.tsx b/packages/select/test/suggestTests.tsx index 83b5868f0d5..9d78f655c61 100644 --- a/packages/select/test/suggestTests.tsx +++ b/packages/select/test/suggestTests.tsx @@ -99,8 +99,9 @@ describe("Suggest", () => { assert.strictEqual(scrollActiveItemIntoViewSpy.callCount, 1, "should call scrollActiveItemIntoView"); }); - it("sets active item to the selected item when the popover is closed", () => { - const wrapper = suggest({ selectedItem: TOP_100_FILMS[10] }); + it("sets active item to the selected item when the popover is closed", (done) => { + // transition duration shorter than timeout below to ensure it's done + const wrapper = suggest({ selectedItem: TOP_100_FILMS[10], popoverProps: { transitionDuration: 5 } }); const queryList = ((wrapper.instance() as Suggest) as any).queryList as QueryList; // private ref assert.deepEqual( @@ -121,7 +122,10 @@ describe("Suggest", () => { wrapper.update(); wrapper.find(QueryList).update(); - assert.deepEqual(queryList.state.activeItem, wrapper.state().selectedItem); + setTimeout(() => { + assert.deepEqual(queryList.state.activeItem, wrapper.state().selectedItem); + done(); + }, 10); }); function checkKeyDownDoesNotOpenPopover(wrapper: ReactWrapper, which: number) { From 7c8158cbe38a03b89ed56880725eb050957552e2 Mon Sep 17 00:00:00 2001 From: Adi Dahiya Date: Tue, 25 Feb 2020 20:20:46 -0500 Subject: [PATCH 9/9] fix lint --- packages/select/test/suggestTests.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/select/test/suggestTests.tsx b/packages/select/test/suggestTests.tsx index 9d78f655c61..e754b3e317e 100644 --- a/packages/select/test/suggestTests.tsx +++ b/packages/select/test/suggestTests.tsx @@ -99,7 +99,7 @@ describe("Suggest", () => { assert.strictEqual(scrollActiveItemIntoViewSpy.callCount, 1, "should call scrollActiveItemIntoView"); }); - it("sets active item to the selected item when the popover is closed", (done) => { + it("sets active item to the selected item when the popover is closed", done => { // transition duration shorter than timeout below to ensure it's done const wrapper = suggest({ selectedItem: TOP_100_FILMS[10], popoverProps: { transitionDuration: 5 } }); const queryList = ((wrapper.instance() as Suggest) as any).queryList as QueryList; // private ref