Skip to content
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

Improper default events for useClickAway #1087

Open
tkrotoff opened this issue Mar 30, 2020 · 1 comment
Open

Improper default events for useClickAway #1087

tkrotoff opened this issue Mar 30, 2020 · 1 comment

Comments

@tkrotoff
Copy link

tkrotoff commented Mar 30, 2020

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
Copy link
Author

tkrotoff commented Mar 30, 2020

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();
});

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant