From 30f968028577cfccbef38adef01c27859cc82677 Mon Sep 17 00:00:00 2001 From: JC Franco Date: Thu, 28 Sep 2023 18:43:16 -0700 Subject: [PATCH] fix(tree): fix tree keyboard selection issue --- .../src/components/tree-item/tree-item.tsx | 8 +- .../src/components/tree/tree.e2e.ts | 299 +++++++++++------- 2 files changed, 193 insertions(+), 114 deletions(-) diff --git a/packages/calcite-components/src/components/tree-item/tree-item.tsx b/packages/calcite-components/src/components/tree-item/tree-item.tsx index 17e0e3a187e..94ce9eb2ea4 100644 --- a/packages/calcite-components/src/components/tree-item/tree-item.tsx +++ b/packages/calcite-components/src/components/tree-item/tree-item.tsx @@ -358,15 +358,12 @@ export class TreeItem implements ConditionalSlotComponent, InteractiveComponent @Listen("keydown") keyDownHandler(event: KeyboardEvent): void { - if (this.isActionEndEvent(event)) { + if (this.isActionEndEvent(event) || event.defaultPrevented) { return; } switch (event.key) { case " ": - if (this.selectionMode === "none") { - return; - } this.userChangedValue = true; this.calciteInternalTreeItemSelect.emit({ modifyCurrentSelection: this.isSelectionMultiLike, @@ -375,9 +372,6 @@ export class TreeItem implements ConditionalSlotComponent, InteractiveComponent event.preventDefault(); break; case "Enter": - if (this.selectionMode === "none") { - return; - } // activates a node, i.e., performs its default action. For parent nodes, one possible default action is to open or close the node. In single-select trees where selection does not follow focus (see note below), the default action is typically to select the focused node. const link = Array.from(this.el.children).find((el) => el.matches("a") diff --git a/packages/calcite-components/src/components/tree/tree.e2e.ts b/packages/calcite-components/src/components/tree/tree.e2e.ts index 8c7d31845d9..6593e6370cd 100644 --- a/packages/calcite-components/src/components/tree/tree.e2e.ts +++ b/packages/calcite-components/src/components/tree/tree.e2e.ts @@ -1038,7 +1038,7 @@ describe("calcite-tree", () => { expect(keydownSpy.lastEvent.defaultPrevented).toBe(true); }); - it("does not prevent space/enter keyboard event on actions with selectionMode of none", async () => { + it("does prevent space/enter keyboard event on actions with selectionMode of none", async () => { const page = await newE2EPage(); await page.setContent(html`
@@ -1058,12 +1058,12 @@ describe("calcite-tree", () => { await page.keyboard.press("Enter"); expect(keydownSpy).toHaveReceivedEventTimes(1); - expect(keydownSpy.lastEvent.defaultPrevented).toBe(false); + expect(keydownSpy.lastEvent.defaultPrevented).toBe(true); await page.keyboard.press("Space"); expect(keydownSpy).toHaveReceivedEventTimes(2); - expect(keydownSpy.lastEvent.defaultPrevented).toBe(false); + expect(keydownSpy.lastEvent.defaultPrevented).toBe(true); }); }); @@ -1101,128 +1101,213 @@ describe("calcite-tree", () => { }); describe("parent node expansion", () => { + describe("keyboard", () => { + assertSelectionModes( + false, + async (_page, item) => { + await item.press("Enter"); + }, + async (_page, item, toggle) => { + const itemExpanded = await item.getProperty("expanded"); + const toggleKey = itemExpanded ? "ArrowLeft" : "ArrowRight"; + await toggle.press(toggleKey); + }, + async (_page, item) => { + await item.press("Enter"); + } + ); + }); + + describe("mouse", () => { + assertSelectionModes( + true, + async (page, item) => { + await directItemClick(page, item); + }, + async (page, _item, toggle) => { + await directItemClick(page, toggle); + }, + async (page, item) => { + await directItemClick(page, item); + } + ); + }); + interface SelectionModeTest { selectionMode: SelectionMode; - canDeselect: boolean; + canDeselect: { + item: boolean; + child: boolean; + }; expandableItemClick: { selectsItem: boolean; selectsChildren: boolean; }; } - const selectionModesTests: SelectionModeTest[] = [ - { - selectionMode: "ancestors", - canDeselect: true, - expandableItemClick: { - selectsItem: true, - selectsChildren: true, + async function assertSelectionModes( + childToggleTraversesParent: boolean, + selectItem: (page: E2EPage, item: E2EElement) => Promise, + toggleItem: (page: E2EPage, item: E2EElement, toggle: E2EElement) => Promise, + selectItemChild: (page: E2EPage, item: E2EElement) => Promise + ): Promise { + const selectionModesTests: SelectionModeTest[] = [ + { + selectionMode: "ancestors", + canDeselect: { item: true, child: true }, + expandableItemClick: { + selectsItem: true, + selectsChildren: true, + }, }, - }, - { - selectionMode: "multichildren", - canDeselect: true, - expandableItemClick: { - selectsItem: true, - selectsChildren: false, + { + selectionMode: "children", + canDeselect: { item: false, child: false }, + expandableItemClick: { + selectsItem: true, + selectsChildren: false, + }, }, - }, - { - selectionMode: "multiple", - canDeselect: true, - expandableItemClick: { - selectsItem: false, - selectsChildren: false, + { + selectionMode: "multichildren", + canDeselect: { item: true, child: true }, + expandableItemClick: { + selectsItem: true, + selectsChildren: false, + }, }, - }, - { - selectionMode: "none", - canDeselect: false, - expandableItemClick: { - selectsItem: false, - selectsChildren: false, + { + selectionMode: "multiple", + canDeselect: { item: true, child: true }, + expandableItemClick: { + selectsItem: false, + selectsChildren: false, + }, }, - }, - { - selectionMode: "single", - canDeselect: true, - expandableItemClick: { - selectsItem: false, - selectsChildren: false, + { + selectionMode: "none", + canDeselect: { item: false, child: false }, + expandableItemClick: { + selectsItem: false, + selectsChildren: false, + }, }, - }, - { - selectionMode: "single-persist", - canDeselect: false, - expandableItemClick: { - selectsItem: false, - selectsChildren: false, + { + selectionMode: "single", + canDeselect: { item: true, child: false }, + expandableItemClick: { + selectsItem: false, + selectsChildren: false, + }, }, - }, - ]; - - selectionModesTests.forEach( - ({ selectionMode, canDeselect, expandableItemClick: { selectsItem, selectsChildren } }) => { - it(`selection-mode = ${selectionMode}`, async () => { - const expandableItemId = "expandable-item"; - const page = await newE2EPage(); - await page.setContent(html` - - Child 1 - - - Child 2 - - - Grandchild 1 - - Grandchild 2 - - - Grandchild 3 - - Great-Grandchild 1 - Great-Grandchild 2 - Great-Grandchild 3 - - - - - - `); - - const tree = await page.find("calcite-tree"); - expect(await tree.getProperty("selectedItems")).toHaveLength(0); - - const expandableParentItem = await page.find(`#${expandableItemId}`); - const childItems = await expandableParentItem.findAll("calcite-tree-item"); - expect(await expandableParentItem.getProperty("expanded")).toBe(false); - - await directItemClick(page, expandableParentItem); - - expect(await expandableParentItem.getProperty("expanded")).toBe(!selectsItem); - const expectedSelectedItemsAfterExpanding = (selectsItem ? 1 : 0) + (selectsChildren ? childItems.length : 0); - expect(await tree.getProperty("selectedItems")).toHaveLength(expectedSelectedItemsAfterExpanding); - - await directItemClick(page, expandableParentItem); - - expect(await expandableParentItem.getProperty("expanded")).toBe(false); - const expectedSelectedItemsAfterCollapsing = canDeselect ? 0 : expectedSelectedItemsAfterExpanding; - expect(await tree.getProperty("selectedItems")).toHaveLength(expectedSelectedItemsAfterCollapsing); + { + selectionMode: "single-persist", + canDeselect: { item: false, child: false }, + expandableItemClick: { + selectsItem: false, + selectsChildren: false, + }, + }, + ]; - const expandableParentToggle = await page.find(`#${expandableItemId} >>> .${CSS.chevron}`); + selectionModesTests.forEach( + ({ selectionMode, canDeselect, expandableItemClick: { selectsItem, selectsChildren } }) => { + it(`selection-mode = ${selectionMode}`, async () => { + const expandableItemId = "expandable-item"; + const expandableItemChildId = "expandable-item-child"; + const page = await newE2EPage(); + await page.setContent(html` + + Child 1 - await directItemClick(page, expandableParentToggle); + + Child 2 - expect(await expandableParentItem.getProperty("expanded")).toBe(true); - expect(await tree.getProperty("selectedItems")).toHaveLength(expectedSelectedItemsAfterCollapsing); + + Grandchild 1 - await directItemClick(page, expandableParentToggle); + Grandchild 2 - expect(await expandableParentItem.getProperty("expanded")).toBe(false); - expect(await tree.getProperty("selectedItems")).toHaveLength(expectedSelectedItemsAfterCollapsing); - }); - } - ); + + Grandchild 3 + + Great-Grandchild 1 + Great-Grandchild 2 + Great-Grandchild 3 + + + + + + `); + + const tree = await page.find("calcite-tree"); + expect(await tree.getProperty("selectedItems")).toHaveLength(0); + + const expandableParentItem = await page.find(`#${expandableItemId}`); + const childItems = await expandableParentItem.findAll("calcite-tree-item"); + expect(await expandableParentItem.getProperty("expanded")).toBe(false); + + await selectItem(page, expandableParentItem); + + expect(await expandableParentItem.getProperty("expanded")).toBe(!selectsItem); + const expectedSelectedItemsAfterExpanding = + (selectsItem ? 1 : 0) + (selectsChildren ? childItems.length : 0); + expect(await tree.getProperty("selectedItems")).toHaveLength(expectedSelectedItemsAfterExpanding); + + await selectItem(page, expandableParentItem); + + expect(await expandableParentItem.getProperty("expanded")).toBe(false); + const expectedSelectedItemsAfterCollapsing = canDeselect.item ? 0 : expectedSelectedItemsAfterExpanding; + expect(await tree.getProperty("selectedItems")).toHaveLength(expectedSelectedItemsAfterCollapsing); + + const expandableParentToggle = await page.find(`#${expandableItemId} >>> .${CSS.chevron}`); + + await toggleItem(page, expandableParentItem, expandableParentToggle); + + expect(await expandableParentItem.getProperty("expanded")).toBe(true); + expect(await tree.getProperty("selectedItems")).toHaveLength(expectedSelectedItemsAfterCollapsing); + + await toggleItem(page, expandableParentItem, expandableParentToggle); + + expect(await expandableParentItem.getProperty("expanded")).toBe(false); + expect(await tree.getProperty("selectedItems")).toHaveLength(expectedSelectedItemsAfterCollapsing); + + const expandableChildItem = await page.find(`#${expandableItemChildId}`); + + await selectItemChild(page, expandableChildItem); + + expect(await expandableParentItem.getProperty("expanded")).toBe( + !selectsItem && !childToggleTraversesParent + ); + expect(await tree.getProperty("selectedItems")).toHaveLength( + selectionMode === "none" + ? 0 + : selectionMode === "ancestors" && !childToggleTraversesParent + ? 7 + : !childToggleTraversesParent && + (selectionMode === "multiple" || selectionMode === "single" || selectionMode === "single-persist") + ? 0 + : 1 + ); + + await selectItemChild(page, expandableChildItem); + + expect(await expandableParentItem.getProperty("expanded")).toBe( + !selectsItem && !childToggleTraversesParent + ); + expect(await tree.getProperty("selectedItems")).toHaveLength( + selectionMode === "none" + ? 0 + : !childToggleTraversesParent && selectionMode === "multiple" + ? 1 + : canDeselect.child + ? 0 + : 1 + ); + }); + } + ); + } }); });