[webkit-reviews] review granted: [Bug 170705] Web Inspector: No context menu available via ENABLE_INSPECTOR_SERVER : [Attachment 306755] Patch

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has granted Ross Kirsling
<ross.kirsling at sony.com>'s request for review:
Bug 170705: Web Inspector: No context menu available via
ENABLE_INSPECTOR_SERVER
https://bugs.webkit.org/show_bug.cgi?id=170705

Attachment 306755: Patch

https://bugs.webkit.org/attachment.cgi?id=306755&action=review




--- Comment #4 from Joseph Pecoraro <joepeck at webkit.org> ---
Comment on attachment 306755
  --> https://bugs.webkit.org/attachment.cgi?id=306755
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=306755&action=review

r=me with comments.

After playing with it I found a few usability issues:

  1. You should not be able to right click the SoftContextMenu and get another
native menu!
    => You can detect clicks and preventDefault if its a right click (but still
allow the original quick selection path)
  2. Keyboard events do not work
    => I have comments below that get this working.

Maybe you can address (1) in a follow-up.

It looks very very close to native!

> Source/WebInspectorUI/UserInterface/Base/InspectorFrontendHostStub.js:-168
> -	   showContextMenu: function(event, menuObject)
> -	   {
> -	   },
> -

You should keep this:

    showContextMenu: function(event, menuObject)
    {
	new WebInspector.SoftContextMenu(items).show(event);
    },

We want the Stub to match InspectorFrontendHost.idl, if someone saw something
missing they might add back a stub and break the conditional you added later.

> Source/WebInspectorUI/UserInterface/Views/SoftContextMenu.js:35
> +
> +    show(event)

Style: Put a divider comment after the constructor and before here:

    // Public

See: https://trac.webkit.org/wiki/WebInspectorCodingStyleGuide

> Source/WebInspectorUI/UserInterface/Views/SoftContextMenu.js:39
> +	   // Create context menu.

Nit: We can drop this comment, the code is clear.

> Source/WebInspectorUI/UserInterface/Views/SoftContextMenu.js:46
> +	   for (let i = 0; i < this._items.length; ++i)
> +	      
this._contextMenuElement.appendChild(this._createMenuItem(this._items[i]));

Style: Use for..of:

    for (let item of this._items)
	this._contextMenuElement.appendChild(this._createMenuItem(item));

> Source/WebInspectorUI/UserInterface/Views/SoftContextMenu.js:60
> +	   this._repositionMenuOnScreen(isSubMenu);

In order to make keyboard events work I had to add a few lines here:

	this._contextMenuElement.tabIndex = -1;
	this._contextMenuElement.focus();

> Source/WebInspectorUI/UserInterface/Views/SoftContextMenu.js:62
> +

Style: Put a divider comment here:

    // Private

> Source/WebInspectorUI/UserInterface/Views/SoftContextMenu.js:86
> +	   if (item.type === "separator")

Ideally we would have an enum WebInspector.ContentMenu.MenuItemType.Separator,
.SubMenu, etc. But since that didn't exist already what you have here is fine.

> Source/WebInspectorUI/UserInterface/Views/SoftContextMenu.js:96
> +	   const checkMarkElement = document.createElement("span");
> +	   // Unicode "CHECK MARK"
> +	   checkMarkElement.textContent = item.checked ? "\u2713" : "";

Style: We would handle this now with const variables to avoid the comments:

    const checkmark = "\u2713";
    const blackRightPointingTriangle = "\u25b6";

    const checkMarkElement = document.createElement("span");
    checkMarkElement.textContent = item.checked ? checkmark : "";
    ...

> Source/WebInspectorUI/UserInterface/Views/SoftContextMenu.js:116
> +	   } else {
> +	       menuItemElement._actionId = item.id;
> +	   }
> +

Style: Drop braces for a single statement block.

> Source/WebInspectorUI/UserInterface/Views/SoftContextMenu.js:306
> +		   this._highlightedMenuItemElement._subMenuTimer =
> +		       setTimeout(this._showSubMenu.bind(this,
this._highlightedMenuItemElement), 150);

Style: Do not wrap this line.

> Source/WebInspectorUI/UserInterface/Views/SoftContextMenu.js:313
> +	   let menuItemElement = this._highlightedMenuItemElement ?
> +	       this._highlightedMenuItemElement.previousSibling :
this._contextMenuElement.lastChild;

Style: Do not wrap this line.

> Source/WebInspectorUI/UserInterface/Views/SoftContextMenu.js:325
> +	   let menuItemElement = this._highlightedMenuItemElement ?
> +	       this._highlightedMenuItemElement.nextSibling :
this._contextMenuElement.firstChild;

Style: Do not wrap this line.

> Source/WebInspectorUI/UserInterface/Views/SoftContextMenu.js:377
> +if (!InspectorFrontendHost.showContextMenu) {

Just keep this in the InspectorFrontendHostStub, see the earlier comment.