From 6639b330752fed4b249172fb9a33912c0352e6ea Mon Sep 17 00:00:00 2001 From: anveshmekala Date: Tue, 11 Jul 2023 16:11:51 -0500 Subject: [PATCH 1/7] fix(action-bar): no longer delegates focus when clicked on non-focusable region --- .../src/components/action-bar/action-bar.scss | 2 +- .../src/components/action-bar/action-bar.tsx | 61 +++++++++++++------ 2 files changed, 43 insertions(+), 20 deletions(-) diff --git a/packages/calcite-components/src/components/action-bar/action-bar.scss b/packages/calcite-components/src/components/action-bar/action-bar.scss index ac76c819ea1..07e5e1f1aa9 100755 --- a/packages/calcite-components/src/components/action-bar/action-bar.scss +++ b/packages/calcite-components/src/components/action-bar/action-bar.scss @@ -53,5 +53,5 @@ } .action-group--bottom { - @apply flex-grow justify-end; + @apply justify-end ms-auto; } diff --git a/packages/calcite-components/src/components/action-bar/action-bar.tsx b/packages/calcite-components/src/components/action-bar/action-bar.tsx index 3893c6c94ee..d6093614a19 100755 --- a/packages/calcite-components/src/components/action-bar/action-bar.tsx +++ b/packages/calcite-components/src/components/action-bar/action-bar.tsx @@ -53,14 +53,23 @@ import { @Component({ tag: "calcite-action-bar", styleUrl: "action-bar.scss", - shadow: { - delegatesFocus: true - }, + // shadow: { + // delegatesFocus: true + // }, + shadow: true, assetsDirs: ["assets"] }) export class ActionBar implements ConditionalSlotComponent, LoadableComponent, LocalizedComponent, T9nComponent { + //-------------------------------------------------------------------------- + // + // Element + // + //-------------------------------------------------------------------------- + + @Element() el: HTMLCalciteActionBarElement; + // -------------------------------------------------------------------------- // // Properties @@ -157,18 +166,6 @@ export class ActionBar // // -------------------------------------------------------------------------- - @Element() el: HTMLCalciteActionBarElement; - - mutationObserver = createObserver("mutation", () => { - const { el, expanded } = this; - toggleChildActionText({ el, expanded }); - this.overflowActions(); - }); - - resizeObserver = createObserver("resize", (entries) => this.resizeHandlerEntries(entries)); - - expandToggleEl: HTMLCalciteActionElement; - @State() effectiveLocale: string; @Watch("effectiveLocale") @@ -178,6 +175,20 @@ export class ActionBar @State() defaultMessages: ActionBarMessages; + @State() groups: HTMLCalciteActionGroupElement[]; + + bottomActionsGroupEl: HTMLCalciteActionGroupElement; + + expandToggleEl: HTMLCalciteActionElement; + + mutationObserver = createObserver("mutation", () => { + const { el, expanded } = this; + toggleChildActionText({ el, expanded }); + this.overflowActions(); + }); + + resizeObserver = createObserver("resize", (entries) => this.resizeHandlerEntries(entries)); + // -------------------------------------------------------------------------- // // Lifecycle @@ -245,7 +256,9 @@ export class ActionBar async setFocus(): Promise { await componentLoaded(this); - this.el?.focus(); + this.groups?.length > 0 + ? await this.groups[0].setFocus() + : await this.bottomActionsGroupEl.setFocus(); } // -------------------------------------------------------------------------- @@ -324,6 +337,10 @@ export class ActionBar this.expandToggleEl = el; }; + setBottomActionsRef = (el: HTMLCalciteActionGroupElement): void => { + this.bottomActionsGroupEl = el; + }; + updateGroups(): void { this.setGroupLayout(Array.from(this.el.querySelectorAll("calcite-action-group"))); } @@ -333,11 +350,11 @@ export class ActionBar } handleDefaultSlotChange = (event: Event): void => { - const groups = slotChangeGetAssignedElements(event).filter((el) => + this.groups = slotChangeGetAssignedElements(event).filter((el) => el?.matches("calcite-action-group") ) as HTMLCalciteActionGroupElement[]; - this.setGroupLayout(groups); + this.setGroupLayout(this.groups); }; // -------------------------------------------------------------------------- @@ -367,7 +384,13 @@ export class ActionBar ) : null; return getSlotted(el, SLOTS.bottomActions) || expandToggleNode ? ( - + {expandToggleNode} From 0984ec5f55b210d224ca67a4c4d54f6a693b9cdf Mon Sep 17 00:00:00 2001 From: anveshmekala Date: Wed, 12 Jul 2023 11:33:19 -0500 Subject: [PATCH 2/7] add e2e test --- .../src/components/action-bar/action-bar.e2e.ts | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/packages/calcite-components/src/components/action-bar/action-bar.e2e.ts b/packages/calcite-components/src/components/action-bar/action-bar.e2e.ts index d1948791607..f4a0bbfe9ef 100755 --- a/packages/calcite-components/src/components/action-bar/action-bar.e2e.ts +++ b/packages/calcite-components/src/components/action-bar/action-bar.e2e.ts @@ -3,6 +3,7 @@ import { html } from "../../../support/formatting"; import { accessible, defaults, focusable, hidden, reflects, renders, slots, t9n } from "../../tests/commonTests"; import { CSS, SLOTS } from "./resources"; import { overflowActionsDebounceInMs } from "./utils"; +import { getFocusedElementProp } from "../../tests/utils"; describe("calcite-action-bar", () => { describe("renders", () => { @@ -247,6 +248,21 @@ describe("calcite-action-bar", () => { focusTargetSelector: "calcite-action" } ); + + it("should not focus any action element when clicked on non-focusable region", async () => { + const page = await newE2EPage(); + await page.setContent(html` + `); + + const actionBarElRect = await page.evaluate(() => { + const actionBarEl = document.querySelector("calcite-action-bar"); + return actionBarEl.getBoundingClientRect().toJSON(); + }); + + await page.mouse.click(actionBarElRect.right / 2, actionBarElRect.y + actionBarElRect.bottom / 2); + expect(await getFocusedElementProp(page, "tagName", { shadow: true })).not.toBe("CALCITE-ACTION"); + }); }); describe("slots", () => { From e2dfa2b00bb4916b6c8a642b4099e75788f177de Mon Sep 17 00:00:00 2001 From: anveshmekala Date: Wed, 12 Jul 2023 11:36:53 -0500 Subject: [PATCH 3/7] remove commented code --- .../src/components/action-bar/action-bar.tsx | 3 --- 1 file changed, 3 deletions(-) diff --git a/packages/calcite-components/src/components/action-bar/action-bar.tsx b/packages/calcite-components/src/components/action-bar/action-bar.tsx index 1890c22a63b..76a6dcf202f 100755 --- a/packages/calcite-components/src/components/action-bar/action-bar.tsx +++ b/packages/calcite-components/src/components/action-bar/action-bar.tsx @@ -53,9 +53,6 @@ import { @Component({ tag: "calcite-action-bar", styleUrl: "action-bar.scss", - // shadow: { - // delegatesFocus: true - // }, shadow: true, assetsDirs: ["assets"] }) From 17234e62375096aa9c8ea6a8d24ded25fa2ce215 Mon Sep 17 00:00:00 2001 From: anveshmekala Date: Wed, 12 Jul 2023 13:53:16 -0500 Subject: [PATCH 4/7] apply ms-auto only on horizontal layout --- .../src/components/action-bar/action-bar.scss | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/packages/calcite-components/src/components/action-bar/action-bar.scss b/packages/calcite-components/src/components/action-bar/action-bar.scss index 07e5e1f1aa9..5e280a479b5 100755 --- a/packages/calcite-components/src/components/action-bar/action-bar.scss +++ b/packages/calcite-components/src/components/action-bar/action-bar.scss @@ -20,6 +20,9 @@ :host([layout="horizontal"]) { @apply flex-row; + .action-group--bottom { + @apply ms-auto; + } } :host([layout="vertical"][overflow-actions-disabled]) { @@ -53,5 +56,5 @@ } .action-group--bottom { - @apply justify-end ms-auto; + @apply justify-end; } From b9aae879dff74dd93bfca31f792d54eb59c5078a Mon Sep 17 00:00:00 2001 From: anveshmekala Date: Wed, 12 Jul 2023 14:28:16 -0500 Subject: [PATCH 5/7] add margin-block for vertical layout --- .../src/components/action-bar/action-bar.scss | 3 +++ 1 file changed, 3 insertions(+) diff --git a/packages/calcite-components/src/components/action-bar/action-bar.scss b/packages/calcite-components/src/components/action-bar/action-bar.scss index 5e280a479b5..e23aaa874ee 100755 --- a/packages/calcite-components/src/components/action-bar/action-bar.scss +++ b/packages/calcite-components/src/components/action-bar/action-bar.scss @@ -16,6 +16,9 @@ :host([layout="vertical"]) { @apply flex-col; + .action-group--bottom { + margin-block-start: auto; + } } :host([layout="horizontal"]) { From 82996dc753378e038371637a9b1a8366ea920d58 Mon Sep 17 00:00:00 2001 From: anveshmekala Date: Wed, 12 Jul 2023 14:48:50 -0500 Subject: [PATCH 6/7] feedback changes --- .../src/components/action-bar/action-bar.tsx | 34 ++++++++----------- 1 file changed, 14 insertions(+), 20 deletions(-) diff --git a/packages/calcite-components/src/components/action-bar/action-bar.tsx b/packages/calcite-components/src/components/action-bar/action-bar.tsx index 76a6dcf202f..870748907b8 100755 --- a/packages/calcite-components/src/components/action-bar/action-bar.tsx +++ b/packages/calcite-components/src/components/action-bar/action-bar.tsx @@ -17,7 +17,7 @@ import { connectConditionalSlotComponent, disconnectConditionalSlotComponent } from "../../utils/conditionalSlot"; -import { getSlotted, slotChangeGetAssignedElements } from "../../utils/dom"; +import { focusFirstTabbable, getSlotted, slotChangeGetAssignedElements } from "../../utils/dom"; import { componentFocusable, LoadableComponent, @@ -163,6 +163,16 @@ export class ActionBar // // -------------------------------------------------------------------------- + mutationObserver = createObserver("mutation", () => { + const { el, expanded } = this; + toggleChildActionText({ el, expanded }); + this.overflowActions(); + }); + + resizeObserver = createObserver("resize", (entries) => this.resizeHandlerEntries(entries)); + + expandToggleEl: HTMLCalciteActionElement; + @State() effectiveLocale: string; @Watch("effectiveLocale") @@ -172,20 +182,6 @@ export class ActionBar @State() defaultMessages: ActionBarMessages; - @State() groups: HTMLCalciteActionGroupElement[]; - - bottomActionsGroupEl: HTMLCalciteActionGroupElement; - - expandToggleEl: HTMLCalciteActionElement; - - mutationObserver = createObserver("mutation", () => { - const { el, expanded } = this; - toggleChildActionText({ el, expanded }); - this.overflowActions(); - }); - - resizeObserver = createObserver("resize", (entries) => this.resizeHandlerEntries(entries)); - // -------------------------------------------------------------------------- // // Lifecycle @@ -253,9 +249,7 @@ export class ActionBar async setFocus(): Promise { await componentFocusable(this); - this.groups?.length > 0 - ? await this.groups[0].setFocus() - : await this.bottomActionsGroupEl.setFocus(); + focusFirstTabbable(this.el); } // -------------------------------------------------------------------------- @@ -347,11 +341,11 @@ export class ActionBar } handleDefaultSlotChange = (event: Event): void => { - this.groups = slotChangeGetAssignedElements(event).filter((el) => + const groups = slotChangeGetAssignedElements(event).filter((el) => el?.matches("calcite-action-group") ) as HTMLCalciteActionGroupElement[]; - this.setGroupLayout(this.groups); + this.setGroupLayout(groups); }; // -------------------------------------------------------------------------- From d3e8b497b0c4f8814999a4ffbc83802575a33a54 Mon Sep 17 00:00:00 2001 From: anveshmekala Date: Wed, 12 Jul 2023 14:51:04 -0500 Subject: [PATCH 7/7] revert bottomActions ref el --- .../src/components/action-bar/action-bar.tsx | 12 +----------- 1 file changed, 1 insertion(+), 11 deletions(-) diff --git a/packages/calcite-components/src/components/action-bar/action-bar.tsx b/packages/calcite-components/src/components/action-bar/action-bar.tsx index 870748907b8..04df478a53c 100755 --- a/packages/calcite-components/src/components/action-bar/action-bar.tsx +++ b/packages/calcite-components/src/components/action-bar/action-bar.tsx @@ -328,10 +328,6 @@ export class ActionBar this.expandToggleEl = el; }; - setBottomActionsRef = (el: HTMLCalciteActionGroupElement): void => { - this.bottomActionsGroupEl = el; - }; - updateGroups(): void { this.setGroupLayout(Array.from(this.el.querySelectorAll("calcite-action-group"))); } @@ -375,13 +371,7 @@ export class ActionBar ) : null; return getSlotted(el, SLOTS.bottomActions) || expandToggleNode ? ( - + {expandToggleNode}