Bug 170705 – Web Inspector: No context menu available via ENABLE_INSPECTOR_SERVER

• Component: Web Inspector
• Status: RESOLVED FIXED

[Ross Kirsling]

Created attachment 306751 [details]
Missing context menu

It appears that ever since the old Web Inspector front-end was removed in r157278, there has been no context menu available when remote inspecting with ENABLE_INSPECTOR_SERVER.

InspectorFrontendHost is unavailable in this case, but the current stub uses a no-op for showContextMenu, where four years ago it used a SoftContextMenu component.

Given that ENABLE_INSPECTOR_SERVER exists, we should reintroduce a non-native context menu to be used in this scenario.

(NOTE: ENABLE_INSPECTOR_SERVER is currently only set for GTK+, but this behavior can be verified by simply overwriting window.InspectorFrontendHost.showContextMenu.)

[Ross Kirsling]

Created attachment 306752 [details]
Proposed context menu

[Ross Kirsling]

Created attachment 306755 [details]
Patch

[Ross Kirsling]

This patch reintroduces the old SoftContextMenu class and applies various fixes:
- Remove legacy globals and prototype extensions.
- Align JS/CSS style with current front-end code.
- Update UI and fix UX to replicate the macOS native context menu.

To verify without ENABLE_INSPECTOR_SERVER, simply "opt-in" to the non-native context menu by commenting out the conditional at SoftContextMenu.js:377.

The appearance and behavior should match the native context menu (exceptions being the lack of a native-backed Inspect Element option and the fact that the menu itself can be right-clicked upon).

[Joseph Pecoraro]

Comment on attachment 306755 [details]
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.

[Ross Kirsling]

Created attachment 307072 [details]
Patch

[Ross Kirsling]

Feedback addressed, including squelching of native Inspect Element.

[Joseph Pecoraro]

Comment on attachment 307072 [details]
Patch

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

Excellent! r=me

> Source/WebInspectorUI/UserInterface/Views/SoftContextMenu.css:3
> + * Copyright (C) 2017 Sony Interactive Entertainment Inc.

Can you add this line to $inspectorLicense in Source/WebInspectorUI/Scripts/copy-user-interface-resources.pl

That is the copyright included appended to minified builds. I normally roll it every now and then but since I remembered now we should add this proactively!

> Source/WebInspectorUI/UserInterface/Views/SoftContextMenu.js:195
> +        // Squelch native Inspect Element.

I don't think "Squelch" is clear terminology.

[Build Bot]

Comment on attachment 307072 [details]
Patch

Attachment 307072 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3531908

New failing tests:
webrtc/multi-video.html

[Build Bot]

Created attachment 307083 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

[Ross Kirsling]

Created attachment 307085 [details]
Patch

[Ross Kirsling]

Feedback addressed.

That test breakage shouldn't be my fault? Looks like this change just landed:
https://bugs.webkit.org/show_bug.cgi?id=170796

[Joseph Pecoraro]

Comment on attachment 307085 [details]
Patch

r=me, would you like me to cq+ this?

[Ross Kirsling]

That would be great! :)

[WebKit Commit Bot]

Comment on attachment 307085 [details]
Patch

Clearing flags on attachment: 307085

Committed r215357: <http://trac.webkit.org/changeset/215357>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.