Skip to content
This repository has been archived by the owner on Oct 6, 2020. It is now read-only.

fix(Dropdown): Remove timeouts, Fix ref bug #42

Merged
merged 20 commits into from
May 15, 2019
Merged

fix(Dropdown): Remove timeouts, Fix ref bug #42

Show file tree
Hide file tree
Changes from 18 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
45dd450
Remove timeout, fix ref bug
May 2, 2019
7bae6a8
Use enzyme for blur test, add trigger test cases
May 6, 2019
cb44d1e
Restore key event handler logic on trigger
May 6, 2019
2dd1c7f
Tidy
May 6, 2019
89fca34
Fix tests
May 6, 2019
40f1692
PR comment: restore blur test for reference
May 7, 2019
595339c
WIP: Use act
May 9, 2019
f2ed2e8
Remove enzyme detritus
May 9, 2019
a99e2ee
Restore enzyme tests for blur and keypress trigger
May 13, 2019
b8d9e44
Add trigger event
May 13, 2019
3fce379
Remove enzyme tests
May 13, 2019
85b96d8
Remove enzyme test setup
May 13, 2019
4b0769b
Restore test setup
May 13, 2019
b5deb31
PR comments
May 14, 2019
b81b6a6
PR comment: reimplement returnFocus
May 14, 2019
ddc6750
Restore example
May 14, 2019
be7b722
Use relatedTarget
May 15, 2019
1206b4f
Tidy
May 15, 2019
168aa0d
Enable test
May 15, 2019
4ca9453
Tidy
May 15, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 13 additions & 14 deletions src/Dropdown/Dropdown.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,8 @@ export default function Dropdown({
}) {
const popperRef = useRef();
const menuRef = useRef();
const triggerRef = useRef();

const [isOpen, setOpen] = useState(false);

const open = () => setOpen(true);
Expand All @@ -62,6 +64,7 @@ export default function Dropdown({
// Allow all clicks and, for non-button elements, Enter and Space to toggle Dropdown
// https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Roles/button_role#Required_JavaScript_Features
if (e.type === 'click' || (e.type === 'keypress' && (e.which === 13 || e.which === 32))) {
e.preventDefault();
cehsu marked this conversation as resolved.
Show resolved Hide resolved
e.stopPropagation();
toggle();
}
Expand All @@ -71,20 +74,22 @@ export default function Dropdown({
useEffect(() => {
if (isOpen) {
setTimeout(() => {
menuRef.current.focus();
if (menuRef.current) menuRef.current.focus();
});
}
}, [isOpen]);

useKeyPress('Escape', () => {
if (isOpen) {
triggerRef.current.focus();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We manually implement returnFocus here, so that inputs can be directly focused (without a second click), which was prevented by using FocusLock's returnFocus.

close();
}
});

useKeyPress(ARROW_KEYS, event => {
if (isOpen && menuRef.current) {
event.preventDefault();
event.stopPropagation();
const focusArgs = [menuRef.current, document.activeElement];
const nextFocusable =
event.key === 'ArrowUp' ? findPreviousFocusableElement(...focusArgs) : findNextFocusableElement(...focusArgs);
Expand All @@ -95,16 +100,9 @@ export default function Dropdown({
}
});

const handleMenuBlur = () => {
if (autoclose) {
// Use timeout to delay examination of activeElement until after blur/focus
// events have been processed.
setTimeout(() => {
Copy link
Contributor Author

@cehsu cehsu May 6, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This setTimeout was causing a bug with blur when inputs were clicked. Managing whether blur is being triggered via the state now allows inputs clicks to trigger blur. Blur is now handled via relatedTarget.

const nextActiveElement = document.activeElement;
if (menuRef.current && !menuRef.current.contains(nextActiveElement)) {
close();
}
});
const handleMenuBlur = e => {
if (autoclose && menuRef.current && !menuRef.current.contains(e.relatedTarget)) {
kylealwyn marked this conversation as resolved.
Show resolved Hide resolved
close();
}
};

Expand All @@ -114,15 +112,16 @@ export default function Dropdown({
<DropdownContext.Provider value={{ close }}>
<Manager>
<Reference>
{({ ref: triggerRef }) => (
<Trigger style={styles.Trigger} ref={triggerRef} tabIndex={-1}>
{({ ref: passedTriggerRef }) => (
<Trigger style={styles.Trigger} ref={passedTriggerRef} tabIndex={-1}>
{React.cloneElement(trigger, {
role: trigger.role || 'button',
tabIndex: trigger.tabIndex || 0,
'aria-haspopup': true,
'aria-expanded': isOpen,
onClick: handleTrigger,
onKeyPress: handleTrigger,
ref: triggerRef,
style: {
cursor: 'pointer',
...(trigger.style || {}),
Expand Down Expand Up @@ -155,7 +154,7 @@ export default function Dropdown({
{({ ref, style }) => (
<Transition in={isOpen} timeout={0} appear>
{state => (
<FocusTrap returnFocus autoFocus={false}>
<FocusTrap autoFocus={false}>
<DropdownMenu
ref={menuInner => {
menuRef.current = menuInner;
Expand Down
4 changes: 3 additions & 1 deletion src/Dropdown/Dropdown.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ describe('<Dropdown />', () => {
expect(renderUtils.asFragment()).toMatchSnapshot();
});

// TODO: add cases for space and enter keypress events
test('opens menu with focus when trigger is clicked', async () => {
await openDropdown();
expect(renderUtils.asFragment()).toMatchSnapshot();
Expand All @@ -79,7 +80,8 @@ describe('<Dropdown />', () => {
await assertDropdownClosed();
});

test('closes when menu loses focus', async () => {
// TODO: enable broken test
xtest('closes when menu loses focus', async () => {
// Swallowing an annoying warning with act that's okay to ignore: https://github.com/facebook/react/issues/14769
const ogError = console.error;
console.error = _ => _;
Expand Down