Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sc: remove context-menu-layer for autofill context-menu #10784

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

sc: remove context-menu-layer for autofill context-menu #10784

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 13 additions & 1 deletion browser/src/control/Control.ContextMenu.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,7 @@ L.Control.ContextMenu = L.Control.extend({

onAdd: function (map) {
this._prevMousePos = null;
this._autoFillContextMenu = false;

map._contextMenu = this;
map.on('locontextmenu', this._onContextMenu, this);
Expand All @@ -131,6 +132,17 @@ L.Control.ContextMenu = L.Control.extend({

_onMouseUp: function (e) {
this._currMousePos = { x: e.originalEvent.pageX, y: e.originalEvent.pageY };
if (this._autoFillContextMenu) {
this._autoFillContextMenu = false;

var latlng = e.latlng;
var mousePos = this._map._docLayer._latLngToTwips(latlng);

// generate a left mouse click so that a single click can select a cell
// after closing the autofill context menu
this._map._docLayer._postMouseEvent('buttondown', mousePos.x, mousePos.y, 1, 1, 0);
this._map._docLayer._postMouseEvent('buttonup', mousePos.x, mousePos.y, 1, 1, 0);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, isn't the autofill menu generated in online?
or do we pass it from core? (I don't remember)

Copy link
Member Author

@bayramcicek bayramcicek Jan 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

autofill contextmenu is generated in online. (only items in the context menu send UNO command to the core.)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but does it take any data from core to create that popup?
if NOT use core at all to invoke popup -> this approach is possibly wrong -> why we would need to do additional click in core while popup was online only? or I missed something?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but does it take any data from core to create that popup? if NOT use core at all to invoke popup -> this approach is possibly wrong -> why we would need to do additional click in core while popup was online only? or I missed something?

yes, that's right. Is there any way to do additional click on the online? I thought _postMouseEvent is on the online.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then - adding a click feels like workaround. It would be good to be able to answer: why we need additional click?

Do we have present .jsdialog-overlay on the screen maybe? Or the map/canvas is not in focus?
Do you have some logs from the browser - what happens just after the first click?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then - adding a click feels like workaround.

  • yes. we can say that it is a workaround.

It would be good to be able to answer: why we need additional click?

  • after opening the autofill contexmenu there is a id="context-menu-layer" that blocks scroll and clicking on the cells.
  • if we click on a cell -to select it-, it does not click the cell but removes the layer. So if we add an additional click at this point, then we can click on the cell we are on - after that layer was removed - without need to double click.

Do we have present .jsdialog-overlay on the screen maybe?

  • we have id="context-menu-layer"

Screenshot_20250103_163658

Or the map/canvas is not in focus?
gets focus after contextmenu is hidden.

Control.ContextMenu.js:219

					hide: function() {
						if(autoFillContextMenu)
							app.map._docLayer._resetReferencesMarks();
						map.focus();
					}

Do you have some logs from the browser - what happens just after the first click?

hide: function() ^^^ executes and id="context-menu-layer" is removed. then auto contextmenu is closed.

Copy link
Contributor

@eszkadev eszkadev Jan 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. what about something like:

diff --git a/browser/src/control/Control.ContextMenu.js b/browser/src/control/Control.ContextMenu.js
index aecb8a6fe7..c7a13d264c 100644
--- a/browser/src/control/Control.ContextMenu.js
+++ b/browser/src/control/Control.ContextMenu.js
@@ -204,9 +204,22 @@ L.Control.ContextMenu = L.Control.extend({
                                                var $menu = opt.$menu;
                                                $menu.attr('tabindex', 0); // Make the context menu focusable
                                        },
+                                       activated: function (opt) {
+                                               console.error('activated');
+                                               console.error(opt);
+
+                                               var $layer = opt.$layer;
+                                               $layer.css('pointer-events', 'none'); // layer doesn't get mouse clicks
+
+                                               $layer.on('mousedown', function () {
+                                                       console.error('This click handler will not be fired');
+                                               });
+                                       },
                                        hide: function() {
-                                               if(autoFillContextMenu)
+                                               if(autoFillContextMenu) {
                                                        app.map._docLayer._resetReferencesMarks();
+                                                       console.error('but this will be still fired thanks to entry in context menu');
+                                               }
                                                map.focus();

activated event is called when context menu is built and ready to use.
We can just remove the layer on show but.. perfect would be to just not handle clicks to avoid errors inside jQuery.

you can read also internals of jQuery: browser/node_modules/jquery-contextmenu/src/jquery.contextMenu.js for interesting details

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. what about something like:

...

activated event is called when context menu is built and ready to use. We can just remove the layer on show but.. perfect would be to just not handle clicks to avoid errors inside jQuery.

Yes. This is better than removing the context-menu-layer on show(). However there are some issues like: when we scroll down or open the e.g. color palette, the autofill context menu is still there:

Peek.2025-01-06.14-04.mp4

I will try to detect if user clicks on the class="leaflet-layer" - without removing the context-menu-layer.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. Yes, please look at these jQuery events, maybe you will find something interesting to use.
I think we are missing "close all other popups" on opening the JSDialog popups ... which might also solve above problem. (User should have only one popup visible at the time).

We have helper: JSDialog.OpenDropdown to open dropdowns in JSDialog where it could be done. Worth trying too.

}
},

_onKeyDown: function(e) {
Expand Down Expand Up @@ -168,7 +180,7 @@ L.Control.ContextMenu = L.Control.extend({
} else if (menuItem.indexOf('.uno:AutoFill') !== -1) {
// we should close the autofill preview popup before open autofill context menu
map.fire('closeautofillpreviewpopup');
autoFillContextMenu = true;
this._autoFillContextMenu = autoFillContextMenu = true;
break;
}
}
Expand Down
Loading