Use click event to determine modifier keys

[smoores-dev]

Previously, editor elements had to listen for mouse_up events to determine when a click had completed. This meant that they only had access to modifier keys that were pressed during the mouse_up event.

This led to some incorrect user experiences, such as executing a ctrl+click if the user pressed ctrl after pressing the mouse button, but before releasing it.

This change adds a click event handler to EditorElement, and adds a modifier() method to the ClickEvent, which only includes the modifier keys that were pressed during both mouse down and mouse up. The code for handling link clicks has been moved into the click event handler, so that it's only triggered when the non-multi-cursor modifier was held for both the mouse down and mouse up events.

Closes #12752, #16074, #17892 (the latter two seem to be duplicates of the former!)

Release Notes:

• Fixed a bug where pressing ctrl/cmd (or other modifiers) after mouse down but before mouse up still triggered ctrl/cmd+click behavior (e.g. "go to definition")

[smoores-dev]

To reviewers:

• Does this make sense as a strategy? It seemed like the most straightforward thing, to me: We were missing proper "click" semantics, so I've added a new (essentially, synthetic) click event at the platform level so that consumers don't have to add additional state management
• I am not super familiar with this codebase — it's very possible that I did something silly or missed something obvious! Apologies in advance
• I've only implemented the new event in the X11 and Wayland platforms because... well I couldn't figure out exactly how Windows or macOS were wired up? If someone can point me in the right direction, I'm happy to add those changes as well!

[mikayla-maki]

It seems like the issue is that the existing click event forces you to pick either the down or up events to work with, and so there's bugs in existing code due to modifiers present in one or the other, is that correct?

If so, I think a simpler solution would be to create a method on ClickEvent, maybe just modifiers() and click_count(), that computes this combined event that you are synthesizing here, and then fix the bugs upstream by calling this method. I don't think we want to hide the underlying nature of click events as being composed of two MouseEvents, as done in this PR.

[smoores-dev]

It's not only that — the ClickEvent before this change was only ever emitted, manually, by the div element. So no click events were emitted at all if a user clicks on text in the editor, for example.

But that makes sense, I'm definitely not opposed to keeping the up and down properties on the ClickEvent and just adding modifiers() and click_count()! I think we would then need to track the entire previous MouseDownEvent (rather than just its modifiers) in the client state, which seems fine to me.

The approach here is modeled after web browser's click event semantics, which doesn't contain any references to the associated mouse down or up events!

[mikayla-maki]

It's not only that — the ClickEvent before this change was only ever emitted, manually, by the div element. So no click events were emitted at all if a user clicks on text in the editor, for example.

True, it is a tradeoff of this approach. I think generally we prefer to push decisions like this up to client APIs though, and so you do have to re-implement this stuff more often.

The approach here is modeled after web browser's click event semantics, which doesn't contain any references to the associated mouse down or up events!

That's a sensible approach we use most of the time as well, and I think that's the model we want the div APIs to follow! But at the GPUI Element level, we are fundamentally trying to build a thinner, more DIY kind of framework than what the web offers :)

[smoores-dev]

That's a sensible approach we use most of the time as well, and I think that's the model we want the div APIs to follow! But at the GPUI Element level, we are fundamentally trying to build a thinner, more DIY kind of framework than what the web offers :)

Ah, ok! This is very helpful to know, thank you. So then it sounds like the right approach would look more like:

• Keep the GPUI clients as they are: only dispatch events that correspond to actual hardware events, like MouseDown and MouseUp
• Add a "synthetic" modifiers() helper method to ClickEvent.
• Potentially update existing uses of click_event.up.modifiers and click_event.down.modifiers to use the new click_event.modifiers()? (I would guess "yes", as this kind of thing is what's causing the issue here in the first place)
• Add some state to track the most recent MouseDownEvent and a ClickEvent dispatcher to editor/src/element.rs, so that we can fix the titular issue

Does that seem right?

[mikayla-maki]

That does seem correct!

[smoores-dev]

Ok, awesome,. Thanks for the guidance @mikayla-maki! I think I've implemented this as described now. I've confirmed that this approach also fixes the issue (at least for me, on Linux, with Wayland)

[smoores-dev]

Howdy! This is one of those things that I'm reminded of every day because I constantly trigger it haha. Just bumping this in case it was forgotten, totally understand that you all may have other priorities and that code review is work!

[smoores-dev]

Oh, funny.

error: unused variable: `position_map`
   --> crates/editor/src/element.rs:669:9
    |
669 |         position_map: &PositionMap,
    |         ^^^^^^^^^^^^ help: if this is intentional, prefix it with an underscore: `_position_map`
    |
    = note: `-D unused-variables` implied by `-D warnings`
    = help: to override `-D warnings` add `#[allow(unused_variables)]`

This is only failing on Windows and macOS because that position_map argument is only used inside a #[cfg(any(target_os = "linux", target_os = "freebsd"))] macro. I added an allow(unused_variables) with the inverse condition — let me know if that wasn't the right move!

[smoores-dev]

I see a lot of green checkmarks! :D @mikayla-maki does this look good to you?

[maxdeviant]

I see a lot of green checkmarks! :D @mikayla-maki does this look good to you?

@smoores-dev Please don’t spam the PR thread for reviews. We will get to this when we have bandwidth.

[mikayla-maki]

Hi @smoores-dev,

I've been heads down working on #22632, and am able to review PRs again!Unfortunately, it looks like that PR has caused several conflicts with this one.

This implementation is looking really good though, I like having a proper click handler in the editor for all of that multicursor logic. Once the conflicts are resolved I'd be happy to merge this :D

[smoores-dev]

Yay, thanks @mikayla-maki! Just rebased onto main and pushed!