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.

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

Lack of consistency in how the ContextMenu reacts on clicking the menu items #6507

Closed
budnix opened this issue Nov 29, 2019 · 6 comments
Closed

Comments

@budnix
Copy link
Member

@budnix budnix commented Nov 29, 2019

Description

This issue #6304 changed behavior on how the context menu items are executed. Since 7.3.0 (and beta 8.0.0) the ContextMenu triggers action on mouseup event. Playing with that changes a little bit I've found that RMB click hasn't changed. This leads to some no consistences where LMB executes on mouseup and RMB on mousedown.

I've compared the behavior with system context menu on Windows OS and all mouse button triggers action on mouseup event.

Steps to reproduce

  1. Go to https://jsfiddle.net/9c7kp14n/;
  2. Trigger context menu and click some menu item;
  3. For comparing click LMB without releasing the button then the same with RMB;

Demo

https://jsfiddle.net/9c7kp14n/

Your environment

  • Handsontable version: 7.3.0
  • Browser Name and version: Chrome
  • Operating System: macOS
@swistach

This comment has been minimized.

Copy link
Contributor

@swistach swistach commented Dec 3, 2019

I guess afterOnCellContextMenu https://github.com/handsontable/handsontable/blob/develop/src/plugins/contextMenu/menu.js#L189 should only call event.preventDefault() and let beforeOnCellMouseUp execute a command.

@budnix budnix self-assigned this Dec 6, 2019
budnix added a commit that referenced this issue Dec 9, 2019
The Walkontable had a bug where `onCellMouseUp` hook was fired for all
mouse buttons except RMB. It was not consistent with `onCellMouseDown`
hook, which fired always.

The behavior of the `onCellDblClick` and `onCellCornerDblClick` is
changed. From now on, the doubleclick is fired only when it's triggered
by LMB. Previously the hooks are triggered on any button except RMB.

Issue: #6507
budnix added a commit that referenced this issue Dec 23, 2019
The Walkontable had a bug where `onCellMouseUp` hook was fired for all
mouse buttons except RMB. It was not consistent with `onCellMouseDown`
hook, which fired always.

The behavior of the `onCellDblClick` and `onCellCornerDblClick` is
changed. From now on, the doubleclick is fired only when it's triggered
by LMB. Previously the hooks are triggered on any button except RMB.

Issue: #6507
jansiegel added a commit that referenced this issue Feb 3, 2020
The Walkontable had a bug where `onCellMouseUp` hook was fired for all
mouse buttons except RMB. It was not consistent with `onCellMouseDown`
hook, which fired always.

The behavior of the `onCellDblClick` and `onCellCornerDblClick` is
changed. From now on, the doubleclick is fired only when it's triggered
by LMB. Previously the hooks are triggered on any button except RMB.

Issue: #6507
@aninde

This comment has been minimized.

Copy link

@aninde aninde commented Feb 4, 2020

If I understood the reproduction correctly, holding or clicking RMB on context menu triggered immediately selected option from context menu.

So our faulty, actual result from 7.3.0 was:
Green circle = RMB
Black circle = LMB

cm-broken-7 3

7.4.0 currently tested version that issue is fixed
https://jsfiddle.net/aninde/su0kyo7j/
cm-fixed-ff-7 4

@AMBudnik

This comment has been minimized.

Copy link
Contributor

@AMBudnik AMBudnik commented Feb 5, 2020

While testing on Windows 10 (Chrome 79)

I got a disappearing menu on LMB and the same for RMB
old

Now, in 7.4.0

The LMB closes the menu but RMB executes the command and keeps the menu opened
new

But this is tricky when we wan tot continues doing those operations on the selected cell as the menu is shifted to the cursor's position (around 9th row). Example below

GIF

EDIT: Quick update. I just had a chance to check this on Mac and there the menu is closed if we click RMB

budnix added a commit that referenced this issue Feb 11, 2020
On Windows machine the "contextmenu" event is fired after the "mouseup"
event. This causes a bug where the context menu was not closed after
clicking the menu item by RMB. I've added a new helpers which detects on
what OS the code runs and add proper conditional to support the
differences between Windows vs Linux and MacOS.

Issue: #6507
budnix added a commit that referenced this issue Feb 11, 2020
Issue: #6507
budnix added a commit that referenced this issue Feb 11, 2020
Issue: #6507
budnix added a commit that referenced this issue Feb 11, 2020
On Windows machine the "contextmenu" event is fired after the "mouseup"
event. This causes a bug where the context menu was not closed after
clicking the menu item by RMB. I've added a new helpers which detects on
what OS the code runs and add proper conditional to support the
differences between Windows vs Linux and MacOS.

Issue: #6507
budnix added a commit that referenced this issue Feb 11, 2020
On Windows machine the "contextmenu" event is fired after the "mouseup"
event. This causes a bug where the context menu was not closed after
clicking the menu item by RMB. I've added a new helpers which detects on
what OS the code runs and add proper conditional to support the
differences between Windows vs Linux and MacOS.

Issue: #6507
@aninde

This comment has been minimized.

Copy link

@aninde aninde commented Feb 12, 2020

This bug no longer occurs on Windows on updated 7.4.0 (build 11.02.20). https://codesandbox.io/s/cold-dew-9kztj
Tested on Windows 10/ Edge 44, Edge Dev 81, Firefox 73, Chrome 79.
It is fixed by new build of release 7.4.0 (11.02.20).
This bug does not occur on Ubuntu / Firefox 68.On Ubuntu contextMenu works like on macOs.

@AMBudnik

This comment has been minimized.

Copy link
Contributor

@AMBudnik AMBudnik commented Feb 12, 2020

Thank you for doing final fixes. The issue fixed officially in 7.4.0

@aninde

This comment has been minimized.

Copy link

@aninde aninde commented Feb 18, 2020

Still working with 7.4.1 https://codesandbox.io/s/mystifying-mccarthy-z1md4 on Windows 10 / Edge 44 and Chrome 79.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.