Bug 195494 - Web Inspector: we should show artificial context menus on mousedown instead of click
Summary: Web Inspector: we should show artificial context menus on mousedown instead o...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Devin Rousso
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-03-08 16:38 PST by Devin Rousso
Modified: 2019-03-14 13:14 PDT (History)
7 users (show)

See Also:


Attachments
Patch (18.14 KB, patch)
2019-03-08 17:05 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews107 for mac-highsierra-wk2 (2.72 MB, application/zip)
2019-03-08 22:29 PST, Build Bot
no flags Details
Patch (20.42 KB, patch)
2019-03-14 00:23 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Devin Rousso 2019-03-08 16:38:37 PST
As an example, the "Add Breakpoint" button in the Debugger/Sources sidebar should show the breakpoint list context menu on "mousedown" so that it doesn't require two clicks to show.  This should be the same for all places where we manually call `WI.ContextMenu.prototype.show`.
Comment 1 Devin Rousso 2019-03-08 17:05:52 PST
Created attachment 364091 [details]
Patch
Comment 2 Build Bot 2019-03-08 22:29:11 PST Comment hidden (obsolete)
Comment 3 Build Bot 2019-03-08 22:29:12 PST Comment hidden (obsolete)
Comment 4 Joseph Pecoraro 2019-03-13 23:52:41 PDT
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 5 Devin Rousso 2019-03-14 00:20:06 PDT
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!
Comment 6 Devin Rousso 2019-03-14 00:23:21 PDT
Created attachment 364639 [details]
Patch
Comment 7 WebKit Commit Bot 2019-03-14 01:01:02 PDT
Comment on attachment 364639 [details]
Patch

Clearing flags on attachment: 364639

Committed r242937: <https://trac.webkit.org/changeset/242937>
Comment 8 WebKit Commit Bot 2019-03-14 01:01:04 PDT
All reviewed patches have been landed.  Closing bug.
Comment 9 Devin Rousso 2019-03-14 13:14:51 PDT
<rdar://problem/48880102>