New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(modal): no longer loses focus trap after clicking inside the component. #6434
fix(modal): no longer loses focus trap after clicking inside the component. #6434
Conversation
@@ -734,4 +734,48 @@ describe("calcite-popover", () => { | |||
shadowFocusTargetSelector: `.${CSS.closeButton}` | |||
})); | |||
}); | |||
|
|||
// refer to https://github.com/Esri/calcite-components/issues/5993 for context |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added this to provide context.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! WDYT about moving the comment inside the test, so if for whatever reason it's modified or moved, it goes with it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
make sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aside from updating the if statement to set tabindex="-1"
when needed, this LGTM!
src/utils/focusTrapComponent.ts
Outdated
@@ -42,12 +42,15 @@ export function connectFocusTrap(component: FocusTrapComponent): void { | |||
return; | |||
} | |||
|
|||
if (focusTrapEl.tabIndex == null) { | |||
focusTrapEl.tabIndex = -1; | |||
if (focusTrapEl.tabIndex === null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This if
block won't be executed as tabIndex
is a number and won't ever be null.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
when there is no tabIndex
defined which is the case for focusTrapEl in modal , the value will be null
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When no tabIndex
is defined, the value will be -1
if it's a non-focusable element by default. Otherwise, it will be 0
.
@@ -734,4 +734,48 @@ describe("calcite-popover", () => { | |||
shadowFocusTargetSelector: `.${CSS.closeButton}` | |||
})); | |||
}); | |||
|
|||
// refer to https://github.com/Esri/calcite-components/issues/5993 for context |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! WDYT about moving the comment inside the test, so if for whatever reason it's modified or moved, it goes with it?
**Related Issue:** #6454 ## Summary This fixes an issue caused by setting the `tabindex` attribute on the host element instead of the property ([previous behavior](#6434)). ### Notes * When `tabindex` was set, `focus-trap` seemed to move focus to the host as focus fallback even if focus was already set within the focus trap. We should look into only setting the host as focus fallback if there is no focusable content within the trap. * The `tabindex` setting logic was removed instead of being rolled back because the [previous version](https://github.com/Esri/calcite-components/blob/059d6eee6a8dc0a0d74db6e113d30cafebac25bb/src/utils/focusTrapComponent.ts#L33-L35) was misleading and would never be invoked (`HTMLElement.tabIndex` [is always an integer](https://developer.mozilla.org/en-US/docs/Web/API/HTMLElement/tabIndex#value)). * Some focus-related tests were fixed as they were not properly set up and produced false positives. * A test util was added to help match elements against the focused element.
Related Issue: #6281
Summary
This PR will fix
focusTrap
issue when user clicks inside themodal
. When user clicks inside the modal,focusTrap
behavior wont be affected.