Summary: | Web Inspector: we should show artificial context menus on mousedown instead of click | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Devin Rousso <hi> | ||||||||
Component: | Web Inspector | Assignee: | Devin Rousso <hi> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | commit-queue, ews-watchlist, hi, inspector-bugzilla-changes, joepeck, rniwa, webkit-bug-importer | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | All | ||||||||||
OS: | All | ||||||||||
Attachments: |
|
Description
Devin Rousso
2019-03-08 16:38:37 PST
Created attachment 364091 [details]
Patch
Comment on attachment 364091 [details] Patch Attachment 364091 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/11436168 New failing tests: accessibility/mac/selection-notification-focus-change.html Created attachment 364117 [details]
Archive of layout-test-results from ews107 for mac-highsierra-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107 Port: mac-highsierra-wk2 Platform: Mac OS X 10.13.6
Comment on attachment 364091 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=364091&action=review > Source/WebInspectorUI/ChangeLog:4 > + Web Inspector: we should show artificial context menus on mousedown instead of click > + https://bugs.webkit.org/show_bug.cgi?id=195494 Awesome! It is not clear from the description that this allows you to mousedown => mouseover context menu => mouseup to quick select a contextmenu item. But it does, and that is awesome. > Source/WebInspectorUI/ChangeLog:14 > + fired when/before a "contextmenu" event is fired, each of the below callers has to maintain What does it mean they are fired "when/before a contextmenu event is fired"? > Source/WebInspectorUI/ChangeLog:41 > + * UserInterface/Controllers/CanvasManager.js: > + (WI.CanvasManager.supportsRecordingAutoCapture): > + Drive-by: fix usage of InspectorBackend.domains.{CanvasAgent => Canvas} These drive-bys should really be separate patches, they are so far removed from this. > Source/WebInspectorUI/UserInterface/Views/CanvasContentView.js:444 > + this._ignoreViewShaderButtonMouseDown = true; Should this move down after the early returns? Otherwise it seems if we hit one of the early returns we would never recover and this mousedown would always bail. > Source/WebInspectorUI/UserInterface/Views/CanvasContentView.js:475 > + this._ignoreViewRecordingButtonMouseDown = true; Same regarding early returns. > Source/WebInspectorUI/UserInterface/Views/ContextMenu.js:227 > + for (let callback of this._beforeShowCallbacks) This would fail if this._beforeShowCallbacks is null. This happens in the Tab Bar, if you have many tabs, make a narrow window, and click the ">>" chevron to switch between hidden tabs. (1) It would be simpler to just construct it with an array and then `addBeforeShowCallback` can be simplified and this would avoid a possible exception. (2) We should update that menu as well! TabBar and LegacyTabBar have calls to ContextMenu.prototype.show Basically it seems might want to do this for any call to ContextMenu.prototype.show in mousedown. Should we add an assert in show along the lines of: console.assert(event.type !== "mousedown" || this._beforeShowCallbacks.length === 1, "ContextMenu.show() in mousedown should have a show callback handler to enable quick selection."); Comment on attachment 364091 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=364091&action=review >> Source/WebInspectorUI/ChangeLog:14 >> + fired when/before a "contextmenu" event is fired, each of the below callers has to maintain > > What does it mean they are fired "when/before a contextmenu event is fired"? If you ran the following on <about:blank>, you'd see that "mousedown" always fires before "contextmenu", even when right/control clicking. ``` document.addEventListener("mousedown", (event) => console.log("mousedown", event)); document.addEventListener("mouseup", (event) => console.log("mouseup", event)); document.addEventListener("contextmenu", (event) => console.log("contextmenu", event)); document.addEventListener("click", (event) => console.log("click", event)); ``` FWIW: "mouseup" and "click" never fire when right/control clicking. >> Source/WebInspectorUI/ChangeLog:41 >> + Drive-by: fix usage of InspectorBackend.domains.{CanvasAgent => Canvas} > > These drive-bys should really be separate patches, they are so far removed from this. I fixed this in a separate patch and hadn't gotten around to rebasing this. My apologies. >> Source/WebInspectorUI/UserInterface/Views/CanvasContentView.js:444 >> + this._ignoreViewShaderButtonMouseDown = true; > > Should this move down after the early returns? Otherwise it seems if we hit one of the early returns we would never recover and this mousedown would always bail. Nice catch! Created attachment 364639 [details]
Patch
Comment on attachment 364639 [details] Patch Clearing flags on attachment: 364639 Committed r242937: <https://trac.webkit.org/changeset/242937> All reviewed patches have been landed. Closing bug. |