Bug 195494 – Web Inspector: we should show artificial context menus on mousedown instead of click

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Devin Rousso]

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`.

[Devin Rousso]

Created attachment 364091 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[Joseph Pecoraro]

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.");

[Devin Rousso]

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!

[Devin Rousso]

Created attachment 364639 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 364639 [details]
Patch

Clearing flags on attachment: 364639

Committed r242937: <https://trac.webkit.org/changeset/242937>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Devin Rousso]

<rdar://problem/48880102>