Fix mouse click event leak on context menu close

[phd]

When closing the F8 popup menu by clicking somewhere on the TigerVNC viewer's window a mouse click event is sent to the server.
No other toolkit nor application behaves like that when closing a popup menu - the closing click is ignored as an input.
And so it should be ignored here, especially that this input is being sent to a remote machine.

This commit fixes it by relying on FLTK's event loop which lets us know the lifetime of our popup menu and therefore easily filter out the closing mouse click event.

[CendioOssman]

This sounds like a FLTK issue, so it should be ideally be fixed there. Have you reported it to them?

That said, a minimal workaround is acceptable if it causes practical practical problems for you.

[CendioOssman]

This is very non-obvious, so a comment would be nice, explaining that this is a workaround and for what

[CendioOssman]

Why the timer? Isn't the mouse press handled by the time popup() returns?

[phd]

No. But when the contextMenu->popup(); returns, the mouse events that caused the popup to close are already queued (and they fire Viewport::handle(int)). So we queue this instant timer event after them and ignore the mouse events until the timeout event happens.
All subsequent mouse events will be queued after our timer, so they will be handled normally.

[phd]

This sounds like a FLTK issue, so it should be ideally be fixed there. Have you reported it to them?

It seems like it's NOT a FLTK bug per se.
FLTK's menubar.cxx test binary works fine. Its Fl_Menu_Button is showing a popup menu and clicking it away does not trigger any events in the underlying window.
Which is strange that TigerVNC's Viewport::handle(int event) is called. Investigating...

Tested with FLTK 1.3.11, which is the old legacy version, and it works fine even there.
My system's FLTK version is at 1.3.8, not sure if it makes a difference.

That said, a minimal workaround is acceptable if it causes practical practical problems for you.

Not that I use this popup menu a lot. But when I do and decide to click it away it causes unwanted input to be sent to the server potentially messing with the running application.

[phd]

Which is strange that TigerVNC's Viewport::handle(int event) is called. Investigating...

I have modified the above menubar.cxx test with my own Fl_Box:

class My_Box : public Fl_Box {
public:
  using Fl_Box::Fl_Box;
  int handle(int event) {
    printf("My_Box::event(%d)\n", event);
    return Fl_Box::handle(event);
  }
};

  My_Box b(200,200,200,100,"Press right button\nfor a pop-up menu");

Fl_Widget::handle(int) is indeed called a lot of times when the widget happens to be under the mouse when clicking away a popup menu:

My_Box::event(3)    // FL_ENTER
My_Box::event(11)   // FL_MOVE
My_Box::event(11)   // FL_MOVE
My_Box::event(11)   // FL_MOVE
My_Box::event(0)    // FL_NO_EVENT
My_Box::event(2)    // FL_RELEASE
My_Box::event(11)   // FL_MOVE

And in TigerVNC's viewer it's the FL_RELEASE event that is being sent to the server.

FLTK must handle this situation internally for its widgets not to act on these events.

So we would either need to filter it out somehow which might be tricky (FL_RELEASE with no earlier FL_PUSH after FL_ENTER?), or simply use this workaround.

[CendioOssman]

Ah, I see what's going on. Unfortunately, it's pretty fundamental to how VNC and vncviewer work. So it will be difficult to work around in a nice way.

The problem is that VNC isn't concerned with FL_MOVE vs FL_PUSH vs FL_RELEASE. It only cares about the current state of the mouse. So we never look at what type of event we got, only what buttons are pressed.

The menu closes when a mouse button is pressed. This is consistent with other toolkits. But after that press, we'll start getting move events. Again, I'd say this is consistent with other toolkits. The issue is that for those move events, we'll correctly deduce that a mouse button is pressed. So we'll send that to the server. Which will result in the applications seeing a mouse press.

[phd]

Subsequent move events (and ButtonPress events they may cause) are not an issue here.
The orphaned Release Event is. It happens even when the user does not move the mouse, just clicks away the menu with a quick ButtonPress and ButtonRelease.

Clicking away any popup menu (Qt, GTK, FLTK) with mouse pointer over a locally running xev window shows this:

EnterNotify event, serial 40, synthetic NO, window 0x5200001,
    root 0x637, subw 0x0, time 691586, (141,73), root:(141,630),
    mode NotifyUngrab, detail NotifyNonlinear, same_screen YES,
    focus NO, state 256

KeymapNotify event, serial 40, synthetic NO, window 0x0,
    keys:  1   0   0   0   0   0   0   0   0   0   0   0   0   0   0   0   
           0   0   0   0   0   0   0   0   0   0   0   0   0   0   0   0   

ButtonRelease event, serial 40, synthetic NO, window 0x5200001,
    root 0x637, subw 0x0, time 691597, (141,73), root:(141,630),
    state 0x100, button 1, same_screen YES

This never generates click events in the underlying windows, so they do not perform any unwanted actions.

But when you click away the TigerVNC's context menu over a xev window running on the remote server, it outputs:

MotionNotify event, serial 38, synthetic NO, window 0x1800001,
    root 0x47c, subw 0x0, time 893923, (36,125), root:(870,155),
    state 0x0, is_hint 0, same_screen YES

ButtonPress event, serial 38, synthetic NO, window 0x1800001,
    root 0x47c, subw 0x0, time 893923, (36,125), root:(870,155),
    state 0x0, button 1, same_screen YES

ButtonRelease event, serial 38, synthetic NO, window 0x1800001,
    root 0x47c, subw 0x0, time 893976, (36,125), root:(870,155),
    state 0x100, button 1, same_screen YES

Indeed we have a problem with ButtonPress Event generated by the VNC.

Clicking away TigerVNC's context menu causes remote applications to perform actions related to this unwanted click event.

This patch suppresses the orphaned Release Event, which solves this problem.
Now remotely running xev shows no events at all for a simple mouse click, which is what we want.

And if you click away the context menu by pressing-and-holding the left mouse button over the remote xev and then drag the mouse, it will not suppress the subsequent move events and the remotely running xev will (correctly) show:

[...]

ButtonPress event, serial 38, synthetic NO, window 0x1800001,
    root 0x47c, subw 0x0, time 1288942, (73,109), root:(907,139),
    state 0x0, button 1, same_screen YES

MotionNotify event, serial 38, synthetic NO, window 0x1800001,
    root 0x47c, subw 0x0, time 1288963, (74,106), root:(908,136),
    state 0x100, is_hint 0, same_screen YES

MotionNotify event, serial 38, synthetic NO, window 0x1800001,
    root 0x47c, subw 0x0, time 1288981, (78,103), root:(912,133),
    state 0x100, is_hint 0, same_screen YES

MotionNotify event, serial 38, synthetic NO, window 0x1800001,
    root 0x47c, subw 0x0, time 1288998, (84,100), root:(918,130),
    state 0x100, is_hint 0, same_screen YES

ButtonRelease event, serial 38, synthetic NO, window 0x1800001,
    root 0x47c, subw 0x0, time 1289859, (116,81), root:(950,111),
    state 0x100, button 1, same_screen YES

[phd]

Updated:

• moved the check to Viewport::handle(int event)
• added vlog.debug() for the suppressed events

You are right, they are all indeed move events (reported from FLTK), 11 == FL_MOVE:

Fri Mar 21 23:00:00 2025
 Viewport:    Ignored pointer event 11 because of opened menu.
 Viewport:    Ignored pointer event 11 because of opened menu.
 Viewport:    Ignored pointer event 11 because of opened menu.
 Viewport:    Ignored pointer event 11 because of opened menu.

But the mouse pointer didn't move during the click!

Are these move events which occurred when the mouse pointer left the context menu and was moving over the main window, before the click?

[phd]

Updated to allow (move) events that report no buttons pressed in buttonMask.
This properly updates the mouse position when closing the context menu with an ESC key.

It seems there is some kind of race condition in FLTK when a popup menu is being closed, because these move events have buttons pressed randomly reported for them.
Or maybe reading Fl::event_button1/2/3() for such events queued while closing a popup menu does not work reliably?

Fri Mar 21 23:17:39 2025
 Viewport:    Ignored mouse event 11,1 while closing menu.
 Viewport:    Ignored mouse event 11,1 while closing menu.
 Viewport:    Ignored mouse event 11,1 while closing menu.
 Viewport:    Ignored mouse event 11,1 while closing menu.

Fri Mar 21 23:17:40 2025
 Viewport:    Allowed mouse event 11,0 while closing menu.
 Viewport:    Allowed mouse event 11,0 while closing menu.
 Viewport:    Allowed mouse event 11,0 while closing menu.
 Viewport:    Allowed mouse event 11,0 while closing menu.

11,1 = event,buttonMask

[phd]

I have added one more temporary debug() to log every mouse event occurring in those situations:

if (menuOpened) {
  if (buttonMask != 0) {
    vlog.debug("Ignored mouse event %d,(%d,%d),%d while closing the menu.", event, Fl::event_x(), Fl::event_y(), buttonMask);
    return 1;
  }
  vlog.debug("Allowed mouse event %d,(%d,%d),%d while closing the menu.", event, Fl::event_x(), Fl::event_y(), buttonMask);
} else {
  vlog.debug("Allowed mouse event %d,(%d,%d),%d.", event, Fl::event_x(), Fl::event_y(), buttonMask);
}

Clicking away the menu, button pressed randomly reported:

Fri Mar 21 23:57:33 2025
 Viewport:    Ignored mouse event 11,(980,525),1 while closing the menu.
 Viewport:    Ignored mouse event 11,(980,525),1 while closing the menu.
 Viewport:    Ignored mouse event 11,(980,525),1 while closing the menu.
 Viewport:    Ignored mouse event 11,(980,525),1 while closing the menu.
 Viewport:    Allowed mouse event 2,(980,525),0.
 Viewport:    Allowed mouse event 11,(980,525),0.

Clicking away the menu, no buttons randomly reported:

Fri Mar 21 23:57:34 2025
 Viewport:    Allowed mouse event 11,(941,556),0 while closing the menu.
 Viewport:    Allowed mouse event 11,(941,556),0 while closing the menu.
 Viewport:    Allowed mouse event 11,(941,556),0 while closing the menu.
 Viewport:    Allowed mouse event 11,(941,556),0 while closing the menu.

Clicking away the menu with press & hold & drag & release:

Fri Mar 21 23:58:11 2025
 Viewport:    Ignored mouse event 11,(952,473),1 while closing the menu.
 Viewport:    Ignored mouse event 11,(952,473),1 while closing the menu.
 Viewport:    Ignored mouse event 11,(952,473),1 while closing the menu.
 Viewport:    Ignored mouse event 11,(952,473),1 while closing the menu.
 Viewport:    Allowed mouse event 11,(953,473),1.
 Viewport:    Allowed mouse event 11,(954,473),1.
 Viewport:    Allowed mouse event 11,(955,473),1.
[...]
 Viewport:    Allowed mouse event 11,(980,486),1.
 Viewport:    Allowed mouse event 2,(980,486),0.
 Viewport:    Allowed mouse event 11,(980,486),0.

Clicking away the menu with press & hold & release:

Fri Mar 21 23:58:55 2025
 Viewport:    Ignored mouse event 11,(920,503),1 while closing the menu.
 Viewport:    Ignored mouse event 11,(920,503),1 while closing the menu.
 Viewport:    Ignored mouse event 11,(920,503),1 while closing the menu.
 Viewport:    Ignored mouse event 11,(920,503),1 while closing the menu.

Fri Mar 21 23:58:56 2025
 Viewport:    Allowed mouse event 2,(920,503),0.
 Viewport:    Allowed mouse event 11,(920,503),0.

Closing the menu with an ESC key:

Fri Mar 21 23:59:38 2025
 Viewport:    Allowed mouse event 11,(1213,468),0 while closing the menu.
 Viewport:    Allowed mouse event 11,(1213,468),0 while closing the menu.
 Viewport:    Allowed mouse event 11,(1213,468),0 while closing the menu.
 Viewport:    Allowed mouse event 11,(1213,468),0 while closing the menu.

These 4 phantom move events at constant pointer position are consistent.

And it seems the patch properly filters out the random reports for the button still being pressed when the menu is being closed.

[CendioOssman]

And if you click away the context menu by pressing-and-holding the left mouse button over the remote xev and then drag the mouse, it will not suppress the subsequent move events and the remotely running xev will (correctly) show:

But as shown in your output, remote applications will see a click, not just movement. Which means that the workaround suggested in this PR is very fragile and will often fail.

Are these move events which occurred when the mouse pointer left the context menu and was moving over the main window, before the click?

The pointer is grabbed by the menu. I suspect we get a fake movement event when the grab is released.

[phd]

@CendioOssman
What the Viewport::handle(int event) function sees are actually Move Events with (or without) mouse button pressed.
So my comment you are quoting was a bit wrong about the Release Events.

Please check my later comments:
#1931 (comment)
#1931 (review)
#1931 (comment)

The problem is with 4 phantom Move Events (FL_MOVE == 11) which should probably never occur in FLTK in the first place.
They are consistent and (selectively) skipping them until the next event loop tick seems like a very reliable way of fixing this.
Every real mouse button press or release by the user will be delivered in one of subsequent event loop passes.

[CendioOssman]

The problem is not just any stray events we get just as the menu closes. We can get more events after that.

This is broken for me:

1. Open menu
2. Press mouse button outside of menu
3. Move mouse slightly
4. Release mouse button

At step 3, I get a mouse click, even with your changes.

Are you seeing the same thing?

[phd]

This is broken for me:
1. Open menu
2. Press mouse button outside of menu
3. Move mouse slightly
4. Release mouse button
At step 3, I get a mouse click, even with your changes.
Are you seeing the same thing?

I'm also getting a mouse click here, but at step 4.
This is consistent with my test case "press & hold & drag & release" here: #1931 (comment)
These events come after the menu is already closed (next event loop iteration fired the 0ms timer and set menuOpened=false).

xev:

(2. mouse press)

(3. mouse move)

ButtonPress event, serial 38, synthetic NO, window 0x1600001,
    root 0x47c, subw 0x0, time 8571492, (116,115), root:(121,145),
    state 0x0, button 1, same_screen YES

MotionNotify event, serial 38, synthetic NO, window 0x1600001,
    root 0x47c, subw 0x0, time 8571509, (115,113), root:(120,143),
    state 0x100, is_hint 0, same_screen YES

MotionNotify event, serial 38, synthetic NO, window 0x1600001,
    root 0x47c, subw 0x0, time 8571529, (114,113), root:(119,143),
    state 0x100, is_hint 0, same_screen YES

(4. mouse release)

ButtonRelease event, serial 38, synthetic NO, window 0x1600001,
    root 0x47c, subw 0x0, time 8572006, (114,113), root:(119,143),
    state 0x100, button 1, same_screen YES

https://phd.github.io/js-mouse-events.html (events order from bottom to top):

[2025-04-28 11:35:38.757] click
[2025-04-28 11:35:38.757] mouseup
(4. mouse release)
[2025-04-28 11:35:34.298] mousemove x10
[2025-04-28 11:35:34.269] mousedown
[2025-04-28 11:35:34.267] mousemove
(3. mouse move)
(2. mouse press)

This behavior is different from what all other toolkits do - they eat also the mouse release event after clicking away the a popup menu.
On the other hand this allows drag&drop right after clicking away the menu.
And with good mouse handling (no mouse move between press and release) still allows user to click away the menu without any interaction.

[CendioOssman]

I think this will give a rather unpredictable user experience. If the mouse is sensitive enough, or if the user is not still enough, they'll still get clicks, but now more randomly.

This needs to be fixed more fully for it to be merged.