Add utility to handle profusion of clicks

[colin-grant-work]

What it does

This is my proposal for a way to handle the problem of double clicks triggering single-click handlers (twice). It's a bit of a sketch. A fuller implementation might:
[ ] - Hook the delay up to a preference so that the user could configure it. (This would be fairly straightforward)
[ ] - Abstract over more event types than click and double click, if there are others that suffer similar problems (maybe key chords?).

Other thoughts have been expressed and proposals made in #9546, #8735, #8168. This approach has at least these advantages:

• It's generalizable - anywhere anyone faces this problem (React or regular DOM), they could use this utility
• Its configurable - you have some options about whether and when to trigger the single-click handler relative to user action and the double-click handler.

One drawback, noted in the code, is that the TreeWidget, where most of the complaints have focused, can't use this code to invoke the single-click handler immediately and then double-click handler once, because invoking the single-click handler triggers a full rerender of the tree, attaching a new listener to the DOM node and so obviating the state stored in the closure around the handler. I've added (in my second commit) an option to call the single-click handler and then the double-click handler, if that's desirable.

How to test

1. Open a tree widget of your choice.
2. Interact with nodes via a single click
3. After ca. 250 milliseconds, you should get what you expect from a single click.
4. Interact with nodes via double click faster than 250 ms
5. You should get what you expect from a double click, but not what you expect from a single click
6. Triple clicking in under 250ms should give you double-click + single click.

Review checklist

• as an author, I have thoroughly tested my changes and carefully followed the review guidelines

Reminder for reviewers

• as a reviewer, I agree to behave in accordance with the review guidelines

Signed-off-by: Colin Grant [email protected]

[colin-grant-work]

@svor, this is an alternative solution to attempting to run a task multiple times (as well as other misinterpretations of clicks). What do you think about this kind of approach?

[svor]

@svor, this is an alternative solution to attempting to run a task multiple times (as well as other misinterpretations of clicks). What do you think about this kind of approach?

@colin-grant-work it looks good to me, thanks!

@tsmaeder WDYT? It's related to eclipse-che/che#19821

[colin-grant-work]

I'd appreciate any thoughts or feedback folks have on this idea. We've had quite a number of issues related to click handling, and this seems like a reasonable approach that we can apply anywhere we need it.

[tsmaeder]

Just a couple of general thoughts on first approach:

• Not sure about the semantics we want on single vs. double click: in the end, it's up to clients on how to handle this. We should give users of TreeWidget the tools to implement common semantics with the option to do their own thing if desired
• Why are we even using the react "double click" event if we're using our own timeout? The point of handling the native double click is to use the default timeout, etc, no?
• If we're handling clicks ourselves, why not just use a single click handler and use the event.detail field to distinguish click count? This way, we wouldn't have to keep state.
• A single click is a click that is not part of a rapid sequence of clicks: it's neither preceded nor followed by other clicks (within the timeout)
• What about three clicks in sequence? Are they a double followed by a single click? Sounds wrong somehow.

Maybe the interface for clients should be this:

onClick(handler)
onClicks(numberOfClicks, handler)

adding a double click handler would be

onClicks(2, handler)

When handlers for multiple click cardinalities are registered for mouse clicks, the handler with the lowest cardinality (onClick being deemed "0") can decide whether to pass the event to the rest of the handlers.

[tsmaeder]

Or maybe the correct semantics are like this:

• a single click is exactly one click preceded and followed by a pause
• a double click is exactly two clicks preceded and followed by a pause
• ...

That way, you handlers are exclusive: when you triple click, the single and double click handlers are not invoked. There is always the "immediate" handler that is invoked on any mouse clicked event, regardless of sequence. Since anything < 0.5 secs or so is perceived as immediate, I think we could handle the "execute on single click" scenario that ways, as well.

[colin-grant-work]

Not sure about the semantics we want on single vs. double click: in the end, it's up to clients on how to handle this. We should give users of TreeWidget the tools to implement common semantics with the option to do their own thing if desired

Since users of the TreeWidget can extend the class and do whatever they want, I think this condition is satisfied by any approach we adopt. The only question we have to answer is what we want the default behavior to be.

Why are we even using the react "double click" event if we're using our own timeout? The point of handling the native double click is to use the default timeout, etc, no?

It's true, we don't have refer to the double-click event at all if we don't want to. It would be nice if we could query the user's OS for what it believes the double-click timeout to be (and if anyone knows how to do it cleanly, please chime in!), but in the absence of that, we can just look for clicks within whatever interval we specify. That change wouldn't change the logic here much, though. As you say, we can just use event.detail. Those are only questions of implementation, rather than approach.

On the semantic questions:

• What about three clicks in sequence? Are they a double followed by a single click? Sounds wrong somehow.
• a single click is exactly one click preceded and followed by a pause; a double click is exactly two clicks preceded and followed by a pause

This becomes a question of how far we want to depart from the browser's semantics and our current setup. My approach here is to depart as little as possible while addressing the bugs that have been raised by our users / adopters. The particular problem that's arisen is running a single-click handler (with undesired side effects) when users were targeting a double-click, and that can be solved by just not running those handlers. If you triple-click and invoke the double-click and then the single-click, we can at least blame your finger :-). But if we consider that an important use case, we can also accommodate it.

[tsmaeder]

I've experimented quickly with my ideas from the comments above and came up with this method (never mind clean typing, etc.)

    protected createNodeAttributes(node: TreeNode, props: NodeProps): React.Attributes & React.HTMLAttributes<HTMLElement> {
        const className = this.createNodeClassNames(node, props).join(' ');
        const style = this.createNodeStyle(node, props);

        return {
            className,
            style,
            onClick: event => {
                this.handleClickEvent(node, event);
                // eslint-disable-next-line @typescript-eslint/no-explicit-any
                const myEvent: any = event as any;
                if (event.detail === 1) {
                    if (myEvent.__timeout) {
                        // the native multi-click timout is shorter than ours: it's sending us two single clicks
                        console.info(`clearing timeout ${myEvent.__timeout}`);
                        clearTimeout(myEvent.__timeout);
                        myEvent.__timeout = undefined;
                        this.handleDblClickEvent(node, event);
                    } else {
                        myEvent.__timeout = setTimeout(() => {
                            this.handleSingleClickEvent(node, event);
                        }, 250);
                        console.info(`setting timeout ${myEvent.__timeout}`);
                    }
                } else if (event.detail === 2) {
                    console.info(`clearing timeout ${myEvent.__timeout}`);
                    clearTimeout(myEvent.__timeout);
                    myEvent.__timeout = undefined;
                    this.handleDblClickEvent(node, event);
                }
            },
            onContextMenu: event => this.handleContextMenuEvent(node, event)
        };
    }

I think it's a bit more straightforward and it handles the case where native double-click timeout is actually shorter than what we have. One of the main ideas, though is that "click" and "single click" are really not the same thing, so there is "handleClick" which is called immediately and "handleSingleClick" and "handleDoubleClick" which are mutually exclusive.

[tsmaeder]

I've read somewhere that this is a no-op in newer React versions. From my testing, I don't think we need it. (https://reactjs.org/docs/events.html).

[colin-grant-work]

Looks like they say that that's the case as of React 17, but we're still on 16.14.0 according to our yarn.lock. Good to know, though, and something to look forward to :-).

[tsmaeder]

Not sure about the use of promises here. Wouldn't a simple setTimout(doStuff(), timeout) be simpler? We're not doing anything with the results of the computation anyway.

[colin-grant-work]

I had hoped that writing the code this way would get around restrictions on the browser's definition of 'user action' (this was one of Anton's original concerns), but it looks like it doesn't achieve that. It looks like there may not really be any way to delay the handling of an event and still count as a user action (without, say, blocking the thread with an infinite loop :-) ), but if you can see any way, I'm open to suggestions.

[tsmaeder]

It avoids running user-triggered code in a timeout handler. There is a timeout, but it's wrapped in a Promise.

@colin-grant-work not sure why this is an advantage.

[colin-grant-work]

I think it's a bit more straightforward and it handles the case where native double-click timeout is actually shorter than what we have. One of the main ideas, though is that "click" and "single click" are really not the same thing, so there is "handleClick" which is called immediately and "handleSingleClick" and "handleDoubleClick" which are mutually exclusive.

This looks interesting to me. I especially like the idea of having an explicitly unconditional handler, along with the single- and double-click handlers. I have two comments:

• You mention handling the case where the native timeout is shorter than ours. That's pretty straightforward - your way works, as does listening for the double click event. The tricky case is if the native timing is longer than we expect. If the user expects to be able to wait 2 seconds between clicks and still get the double click effect, then I think your code and mine have the same problem: we'll execute the double-click handler, but we'll have executed the single-click handler already. I'm not sure how to get around this without being able to query the native double click timeout, other than by letting the user set a preference.
• It looks like your code relies on being able to set a field on the event (__timeout), and then see that same field on the event triggered by a different click. If that works, I think it will only work in React code, where the synthetic event gets reused. In the native DOM, it's a different event every time. And even in React, if the user calls event.persist() because they want to run some asynchronous code with it, React will be forced to use a new event instance for any future clicks, so any information stored on the event itself will be lost. Given that, I think it's better to hold onto the first event somehow rather than relying on the persistence of the event instance.

[tsmaeder]

It looks like your code relies on being able to set a field on the event (__timeout), and then see that same field on the event triggered by a different click.

Indeed it does. What bothers me in your code is that you're retaining the superseded event in a closure, but since the tree item is re-rendered on the first click, the closure should be a new one and the event variable undefined. Your code seems to work, though, and I just don't understand why.

An alternative would be to save the state in the tree node: as I understand it it is the stable object that is rendered to html.

[colin-grant-work]

Indeed it does. What bothers me in your code is that you're retaining the superseded event in a closure, but since the tree item is re-rendered on the first click, the closure should be a new one and the event variable undefined. Your code seems to work, though, and I just don't understand why.

Somewhere in the code I have I comment that points out that React code can't use the invokeImmediately option for that reason - and couldn't use your unconditionally invoked handler, either. As long as nothing happens between the single and double clicks that causes a rerender, my code works for React, but you're right that if anything does, there's a problem. There are two alternatives: storing the first event (or something about it) on a field in the tree (#8735 has a timeout and a preventClick field on the tree to handle it), or use a click-handler that persists across rerenders. Those make it a little tricky to make sure you're handling rapid clicks on separate elements correctly. I think #8735 handles things correctly, but I haven't checked all scenarios. On the other hand, #8735 can't be generalized to other contexts we might want to use it in. I'm happy to experiment with non-closure persistence to see if something can be found that will deliver all of our desiderata.

[tsmaeder]

Since the tree widget is widely used in many different timing scenarios, I think it's worth getting this right. The number is attempts of fixing this problem is the best indicator.

[tsmaeder]

Those make it a little tricky to make sure you're handling rapid clicks on separate elements correctly.

So two rapid clicks on different elements are a double click or two singles? Stop hurting, my poor head! ;-)

[tsmaeder]

@colin-grant-work any news on this one?

[colin-grant-work]

@tsmaeder, no, alas - other things have been keeping me busy. I can likely put some work into it today, though.

[colin-grant-work]

@tsmaeder, I've rewritten my code to use a class rather than a closure. Your suggestion re: event.detail was very useful, because that keeps track of click target identity so we don't have to. If we see a .detail number lower than the last we saw, either the OS timeout has expired or the user has started clicking somewhere new.

The new implementation also supports immediate invocation and an arbitrary number of handlers, if people want to set up exclusive quintuple click handlers.

One potentially controversial decision is that if the user continues clicking madly on a single element, clicks outside the bounds of the defined handlers are basically ignored. Alternatively we could modulo the .detail and run back through the handlers as the user clicks, maybe always invoking the last one in the list when the user hits that point? I don't really think this is an issue that's come up, though, so we're likely fine ignoring the odd triple-click.

[msujew]

@colin-grant-work The code looks good to me, and it seems like this is a clean solution for the exclusivity problem of (multi-)clicks. However, I am a bit hesitant to fully support the current proposal, mainly because some trees seem quite unresponsive with the default timeout of 500ms in place, especially since a lot of them have no issues handling the first click of a double click as a single click (e.g. the navigator).

The immediateAction handler seems to go in the right direction, although I would like to see an addition to it: Being able to handle the first n clicks independently of the handler. What I think is that being able to handle the preview functionality of the navigator after the first click and opening it fully after the second click becomes a bit convoluted with immediateAction, as that would trigger 3 events (opening the preview after the first click, opening the file fully after the second click and trying to open it again after the second click due to the immediateAction). Instead I would like to see the something like a immediateCount?: number property that would be passed to the handler by using the TreeProps (perhaps in a special tree related property like handleSingleClickImmediately?: boolean).

Does that make sense to improve the responsiveness?

[colin-grant-work]

@msujew, re: the click timeout length, I agree the 500ms timeout is too high - I would expect most users to want something lower, so we can adjust that, if we'd like. I chose it just because it happens to be the default double-click timeout for Windows.

re: the immediateCount idea, that sounds interesting. Would it look something like this:

• Pass {immediateCount: 2} to the handler
• The handler then invokes the single and double-click handlers immediately.

I think I do like that better than having one action that you do on every click - I can't think of a lot of contexts in which that makes sense.

But I think the whole discussion has maybe gotten a bit too complex. If we assume that we really only need to worry about single vs. double click, which I think is a fair assumption, then it boils down to making the single-click responsive enough - we can always invoke the double-click handler immediately. That means we really have two options: invoke single-click immediately, or wait to see if there will be a second click. If we wait, then the only question is how long to wait.

In the context of this implementation, I can certainly add an immediateCount option instead of or in addition to the current immediate handler to allow people to invoke the single-click handler immediately. For those trees that need a delay, we can shorten the default, but I don't really see a way around just waiting for whatever period is specified.

[tsmaeder]

I somehow missed the updated code here. I'll have a look as soon as I can (today is busy).

[JonasHelming]

@tsmaeder : Ping!

[colin-grant-work]

There doesn't seem to be an excess of interest in this suggestion - I'm going to close for now, but it can be revived if there are additional issues with single- and double-click handling.