-
Notifications
You must be signed in to change notification settings - Fork 69
Updates Menu Backdrop behavior #2767
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
Merged
Merged
Changes from all commits
Commits
Show all changes
28 commits
Select commit
Hold shift + click to select a range
8a71bc3
allow native event propagation
TheSonOfThomp 95bf339
removes backdrop click mousedown handler
TheSonOfThomp 9807f87
Update menu-trigger-propagation.md
TheSonOfThomp 63d7925
Merge branch 'main' into a/menu-bug
TheSonOfThomp bebe792
Revert "removes backdrop click mousedown handler"
TheSonOfThomp 5d1e0a7
updates menu tests
TheSonOfThomp e73c9e6
transition popover display property
TheSonOfThomp 464180c
Adds onBeforeToggle to Popover
TheSonOfThomp 5676204
Merge branch 'main' into a/menu-bug
TheSonOfThomp 8e1e6c9
Merge branch 'main' into a/menu-bug
TheSonOfThomp 82e5610
Update getPopoverRenderModeProps usage
TheSonOfThomp a5fec0d
cleanup GetPopoverRenderModePropsArgs types
TheSonOfThomp 0ed2bb6
creates getClosestFocusableElement
TheSonOfThomp 2a3e161
fix GetPopoverRenderModeProps export
TheSonOfThomp 6b3a932
updates menu clickaway test
TheSonOfThomp 4f1f09d
implement getClosestFocusableElement in Menu
TheSonOfThomp 3a61853
Create getClosestFocusableElement.spec.ts
TheSonOfThomp 10bd634
adds options to useBackdropClick
TheSonOfThomp 0dcd22e
check if event is click
TheSonOfThomp 87c82f9
add todo
TheSonOfThomp 60bd64a
Update menu-trigger-propagation.md
TheSonOfThomp 97211c1
Create backdrop-allow-propagation.md
TheSonOfThomp eff6644
Create closest-focusable-element.md
TheSonOfThomp 9b2376e
reverts popover changes
TheSonOfThomp 3126164
Merge branch 'main' into a/menu-bug
TheSonOfThomp 64bb1e1
Merge branch 'main' into a/menu-bug
TheSonOfThomp 87a8853
pr feedaback
TheSonOfThomp 94909bb
add jira ticket
TheSonOfThomp File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
--- | ||
'@leafygreen-ui/hooks': minor | ||
--- | ||
|
||
- Updates 3rd argument in `useBackdropClick` to accept an options object. Retains (but deprecates) boolean-only functionality. | ||
- Adds `options.allowPropagation` to allow or disallow the click event to bubble up to other elements. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
--- | ||
'@leafygreen-ui/lib': minor | ||
--- | ||
|
||
Adds `getClosestFocusableElement`. | ||
This function crawls up the DOM tree to find the closest focusable element to the provided element | ||
Returns the element itself if it is focusable or if no focusable element is found, it returns the body element. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
--- | ||
'@leafygreen-ui/menu': major | ||
--- | ||
|
||
- Re-enables native event propagation on trigger clicks. Previously this was done to prevent errors caused by different event handling in React vs Backbone. | ||
- Adds logic when the menu closes to check if the click occurred on an element that is focusable. If it is then we want to focus that element, otherwise we want to focus the menu trigger. | ||
- Moves `popoverRef` from the `ul` to the root popover `div` |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
112 changes: 112 additions & 0 deletions
112
packages/lib/src/getClosestFocusableElement/getClosestFocusableElement.spec.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,112 @@ | ||
import { getClosestFocusableElement } from './getClosestFocusableElement'; | ||
|
||
describe('getClosestFocusableElement', () => { | ||
// Setup and cleanup | ||
beforeEach(() => { | ||
document.body.innerHTML = ''; | ||
}); | ||
|
||
afterEach(() => { | ||
jest.restoreAllMocks(); | ||
}); | ||
|
||
describe('when element itself is focusable', () => { | ||
test('returns anchor element when element is an anchor', () => { | ||
const anchor = document.createElement('a'); | ||
document.body.appendChild(anchor); | ||
expect(getClosestFocusableElement(anchor)).toBe(anchor); | ||
}); | ||
|
||
test('returns button element when element is a button', () => { | ||
const button = document.createElement('button'); | ||
document.body.appendChild(button); | ||
expect(getClosestFocusableElement(button)).toBe(button); | ||
}); | ||
|
||
test('returns frame element when element is a frame', () => { | ||
const frame = document.createElement('frame'); | ||
document.body.appendChild(frame); | ||
expect(getClosestFocusableElement(frame)).toBe(frame); | ||
}); | ||
|
||
test('returns iframe element when element is an iframe', () => { | ||
const iframe = document.createElement('iframe'); | ||
document.body.appendChild(iframe); | ||
expect(getClosestFocusableElement(iframe)).toBe(iframe); | ||
}); | ||
|
||
test('returns input element when element is an input (not hidden)', () => { | ||
const input = document.createElement('input'); | ||
document.body.appendChild(input); | ||
expect(getClosestFocusableElement(input)).toBe(input); | ||
}); | ||
|
||
test('does not return input when type is hidden', () => { | ||
const hiddenInput = document.createElement('input'); | ||
hiddenInput.type = 'hidden'; | ||
const div = document.createElement('div'); | ||
div.appendChild(hiddenInput); | ||
document.body.appendChild(div); | ||
|
||
expect(getClosestFocusableElement(hiddenInput)).toBe(document.body); | ||
}); | ||
|
||
test('returns select element when element is a select', () => { | ||
const select = document.createElement('select'); | ||
document.body.appendChild(select); | ||
expect(getClosestFocusableElement(select)).toBe(select); | ||
}); | ||
|
||
test('returns textarea element when element is a textarea', () => { | ||
const textarea = document.createElement('textarea'); | ||
document.body.appendChild(textarea); | ||
expect(getClosestFocusableElement(textarea)).toBe(textarea); | ||
}); | ||
|
||
test('returns element with tabindex when element has tabindex', () => { | ||
const div = document.createElement('div'); | ||
div.setAttribute('tabindex', '0'); | ||
document.body.appendChild(div); | ||
expect(getClosestFocusableElement(div)).toBe(div); | ||
}); | ||
}); | ||
|
||
describe('when element is not focusable but has focusable parent', () => { | ||
test('returns the closest focusable parent', () => { | ||
const button = document.createElement('button'); | ||
const span = document.createElement('span'); | ||
const text = document.createElement('p'); | ||
|
||
button.appendChild(span); | ||
span.appendChild(text); | ||
document.body.appendChild(button); | ||
|
||
expect(getClosestFocusableElement(text)).toBe(button); | ||
}); | ||
|
||
test('returns the closest focusable ancestor at any level', () => { | ||
const anchor = document.createElement('a'); | ||
const div1 = document.createElement('div'); | ||
const div2 = document.createElement('div'); | ||
const div3 = document.createElement('div'); | ||
|
||
anchor.appendChild(div1); | ||
div1.appendChild(div2); | ||
div2.appendChild(div3); | ||
document.body.appendChild(anchor); | ||
|
||
expect(getClosestFocusableElement(div3)).toBe(anchor); | ||
}); | ||
}); | ||
|
||
describe('when no focusable element is found', () => { | ||
test('returns document.body when no focusable element exists in the chain', () => { | ||
const div = document.createElement('div'); | ||
const span = document.createElement('span'); | ||
div.appendChild(span); | ||
document.body.appendChild(div); | ||
|
||
expect(getClosestFocusableElement(span)).toBe(document.body); | ||
}); | ||
}); | ||
}); |
27 changes: 27 additions & 0 deletions
27
packages/lib/src/getClosestFocusableElement/getClosestFocusableElement.tsx
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,27 @@ | ||
const focusableSelectors = [ | ||
'a', | ||
'button', | ||
'frame', | ||
'iframe', | ||
'input:not([type=hidden])', | ||
'select', | ||
'textarea', | ||
'*[tabindex]', | ||
] as const; | ||
|
||
const focusableSelectorString = focusableSelectors.join(', '); | ||
|
||
/** | ||
* Crawls up the DOM tree (using `el.closest`) to find the closest focusable element. | ||
* Returns the element itself if it is focusable. | ||
* If no focusable element is found, it returns the body element. | ||
* | ||
* This is useful for ensuring that focus is returned to a valid element | ||
* after an event, such as a click or key press. | ||
*/ | ||
export const getClosestFocusableElement = (el: HTMLElement): HTMLElement => { | ||
const focusableElement = el.closest( | ||
focusableSelectorString, | ||
) as HTMLElement | null; | ||
return focusableElement ?? document.body; | ||
}; |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
export { getClosestFocusableElement } from './getClosestFocusableElement'; |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,6 +9,7 @@ import { | |
} from '@testing-library/react'; | ||
import userEvent from '@testing-library/user-event'; | ||
|
||
import Button from '@leafygreen-ui/button'; | ||
import { Optional } from '@leafygreen-ui/lib'; | ||
import { RenderMode } from '@leafygreen-ui/popover'; | ||
import { waitForTransition } from '@leafygreen-ui/testing-lib'; | ||
|
@@ -173,8 +174,8 @@ describe('packages/menu', () => { | |
expect(menuEl).toBeInTheDocument(); | ||
}); | ||
|
||
test('Clicking outside the menu closes the menu', async () => { | ||
const { openMenu, backdropEl } = renderMenu({}); | ||
test('Clicking outside the menu closes the menu, and keeps focus on the menu trigger', async () => { | ||
const { openMenu, backdropEl, triggerEl } = renderMenu({}); | ||
const { menuEl } = await openMenu(); | ||
|
||
expect(menuEl).toBeInTheDocument(); | ||
|
@@ -183,6 +184,34 @@ describe('packages/menu', () => { | |
await waitForElementToBeRemoved(menuEl); | ||
|
||
expect(menuEl).not.toBeInTheDocument(); | ||
expect(triggerEl).toHaveFocus(); | ||
}); | ||
|
||
test("Clicking a button outside the menu fires that button's handlers, and sets focus to the button", async () => { | ||
const otherButtonHandler = jest.fn(); | ||
const { getByTestId, findByTestId } = render( | ||
<> | ||
<Menu trigger={defaultTrigger} data-testid={menuTestId}> | ||
<MenuItem>Item A</MenuItem> | ||
<MenuItem>Item B</MenuItem> | ||
</Menu> | ||
<Button data-testid="outside-button" onClick={otherButtonHandler}> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
Outside Button | ||
</Button> | ||
</>, | ||
); | ||
const button = getByTestId('outside-button'); | ||
const trigger = getByTestId(menuTriggerTestId); | ||
userEvent.click(trigger); | ||
|
||
const menuEl = await findByTestId(menuTestId); | ||
|
||
userEvent.click(button); | ||
await waitForElementToBeRemoved(menuEl); | ||
await waitFor(() => { | ||
expect(otherButtonHandler).toHaveBeenCalled(); | ||
expect(button).toHaveFocus(); | ||
}); | ||
}); | ||
|
||
test('Click handlers on parent elements fire (propagation is not stopped)', async () => { | ||
|
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 a ticket?