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
26 changes: 12 additions & 14 deletions src/Dropdown/Dropdown.js
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,13 +74,14 @@ export default function Dropdown({
useEffect(() => {
if (isOpen) {
setTimeout(() => {
menuRef.current.focus();
if (menuRef.current) menuRef.current.focus();
});
}
}, [isOpen]);

useKeyPress('Escape', () => {
if (isOpen) {
if (triggerRef.current) triggerRef.current.focus();
close();
}
});
Expand All @@ -95,16 +99,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

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 +111,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 +153,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
3 changes: 1 addition & 2 deletions src/Dropdown/Dropdown.spec.js
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 @@ -88,9 +89,7 @@ describe('<Dropdown />', () => {

// Some issues with fireEvent.focus: https://github.com/kentcdodds/react-testing-library/issues/276#issuecomment-473392827
renderUtils.wrapper.focus();
renderUtils.getByTestId('dropdown-menu').blur();

await waitForDomChange();
await assertDropdownClosed();

console.error = ogError;
Expand Down