streamich / react-use

React Hooks — 👍
http://streamich.github.io/react-use
The Unlicense
41.87k stars 3.16k forks source link

Improper default events for useClickAway #1087

Open tkrotoff opened 4 years ago

tkrotoff commented 4 years ago

In #264, iOS support replaced previous 'click' implementation. IMHO the solution is too simplistic. Under all browsers it should be 'click' except iOS where 'click' does not work properly: https://www.quirksmode.org/blog/archives/2014/02/mouse_event_bub.html.

Considering iOS has 15% of market share (https://gs.statcounter.com/os-market-share), iOS way cannot be the default at the expense of all others implementations (85%).

The best implementation for users that I know of is Bootstrap: https://getbootstrap.com/docs/4.4/components/dropdowns/#single-button: the dropdown closes when clicking outside and not when scrolling. Material does the same (buggy on some examples: the menu sticks when scrolling): https://material-ui.com/components/menus/#menulist-composition

(tested with iOS Simulator 10.1: iOS 12.1 > iPhone XR)

Related:

If the proper trick for iOS cannot/shouldn't be performed inside useClickAway(), then it should be documented so users are not surprised.

tkrotoff commented 4 years ago

This is what I do to fix the issue for now:

import React, { useRef, useState } from 'react';
import { useClickAway, useKey } from 'react-use';
import { useClickAwayFixIOS } from './useClickAwayFixIOS';

// ...

const [showDropdown, setShowDropdown] = useState(false);

const dropdownRef = useRef<HTMLDivElement>(null);
// https://github.com/streamich/react-use/issues/1087
useClickAway(dropdownRef, () => setShowDropdown(false), ['click']);
useClickAwayFixIOS();
useKey('Escape', () => setShowDropdown(false));

// ...

<div ref={dropdownRef}>
  <button
    type="button"
    onClick={() => setShowDropdown(!showDropdown)}
    className="btn btn-primary dropdown-toggle"
  />
  <div
    className={classNames('dropdown-menu', {
      show: showDropdown
    })}
  >
    {showDropdown && (
      // ...
    )}
  </div>
</div>
// File useClickAwayFixIOS.ts 
import { useEffect } from 'react';
import { noop } from 'lodash-es';

export function useClickAwayFixIOS() {
  useEffect(() => {
    // https://github.com/twbs/bootstrap/blob/e1f5d819c73ad66e6ec0480e75e5e08c815a633e/js/src/dropdown.js#L187-L195
    if ('ontouchstart' in document.documentElement) {
      Array.from(document.body.children).forEach(e => e.addEventListener('mouseover', noop));
    }

    return () => {
      // https://github.com/twbs/bootstrap/blob/e1f5d819c73ad66e6ec0480e75e5e08c815a633e/js/src/dropdown.js#L414-L419
      if ('ontouchstart' in document.documentElement) {
        Array.from(document.body.children).forEach(e => e.removeEventListener('mouseover', noop));
      }
    };
  }, []);
}

Tested with iOS Simulator 10.1: iOS 12.1 > iPhone XR, Android 10 Chrome 80 + desktop browsers.

Edit:

The unit tests

// File useClickAwayFixIOS.test.tsx
import React from 'react';
import { fireEvent, render } from '@testing-library/react';
import * as _ from 'lodash-es';

import { assert } from './assert';
import { useClickAwayFixIOS } from './useClickAwayFixIOS';

jest.mock('lodash-es', () => ({
  ...jest.requireActual('lodash-es'),
  noop: jest.fn()
}));

function Test() {
  useClickAwayFixIOS();

  return (
    <>
      <div>
        test1
        <div>test11</div>
      </div>
      <div>
        test2
        <div>test21</div>
      </div>
    </>
  );
}

beforeAll(() => {
  // Check jsdom does not define ontouchstart
  assert(document.documentElement.hasOwnProperty('ontouchstart') === false);
});

afterEach(() => {
  // Back to original jsdom implementation
  delete document.documentElement.ontouchstart;
});

afterAll(() => {
  // Check jsdom does not define ontouchstart
  assert(document.documentElement.hasOwnProperty('ontouchstart') === false);
});

test('call noop when mouse over the divs', () => {
  document.documentElement.ontouchstart = () => 'whatever';

  const spy = jest.spyOn(_, 'noop');

  const { container, getByText } = render(<Test />);

  fireEvent.mouseOver(document.body);
  expect(spy).toHaveBeenCalledTimes(0);

  fireEvent.mouseOver(container);
  expect(spy).toHaveBeenCalledTimes(1);

  fireEvent.mouseOver(getByText('test1'));
  expect(spy).toHaveBeenCalledTimes(2);

  fireEvent.mouseOver(getByText('test11'));
  expect(spy).toHaveBeenCalledTimes(3);

  fireEvent.mouseOver(getByText('test2'));
  expect(spy).toHaveBeenCalledTimes(4);

  fireEvent.mouseOver(getByText('test21'));
  expect(spy).toHaveBeenCalledTimes(5);

  spy.mockRestore();
});

test('do nothing if ontouchstart is not defined', () => {
  const spy = jest.spyOn(_, 'noop');

  const { getByText } = render(<Test />);

  fireEvent.mouseOver(getByText('test1'));
  expect(spy).toHaveBeenCalledTimes(0);

  spy.mockRestore();
});

test('do not call noop after useEffect cleanup', () => {
  document.documentElement.ontouchstart = () => 'whatever';

  const spy = jest.spyOn(_, 'noop');

  const { unmount, container } = render(<Test />);

  fireEvent.mouseOver(container);
  expect(spy).toHaveBeenCalledTimes(1);

  unmount();

  fireEvent.mouseOver(container);
  expect(spy).toHaveBeenCalledTimes(1); // noop hasn't be called

  spy.mockRestore();
});