[Webkit-unassigned] [Bug 170705] Web Inspector: No context menu available via ENABLE_INSPECTOR_SERVER

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=170705

Joseph Pecoraro <joepeck at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |joepeck at webkit.org
 Attachment #306755|review?                     |review+, commit-queue-
              Flags|                            |

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

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20170413/64b11e2c/attachment.html>