-
Notifications
You must be signed in to change notification settings - Fork 1.2k
[Popover ] Fixed incorrect element being focused when closed #2255
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,42 @@ | ||
import {FOCUSABLE_SELECTOR} from '@shopify/javascript-utilities/focus'; | ||
import {isElementInViewport} from './is-element-in-viewport'; | ||
|
||
type Filter = (element: Element) => void; | ||
|
||
export function handleMouseUpByBlurring({ | ||
currentTarget, | ||
}: React.MouseEvent<HTMLAnchorElement | HTMLButtonElement>) { | ||
currentTarget.blur(); | ||
} | ||
|
||
export function nextFocusableNode( | ||
node: HTMLElement, | ||
filter?: Filter, | ||
): HTMLElement | Element | null { | ||
const allFocusableElements = [ | ||
...document.querySelectorAll(FOCUSABLE_SELECTOR), | ||
]; | ||
const sliceLocation = allFocusableElements.indexOf(node) + 1; | ||
const focusableElementsAfterNode = allFocusableElements.slice(sliceLocation); | ||
|
||
for (const focusableElement of focusableElementsAfterNode) { | ||
if ( | ||
isElementInViewport(focusableElement) && | ||
(!filter || (filter && filter(focusableElement))) | ||
) { | ||
return focusableElement; | ||
} | ||
} | ||
|
||
return null; | ||
} | ||
|
||
export function focusNextFocusableNode(node: HTMLElement, filter?: Filter) { | ||
const nextFocusable = nextFocusableNode(node, filter); | ||
if (nextFocusable && nextFocusable instanceof HTMLElement) { | ||
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. The |
||
nextFocusable.focus(); | ||
return true; | ||
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. Added a boolean return rather than void so we can determine if an element was focused or not |
||
} | ||
|
||
return false; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,10 @@ | ||
export function isElementInViewport(element: Element) { | ||
const {top, left, bottom, right} = element.getBoundingClientRect(); | ||
|
||
return ( | ||
top >= 0 && | ||
right <= window.innerWidth && | ||
bottom <= window.innerHeight && | ||
left >= 0 | ||
); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,14 +1,128 @@ | ||
import {MouseEvent} from 'react'; | ||
import {handleMouseUpByBlurring} from '../focus'; | ||
|
||
describe('focus', () => { | ||
describe('handleMouseUpByBlurring()', () => { | ||
it('calls blur on the currentTarget', () => { | ||
const currentTarget = document.createElement('button'); | ||
jest.spyOn(currentTarget, 'blur'); | ||
const mouseEvent = {currentTarget}; | ||
handleMouseUpByBlurring(mouseEvent as MouseEvent<HTMLButtonElement>); | ||
expect(currentTarget.blur).toHaveBeenCalled(); | ||
import { | ||
handleMouseUpByBlurring, | ||
focusNextFocusableNode, | ||
nextFocusableNode, | ||
} from '../focus'; | ||
|
||
describe('handleMouseUpByBlurring()', () => { | ||
it('calls blur on the currentTarget', () => { | ||
const currentTarget = document.createElement('button'); | ||
jest.spyOn(currentTarget, 'blur'); | ||
const mouseEvent = {currentTarget}; | ||
handleMouseUpByBlurring(mouseEvent as MouseEvent<HTMLButtonElement>); | ||
expect(currentTarget.blur).toHaveBeenCalled(); | ||
}); | ||
}); | ||
|
||
describe('nextFocusableNode', () => { | ||
it('does not return the initial element as the focusable node', () => { | ||
const {activator, otherNode} = domSetup(); | ||
|
||
expect(nextFocusableNode(activator)).toBe(otherNode); | ||
}); | ||
|
||
it('returns null when a focusable element is not found', () => { | ||
const {activator} = domSetup({ | ||
otherNodeTag: 'div', | ||
}); | ||
|
||
expect(nextFocusableNode(activator)).toBeNull(); | ||
}); | ||
|
||
it('filters out elements', () => { | ||
const {activator} = domSetup(); | ||
|
||
expect(nextFocusableNode(activator, () => false)).toBeNull(); | ||
}); | ||
|
||
it("returns the parent of an adjacent element when it's focusable", () => { | ||
const {activator, otherNode} = domSetup(); | ||
|
||
expect(nextFocusableNode(activator)).toBe(otherNode); | ||
}); | ||
|
||
it('searches adjacent elements for focusable children', () => { | ||
const {activator, otherNodeNested} = domSetup({ | ||
otherNodeTag: 'div', | ||
nested: true, | ||
}); | ||
|
||
expect(nextFocusableNode(activator)).toBe(otherNodeNested); | ||
}); | ||
|
||
it('searches parent elements for focusable children', () => { | ||
const {activator, parentsFocusableNode} = domSetup({ | ||
otherNodeTag: 'div', | ||
parents: true, | ||
}); | ||
|
||
expect(nextFocusableNode(activator)).toBe(parentsFocusableNode); | ||
}); | ||
}); | ||
|
||
describe('focusNextFocusableNode', () => { | ||
it('returns true when the node was focused', () => { | ||
const {activator} = domSetup(); | ||
|
||
expect(focusNextFocusableNode(activator)).toBe(true); | ||
}); | ||
|
||
it('returns false when the node was not focused', () => { | ||
const {activator} = domSetup({otherNodeTag: 'div'}); | ||
|
||
expect(focusNextFocusableNode(activator)).toBe(false); | ||
}); | ||
|
||
it('focused the node', () => { | ||
const {activator, otherNode} = domSetup(); | ||
|
||
focusNextFocusableNode(activator); | ||
|
||
expect(document.activeElement).toBe(otherNode); | ||
}); | ||
}); | ||
|
||
function domSetup( | ||
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. This is great, not asking to change it, but I wonder if it wouldn't have been simpler outline the dom being tested with each test. It makes it a little tricky to review and it's almost like |
||
options: { | ||
wrapperTag?: string; | ||
activatorTag?: string; | ||
otherNodeTag?: string; | ||
otherNodeNestedTag?: string; | ||
nested?: boolean; | ||
parents?: true; | ||
} = {}, | ||
) { | ||
const div = 'div'; | ||
const button = 'button'; | ||
const { | ||
wrapperTag = div, | ||
activatorTag = button, | ||
otherNodeTag = button, | ||
otherNodeNestedTag = button, | ||
nested, | ||
parents, | ||
} = options; | ||
const wrapper = document.createElement(wrapperTag); | ||
const activator = document.createElement(activatorTag); | ||
const otherNode = document.createElement(otherNodeTag); | ||
let otherNodeNested = null; | ||
let parentNode = null; | ||
let parentsFocusableNode = null; | ||
|
||
if (nested) { | ||
otherNodeNested = document.createElement(otherNodeNestedTag); | ||
otherNode.appendChild(otherNodeNested); | ||
} | ||
|
||
wrapper.append(activator, otherNode); | ||
|
||
if (parents) { | ||
parentNode = document.createElement(div); | ||
parentsFocusableNode = document.createElement(button); | ||
parentNode.append(wrapper, parentsFocusableNode); | ||
} | ||
|
||
document.body.appendChild(parentNode || wrapper); | ||
return {wrapper, activator, otherNode, otherNodeNested, parentsFocusableNode}; | ||
} |
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.
❤️