feat(dialog): add focusTrapDisabled property for non-modal dialogs #11362
Conversation
github-actions
bot
added
the
enhancement
Elijbet
force-pushed
the
elijbet/10685-focusTrapDisabled-attribute-for-non-modal-dialogs
branch
from
3a1913a to
702b4c8
Compare
| @@ -366,6 +369,19 @@ export class Dialog | |||
| // #endregion | |||
|
|
|||
| // #region Private Methods | |||
|
|
|||
| private handleFocusTrapDisabled(focusTrapDisabled: boolean): void { | |||
| if (!this.open) { | |||
There was a problem hiding this comment.
shouldn't it deactivate if not open?
There was a problem hiding this comment.
The logic needs to handle changes to open, modal and focusTrapDisabled.
- if
openis false => do nothing modalis true orfocusTrapDisabledis false => activateFocusTrap- if
focusTrapDisabledis true and modal is false => deactivateFocusTrap - else deactivateFocusTrap
There was a problem hiding this comment.
I think this hasn't been resolved yet.
There was a problem hiding this comment.
I think the onOpen/onClose handles the deactivation part. We may not need this check at all. We can probably remove this function and call activateFocusTrap in its place
There was a problem hiding this comment.
Gotcha. Sounds like focusTrapDisabled is intended for use at "open" time. Maybe we can clarify this in the docs. cc @geospatialem @DitwanP
There was a problem hiding this comment.
Yeah, I think we handle that in multiple places. onClose takes care of deactivateFocusTrap, and the rest happens in
focusTrapDisabledOverride(): boolean { return !this.modal && this.focusTrapDisabled; }
| @@ -366,6 +369,19 @@ export class Dialog | |||
| // #endregion | |||
|
|
|||
| // #region Private Methods | |||
|
|
|||
| private handleFocusTrapDisabled(focusTrapDisabled: boolean): void { | |||
| if (!this.open) { | |||
There was a problem hiding this comment.
The logic needs to handle changes to open, modal and focusTrapDisabled.
- if
openis false => do nothing modalis true orfocusTrapDisabledis false => activateFocusTrap- if
focusTrapDisabledis true and modal is false => deactivateFocusTrap - else deactivateFocusTrap
packages/calcite-components/src/components/dialog/dialog.e2e.ts
Outdated
Show resolved
Hide resolved
…-non-modal-dialogs
|
@Elijbet can we rename
to |
Elijbet
changed the title
Elijbet
changed the title
packages/calcite-components/src/utils/focusTrapComponent.spec.ts
Outdated
Show resolved
Hide resolved
| @@ -112,4 +112,37 @@ describe("focusTrapComponent", () => { | |||
| expect(customFocusTrapStack).toHaveLength(1); | |||
| }); | |||
| }); | |||
| describe("focusTrapDisabledOverride", () => { | |||
| it("should activate focus trap when focusTrapDisabledOverride returns false", () => { | |||
| const fakeComponent = {} as FocusTrapComponent; | |||
There was a problem hiding this comment.
Let's DRY test setup by using beforeEach.
| expect(activeElementId).toBe(await outsideEl.getProperty("id")); | ||
| }); | ||
|
|
||
| it("cannot tab out of dialog when modal=true and focusTrapDisabled=true", async () => { |
There was a problem hiding this comment.
For completeness, can you add tests that cover focusTrapDisabled = false?
| @@ -366,6 +369,19 @@ export class Dialog | |||
| // #endregion | |||
|
|
|||
| // #region Private Methods | |||
|
|
|||
| private handleFocusTrapDisabled(focusTrapDisabled: boolean): void { | |||
| if (!this.open) { | |||
There was a problem hiding this comment.
I think this hasn't been resolved yet.
Related Issue: #10685
Summary
Add
focusTrapDisabledattribute to for non-modaldialogs.