ENA-136 Fix context menu closing on the same right-click which opened it

[towerofnix]

Resolves

Fixes #2085.

Proposed Changes

See updated: #2834 (comment). Original below:

Closure Library already handles the difficult work (keeping track of coordinates). This PR changes handleContextMenu so that it accesses the same code paths as normal mouse-ups (where the fix is located in Closure) and provides a supporting override which makes right-clicks "count" as the browser action button in this context. It also provides the mouse event which was responsible for creating the menu directly to the menu, which is required for the Closure Library fix to work.

Reason for Changes

See updated: #2834 (comment). Original below:

It's preferable to make use of a fix which was already implemented as part of the library we use, rather than to re-create a similar fix from scratch. The fix in Closure Library includes an automated unit test and was developed professionally (according to Google's standards for Closure Library).

Thus, this pull request opts to ensure scratch-blocks accesses the code paths Closure Library expects to be working with.

Initially I thought it would be enough to simply set call menu.setVisible(true, true, e), ensuring Closure has access to the event which spawned the context menu (which is all it needs to perform its own fix). Unfortunately in practice, this wasn't enough, as the scratch-blocks code responsible for handling context menus wasn't even reaching the normal code paths in the first place—it was just directly emitting the ACTION event, bypassing all particular behavior Closure Library provides. (I feel it's fair to describe this as a "hack", as although it's effective, we probably shouldn't be deliberately skipping any code our UI framework specifically includes to make behavior more nuanced.)

So I changed the existing handleContextMenu override to instead treat the context menu click as a normal mouse-up event. This almost worked, but the code which decides whether or not to perform an action specifically checks if the event was an "action" button, i.e. a left-click (and without ctrl pressed on Mac). I overrode this function (on the single involved mouse event only) so that all left and right clicks would count as "action" buttons (middle click is still ignored though!).

See a full technical summary here if wanted: #2085 (comment)

Test Coverage

First I created a reliable repro case which works in tests/vertical_playground.html to make testing easier:

• Create a lot of variables. You can import from this XML to make this step quicker.
• Drag out a variable block and position it near the far right edge of the viewport. Vertically it can be basically anywhere (as long as your display isn't super tall)—within the upper half of the display (~1000px) is reliable in this repro.
• Right click the variable.
• The variable should be immediately replaced with a variable of another name with the context menu never made visible.

In the end I tested this manually:

• Old intended behavior is retained.
  • Left clicking activates the menu option.
  • Right clicking activates the menu option.
  • Middle clicking does not activate the menu option.
• This fixes the bug!
  • Above repro case does not close the menu and perform an action immediately.
  • Menu appears and then has no further behavior until the next click.

We also get a free actual unit test right from Closure Library!

[towerofnix]

Technically this line ("force") could be false since the menu is visible by default anyway, and so its value doesn't change. It would avoid some slight overhead of re-applying certain DOM values, and would be less misleading without any SHOW/AFTER_SHOW side effects—but it necessitates familiarity/assumption that the menu starts out visible. (Obviously we can take this for granted, or else menu rendering as-is wouldn't work in the first place.)

[towerofnix]

NB: All pull requests are failing atm, it doesn't indicate any issues with this pull request afaik.

[GarboMuffin]

Update from the future: Someone reported that this bug still happens in TurboWarp so my patch is probably incomplete -- I am missing something.

I think the only part of this that's necessary is

diff --git a/core/contextmenu.js b/core/contextmenu.js
index 65401747..13b03f4e 100644
--- a/core/contextmenu.js
+++ b/core/contextmenu.js
@@ -103,9 +103,14 @@ Blockly.ContextMenu.populate_ = function(options, rtl) {
     if (option.enabled) {
       goog.events.listen(
           menuItem, goog.ui.Component.EventType.ACTION, option.callback);
-      menuItem.handleContextMenu = function(/* e */) {
-        // Right-clicking on menu option should count as a click.
-        goog.events.dispatchEvent(this, goog.ui.Component.EventType.ACTION);
+      menuItem.handleMouseDown = function(e) {
+        // Menu options only respond to left clicking (browser action button).
+        // Override the event to let right clicking work too.
+        e.isMouseActionButton = function() {
+          return this.isButton(goog.events.BrowserEvent.MouseButton.LEFT) ||
+            this.isButton(goog.events.BrowserEvent.MouseButton.RIGHT);
+        };
+        goog.ui.MenuItem.base(this, 'handleMouseDown', e);
       };
     }
   }

If I remove the added setVisible call, I still can't reproduce the bug. Since the problem was (to my understanding) that the browser sends a mousedown event (which shows the menu) then a contextmenu event (which forcibly selects the hovered item), removing the contextmenu listener would fix it since the contextmenu event is now ignored. Am I missing something?

When I try to compile this locally for production I get

Using local compiler: google-closure-compiler ...

core/contextmenu.js:113: ERROR - incorrect use of goog.ui.MenuItem.base: Must be used within goog.ui.MenuItem methods
        goog.ui.MenuItem.base(this, 'handleMouseDown', e);
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

1 error(s), 0 warning(s)
UNKNOWN ERROR

which may be related to checks failing

(in TurboWarp I am fixing this bug in a different way with a really sketchy hack: TurboWarp@ad65dda)

[benjiwheeler]

It sounds like maybe this still needs work, and additionally needs some extensive QA testing? Our devices lean heavily towards Mac/iOS/Chromebook/Android and we're pretty lacking in Windows and especially Linux...

[benjiwheeler]

(And, thanks for all of the work on this bug!)

[towerofnix]

Thanks for the review @benjiwheeler, and thanks for the input @GarboMuffin - I've been extra busy with work lately so haven't gotten around to reviewing your feedback!

If I remove the added setVisible call, I still can't reproduce the bug. Since the problem was (to my understanding) that the browser sends a mousedown event (which shows the menu) then a contextmenu event (which forcibly selects the hovered item), removing the contextmenu listener would fix it since the contextmenu event is now ignored. Am I missing something?

This is a simpler fix than what I have, yeah. If that's the case, I think we can avoid the error you encountered (which - you're right - is also what is erroring this build) just by using the existing goog.events.dispatchEvent call, rather than the full-featured handleMouseDown call introduced in this PR.

I'll try that locally and see if it works (and update back here), then probably test across multiple devices (I have full access to Mac, Linux, and Windows devices).

[towerofnix]

If that's the case, I think we can avoid the error you encountered (which - you're right - is also what is erroring this build) just by using the existing goog.events.dispatchEvent call, rather than the full-featured handleMouseDown call introduced in this PR.

Update: This won't work because dispatchEvent is just a backhanded way of calling handleMouseDown, oops. I think we'll need a slightly more (or maybe much less) clever fix, but it shouldn't be too bad.

[towerofnix]

Alright, I've pushed an alternate fix which makes most of everything in the original PR description out of date. Please see the updated description below.

Proposed Changes

Remove existing code which tacks on a unique function handleContextMenu to each created goog.ui.MenuItem (during Blockly.ContextMenu.populate_) and implements identical behavior in a new and formally defined subclass of goog.ui.MenuItem, dubbed Blockly.ContextMenuItem.

Rather than defining handleContextMenu on the new subclass, we opt to add a few lines in handleMouseDown which override (the event).isMouseActionButton to also accept right-clicking as a valid "mouse action button".

Reason for Changes

The object of both the original code (current in LLK/scratch-blocks) and the new code (this PR) is to allow right-clicking menu items to also activate the menu item, just like left-clicking. However, the original code is "hacky" in two separate ways, both of which the new code works around.

1. Adding a handleContextMenu handler (which subsequently called goog.ui.dispatchEvent(this, EventType.ACTION) sprung the bug ENA-136 rightclicking on a block in the editor can close the context menu in the same click #2085, because handleContextMenu was unexpectedly being called during the same click which spawned the menu in the first place (under particular rare, but consistent and bothersome, circumstances). We avoid this by instead overriding handleMouseDown more directly, teaching Closure Library (goog.ui.Control) that right clicks count as a "mouse action button" too.

2. The original code directly added behavior to each and every goog.ui.MenuItem created for a context menu (during populate_), which was convenient in that it avoided extra boilerplate and kept the public goog.provides surface more minimal, as compared to creating and providing a whole new subclass... but this brought up a number of issues:

   • The added behavior is fundamentally limited because it's "tacked on" instead of being defined as a proper subclass prototype method: it can't access goog.ui.MenuItem.base because, even though it's "part of" the (overridden) goog.ui.MenuItem code, Closure Compiler doesn't allow access this way. (This posed a problem when attempting the more "minimal" update to this PR.)
   • Performance-wise, it's not ideal to override the prototype-defined behavior on each of potentially many items created all at once every time a context menu is opened. (The new code also involves overriding a function, but this is during handleMouseDown and it only overrides one function, so it's much less costly.)
   • It's just not good code style to "tack on" behavior instead of using a subclass. By newly providing Blockly.ContextMenuItem, we enable the possibility to further add or modify menu item functionality without having to cram everything into the populate_ function (and potentially run into the issues described above).

---

Test coverage is same as before (the same repro case described should be used). I'll manually test this across OS and browser combinations to help with QA!

[towerofnix]

QA OS/Browser Testing

Unfortunately, I couldn't find a way to reliably reproduce the bug #2085 outside of the vertical scratch-blocks playground. Where marked, I used this playground for QA; otherwise, all QA is in a local build of scratch-gui with this version of scratch-blocks linked.

Test Description	Mac Catalina 10.15.17	Linux Ubuntu 22.04.1 LTS	Windows Win10 Home 19042.1586
Opening a menu and choosing an option with the left mouse button works. Drag out "move 10 steps" into the code area. Right- or control/command-click to show the context menu. Use the left mouse button and click "Duplicate". The block duplicates as expected.	Firefox 91.12.0esr ✔️ Chrome 104.0.5112.101 ✔️ Safari 15.6 ✔️	Firefox 103.0.2 ✔️ Chromium 104.0.5112.79 ✔️	Firefox 99.0 ✔️ Chrome 100.0.4896.75 ✔️ Edge 100.0.1185.36 ✔️
Opening a menu and choosing an option with the right mouse button works. Same repro as above but with the right mouse button. The block duplicates as expected.	Firefox ✔️ Chrome ✔️ Safari ✔️	Firefox ✔️ Chromium ✔️	Firefox ✔️ Chrome ✔️ Edge ✔️
Opening a menu and choosing an option with the middle mouse button does nothing. Drag out "move 10 steps" into the code area. Right- or control/command-click to show the context menu. Use the middle mouse button and click "Duplicate". Nothing happens: the menu doesn't disappear and the block isn't duplicated.	Firefox ✔️ [a] Chrome ✔️ Safari ✔️	Firefox ✔️ Chromium ✔️	My Windows laptop doesn't have a middle click option, sorry!
Opening a menu does NOT instantly hide it in the particular circumstances where it did before. Use the scratch-blocks vertical playground instead of scratch-gui. Import from XML this workspace with a lot of defined variables. Scroll the workspace so that the imported variable block is aligned along the right edge of the screen. Right- or control/command-click the variable to show the context menu. The context menu shows, instead of disappearing immediately (and replacing the variable with another variable).	Firefox ✔️ Chrome ✔️ Safari ✔️	Firefox ✔️ Chromium ✔️	Firefox ✔️ Chrome ✔️ Edge ✔️
[a] Firefox 91 ESR has some kinda inconsistent behavior when clicking with the mousewheel, partially depending on whether "autoscroll" is enabled in the preferences. This seems to be a browser bug, probably nothing to do with Scratch - especially because I couldn't repro the inconsistencies in more recent Firefox.