fix(@toss/react): Fix the useOutsideClickEffect callback function to be called only once

[sa02045]

Overview

FIX #354

Because of the limitations on the touch event test in Testing library, no way to test was found.
Let me know if there's any other way

PR Checklist

• I read and included theses actions below

1. I have read the Contributing Guide
2. I have written documents and tests, if needed.

[netlify]

👷 Deploy request for slash-libraries pending review.

Visit the deploys page to approve it

Name	Link
🔨 Latest commit	dda3bcb

[okinawaa]

Thank you for contributing.

I think you solved it well.

Since there is a problem with the touch device, can't we solve problem either device is touch device or not ?

IsMobileWeb is only branching according to iOS, android, not processing whether it's a touch device or not.

I think you can go over things like navigator.maxTouchPoints

[minsoo-web]

Can't we listen to both touch and mouse events?
Since mouse events don't run on touch devices, adding listeners won't cause any performance inconvenience.

[sa02045]

Can't we listen to both touch and mouse events? Since mouse events don't run on touch devices, adding listeners won't cause any performance inconvenience.

Thank you for your response.
In fact, user touches the screen both touch and mouse events will occur (So the core of this PR is that there is a bug where the callback of useOutsideClickEffect runs twice when you touch it.)
reference

Even when I experimented with my iPad, mouse events such as �mousedown occurred when I touched it

https://codesandbox.io/s/trusting-fog-6vgygn?file=/src/App.js

20231106_200311.mp4

[sa02045]

Thank you for contributing.

I think you solved it well.

Since there is a problem with the touch device, can't we solve problem either device is touch device or not ?

IsMobileWeb is only branching according to iOS, android, not processing whether it's a touch device or not.

I think you can go over things like navigator.maxTouchPoints

Yes. As you said, branching out depending on whether it's a touch device is one of the solutions.

But the problem with this approach is that there are devices like touchable laptops.

Desktop browsers support touch as well as mobile browsers. https://caniuse.com/?search=touch

The summary is as follows.

Device	Trigger Events(When touch or click)	Event to need to listen to
📱 Phone(Pad)	touch, click	✅ touch ❌ click (Now the problem is here!)
💻 touchable laptop	touch, click	✅ touch, ✅ click
💻 Laptop	click	✅ click ⚠️ touch (doesn't matter this listen to a touch event because this can't touch it)

What if the code changes as follows?

document.addEventListener('touchstart', handleDocumentClick);
if (!isMobileWeb()) {
  document.addEventListener('click', handleDocumentClick);
}

[ssi02014]

@sa02045
It's a interesting topic, and I'm keeping track of it!! 👍
Off topic, I think defining an eventType like this would reduce repetitive code

useEffect(() => {
  const eventType = isMobileWeb() ? 'touchstart' : 'click';

  document.addEventListener(eventType, handleDocumentClick);
  return () => {
    document.removeEventListener(eventType, handleDocumentClick);
  };
});

[hoseungme]

The main problem we should solve is that the callback could be called twice, not which event must be triggered.

So we just make useOutsideClickEffect only listen click event to solve the problem. Click event is triggered regardless of whether users interacted with screen by mouse or touch or something.

useEffect(() => {
  document.addEventListener('click', handleDocumentClick);

  return () => {
    document.removeEventListener('click', handleDocumentClick);
  };
});

[sa02045]

The main problem we should solve is that the callback could be called twice, not which event must be triggered.

So we just make useOutsideClickEffect only listen click event to solve the problem. Click event is triggered regardless of whether users interacted with screen by mouse or touch or something.

useEffect(() => {
  document.addEventListener('click', handleDocumentClick);

  return () => {
    document.removeEventListener('click', handleDocumentClick);
  };
});

Yes, listening to click events alone solves the problem easily.

However, listening only to click events can lead to poor usability for mobile users.
This is because there is a subtle difference in the trigger timing of click events and touchstart events.

A click event is triggered when a finger is removed from the touchscreen. However, in a mobile environment, users often swipe the screen or drag the screen without removing their finger.

This creates a "delay" in the action we want to invoke. Personally, I feel this has a lot of negative impact on usability.

Therefore, I think we should consider listening to the touch event as well and fix it.

[hoseungme]

However, listening only to click events can lead to poor usability for mobile users.
This is because there is a subtle difference in the trigger timing of click events and touchstart events.

A click event is triggered when a finger is removed from the touchscreen. However, in a mobile environment, users often swipe the screen or drag the screen without removing their finger.

This creates a "delay" in the action we want to invoke. Personally, I feel this has a lot of negative impact on usability.

But that's exactly why I told you that useOutsideClickEffect should listen only click events.

First of all, what is click? It is an action to press on and release an element with something like pointer, finger, ...

But how about the touchstart event? It is triggered when you just put your fingers on a screen. It is different from the click action and it makes useOutsideClickEffect work differently on non-touch and touch devices.

And you also told me about 'delay' and the swipe and drag gestures. But the gestures are also different from the click action, and they never trigger the click events. So I think it is not appropriate to call them 'delay'.

[hoseungme]

Simply put, I would like to ask you to think more about the name of useOutside"Click"Effect.