docs: make the asynchronous create demo work on right click - #806
Merged
Conversation
The demo opened its menu from a `mouseup` handler, which is not the event you get when the user right clicks. Chromium keeps its own context menu open over the page and never delivers the `mouseup`, so only the native menu showed up. Firefox delivers it, but only after the native menu is already on screen. Bind `contextmenu` instead and call `preventDefault()` on it so the browser menu stays away. The items are now handed to a callback from a `setTimeout`, so the demo really is asynchronous rather than building the items inline and merely deferring the open. Because `$.fn.contextMenu()` opens the menu by triggering a `contextmenu` event on the trigger, the handler runs again. A flag on the trigger makes that re-entrant call a no-op, and it doubles as a guard against starting a second request while one is still in flight. The spec file was renamed from `aync-create.js` to `async-create.js` and extended to cover the right click flow: the menu only appears after the delay, the native menu is prevented, the menu also opens from a `contextmenu` event that is never followed by a `mouseup`, the re-entrant event settles instead of recursing, and a second right click still works.
Asserting the menu is still hidden right after the right click passes on the first poll, but it would fail on a machine slow enough to let the 1s delay elapse first. Measure the time from the click to the menu appearing and only bound it from below instead.
Member
Author
|
👍 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #735
The problem
documentation/demo/async-create.mdsays "right click me", but it opened the menu from amouseuphandler. That is not the event you get when a user right clicks:mouseup, so only the native menu appears and the plugin menu never opens.The demo was also asynchronous in name only.
createSomeMenu()built the items inline and thesetTimeoutmerely deferred the call to$this.contextMenu(position), so it did not actually demonstrate items that take a while to arrive.The fix
Demo only, no plugin source changes.
contextmenuon the trigger andpreventDefault()it, so the browser's own menu stays away while we wait.fetchSomeMenu(done)helper whosesetTimeoutstands in for a server round trip.build()picks the result up off the trigger, which is the pattern the reporter described.$.fn.contextMenu()opens the menu by triggering acontextmenuevent on the element, so the handler runs a second time. AnasyncMenuBusyflag on the trigger makes that call a no-op, and it doubles as a guard against firing a second request while one is still in flight. It is cleared right after the open, so the next right click works normally.Tests
test/specs/aync-create.jswas renamed totest/specs/async-create.js(the old filename had a typo) and extended from one test to five:defaultPreventedon the realcontextmenuevent)contextmenuevent that is never followed by amouseup, which is the Chromium situation from the issuecontextmenuinvocations and one menu, so the re-entrant event settles instead of recursingTests 2 and 3 fail against the demo as it is on
masterand pass with this change. Verified green on jQuery 1.12.4, 2.2.4, 3.7.1 and 4.0.0.Note on the plugin source
No source change seemed warranted, the correct pattern is expressible today. One rough edge worth recording though:
$.fn.contextMenu({x: undefined, y: undefined})does not match thex/ybranch, falls through to$.isPlainObject()and ends up in$.contextMenu('create', ...), which throwsNo selector specified. That is a confusing failure mode for what looks like a position argument, but it is out of scope here.