fix: autoHide never applied to a nested trigger with a different trigger mode - #808
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a58f812f29
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // menu. See https://github.com/swisnl/jQuery-contextMenu/issues/727 | ||
| if (target && (triggerAction || activatesOtherTrigger(target, button))) { | ||
| var showTarget = function () { | ||
| $(target).contextMenu({x: x, y: y, button: button}); |
There was a problem hiding this comment.
Avoid bubbling a contextmenu for nested left clicks
When a right-click menu is open and the user left-clicks a nested left-trigger, showTarget calls $.fn.contextMenu, whose coordinate overload triggers a bubbling synthetic contextmenu event. Any application contextmenu listener on an ancestor of the nested trigger therefore fires for a plain left click before the plugin's delegated document handler can stop it. Dispatch the matched menu directly, as handle.click does, rather than reintroducing the synthetic event leakage that direct dispatch was designed to avoid.
Useful? React with 👍 / 👎.
|
Thanks, that one is fair and now addressed in 8b66cc2. Addressed: the P2 about bubbling a synthetic
The new assertion in Not changed: the Unit tests, acceptance tests against jQuery 1.12.4 / 2.2.4 / 3.7.1 / 4.0.0 and eslint are green locally. |
8b66cc2 to
a97f804
Compare
|
Rebased onto The rebase had one real conflict. Both PRs restructure the same deferred re-open block in
Taking either side wholesale drops the other's fix, so the two conditions are now combined: wait for Verified together, not just separately: |
|
👍 |
…ger mode Registering two menus on nested elements with different trigger modes, e.g. `trigger: 'left'` on a button inside a row registered with `trigger: 'right'`, left the inner menu unusable once the outer one had been opened, and autoHide never kicked in for it. Three things went wrong, all in the hand-off between the menu that is closing and the one that should open: - handle.layerClick() decided whether the click that dismissed the open menu should also open another one from the *closing* menu's own `trigger` option. A left-click while a right-click menu was open therefore never re-dispatched, so the nested trigger's menu simply never showed. The button is now matched against the triggers that are actually registered for the element under the cursor, and the menu asked to show still validates the button against its own `trigger` option as before. - Resolving what the click landed on required `root.$layer` to still be there 50ms later. autoHide's own timer routinely closes the menu inside that window, which left the lingering layer element itself reported as the click target. - op.hide() unbound the document-level keyboard and autoHide handlers unconditionally, so a menu that had just been shown while the previous one was still tearing down lost its handlers again and never auto hid. The same applied to `$currentTrigger`. Both are now only cleared when they still belong to the menu being hidden. Closes #727
…bling event Re-opening through $.fn.contextMenu's coordinate overload triggers a bubbling synthetic 'contextmenu' event, which an application listener on an ancestor of the trigger cannot tell apart from a real right-click. On a plain left-click on a nested left-trigger that is exactly the leak #754 removed from handle.click. The trigger lookup now returns the matched element and its options, so the menu can be dispatched straight to handle.contextmenu the way handle.click does. It resolves the innermost match, mirroring which delegated handler a bubbling event would have reached first, and skips selector-string registrations whose context does not contain the element. Covered by an added assertion that a jQuery 'contextmenu' listener on an ancestor stays silent while the nested menu opens on left-click.
a97f804 to
e6d0377
Compare
Problem
Registering two menus on nested elements with different trigger modes, the setup from #727, a
trigger: 'left'button inside a row registered withtrigger: 'right', both withautoHide: true, leaves the inner menu unusable once the outer one has been opened, and autoHide never kicks in for it.Root cause
Three things go wrong, all in the hand-off between the menu that is closing and the one that should open.
handle.layerClick()decided whether the click that dismissed the open menu should also open another one from the closing menu's owntriggeroption (triggerAction). A left-click while a right-click menu is open therefore never re-dispatched, so the nested trigger's menu simply never showed. The button is now also matched against the triggers actually registered for the element under the cursor. The menu that gets asked to show still validates the button against its owntriggeroption inhandle.contextmenu(), so nothing new can slip through.root.$layerto still be present 50ms later. autoHide's own timer routinely closes the menu inside that window, and the layer element lingers in the document for another few milliseconds afterroot.$layeris gone, soelementFromPoint()reported the layer itself as the click target.op.hide()unbound the document-level keyboard and autoHide handlers unconditionally. A menu shown while the previous one was still tearing down lost its handlers again immediately and never auto hid. The same applied to$currentTrigger. Both are now only cleared when they still belong to the menu being hidden.Tests
New demo page
documentation/demo/nested-triggers-autohide.mdwith the reporter's configuration, plustest/specs/nested-triggers-autohide.jscovering:The last two fail on
masterand pass with this change. The single-trigger cases pass either way and are kept as regression cover. Both flows also assert that no#context-menu-layeris left behind, so no orphaned menu stays on screen.Unit tests, acceptance tests against jQuery 1.12.4 / 2.2.4 / 3.7.1 / 4.0.0, eslint and
npm run docs:buildare all green.Closes #727