Bug 124747 – Web Inspector: Allow showing a context menu on all mouse events.

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

[Antoine Quint]

Right now trying to show a context menu on an event other than a contextmenu event will fail. In order to support https://bugs.webkit.org/show_bug.cgi?id=124363, and other UI refinements where we would like to show a context menu with submenus rather than pop-up a hidden <select>, we need to be able to show a context menu from any mouse event.

[Antoine Quint]

Created attachment 217619 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 217619 [details]
Patch

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

There are a lot of layering violations here. r- for this patch because I'd like to see a new patch with them addressed.

Source/WebCore/platform is special. It is supposed to be a base platform layer. Files in that directory should not know about anything else inside of Source/WebCore.

Also after digging around in WebCore more I see ContextMenuController which seems to do a lot of what you want.

Sorry if I was misleading you before, I wasn't really familiar with the ContextMenu handling in WebCore.

> Source/WebCore/ChangeLog:19
> +        * platform/ContextMenu.h:
> +        Add Mac-specific members and Objective-C class to handle the display of an
> +        NSMenu and act as the target of its NSMenuItems.

I think making this Mac-specific is unnecessarily ignoring other ports that share the Inspector.

I don't yet know how this will be used by the inspector, but I would like to avoid seeing code in the Inspector that looks like:

    #if PLATFORM(MAC)
    // Do something really cool
    #else
    // Fallback to something less cool
    #endif

For something like this you can define the interface for everyone, and put stub notImplemented(); impls in other Platform ContextMenu* implementation files, along with a // FIXME: Bug(foo) bugzilla bug for each port.

If the feature is something we can't do in the Inspector without implementation support, then we can make runtime check functions "canShowContextMenuAtAnyTime()" and return true on ports that support the feature and false on others. Eventually when all ports support the feature we could remove the if check. I don't think that would really be needed here though.

So, worst case, the inspector code would end up looking like:

    if (hasCapability(...)) {
      // Do something really cool
    } else {
      // Do something less cool, or nothing at all. This port should really implement capability(...)!
    }

> Source/WebCore/platform/ContextMenu.h:35
> +#include "Event.h"
> +#include "Page.h"

Layering violation.

> Source/WebCore/platform/ContextMenu.h:49
> +#ifdef __OBJC__
> +@class WebContextMenuTarget;
> +#else
> +class WebContextMenuTarget;
> +#endif

Ugly pattern eh? We have macros for this! You should be able to do:

    OBJC_CLASS WebContextMenuTarget;

> Source/WebCore/platform/ContextMenu.h:102
> +        WebContextMenuTarget* m_contextMenuTarget;

Nit: RetainPtr

> Source/WebCore/platform/ContextMenu.h:103
> +        Page* m_page;

m_page is no longer used.

> Source/WebCore/platform/mac/ContextMenuMac.mm:35
> +#include "Document.h"
> +#include "DOMWrapperWorld.h"
> +#include "EventTarget.h"
> +#include "FrameView.h"
> +#include "Node.h"
> +#include "MouseEvent.h"
> +#include "ScriptFunctionCall.h"
> +#include "UserGestureIndicator.h"

Here are a lot of layering violations that would need to be addressed.

> Source/WebCore/platform/mac/ContextMenuMac.mm:50
> +    m_contextMenuTarget = [[WebContextMenuTarget alloc] init];

This would be leaked as written because m_contextMenuTarget is never deallocated. Also, the existing NSMutableArray allocation is sub-optimal. Using RetainPtr would be better, less error prone. I'd much rather see m_contextMenuTarget be a RetainPtr and this kind of constructor be rewritten as:

    ContextMenu::ContextMenu()
        : m_platformDescription(adoptNS([[NSMutableArray alloc] init]))
        , m_contextMenuTarget(adoptNS([[WebContextMenuTarget alloc] init]))
    {
    }

Less retain/release churn, and now you don't need to worry about releasing the ivars.

> Source/WebCore/platform/mac/ContextMenuMac.mm:182
> +@implementation WebContextMenuTarget
> +
> +- (void)menuSelection:(NSMenuItem *)menuItem
> +{
> +    JSC::ExecState* frontendExecState = execStateFromPage(WebCore::debuggerWorld(), self.page);
> +    WebCore::ScriptObject frontendApiObject;
> +    if (!WebCore::ScriptGlobalObject::get(frontendExecState, "InspectorFrontendAPI", frontendApiObject)) {
> +        ASSERT_NOT_REACHED();
> +        return;
> +    }
> +
> +    WebCore::UserGestureIndicator gestureIndicator(WebCore::DefinitelyProcessingUserGesture);
> +    WebCore::ScriptFunctionCall function(frontendApiObject, "contextMenuItemSelected");
> +    function.appendArgument([menuItem tag] - WebCore::ContextMenuItemBaseCustomTag);
> +    function.call();
> +}
> +
> +@end

WebCore/platform/ContextMenu should certainly not be running JavaScript on a page!

A better API model would be to give the ContextMenu a ContextMenuClient. And that ContextMenuClient, presumably in the Inspector would be told of the menu selection and maybe also if the menu was dismissed without a selection.

In fact, I think that is what WebCore/page/ContextMenuController.h already provides.

    • ContextMenuController contains a platform ContextMenu
    • ContextMenuController takes in a Page and Client: ContextMenuController(Page&, ContextMenuClient&);
    • ContextMenuController's API has: void showContextMenu(Event*, PassRefPtr<ContextMenuProvider>);
    • ContextMenuClient has methods for a selection and when the menu is destroyed (maybe that means dismissed):
        virtual void contextMenuDestroyed() = 0;
        virtual void contextMenuItemSelected(ContextMenuItem*, const ContextMenu*) = 0;

It looks like ContextMenuController is already a platform abstraction over "show a context menu". I think Inspector code should be able to create and use your own ContextMenuController in Inspector code.

Some notes about ContextMenuController. It looks like is geared specifically toward being a "Page" right click context menu. For example it has:

    #if ENABLE(INSPECTOR)
        if (m_page.inspectorController()->enabled())
            addInspectElementItem();
    #endif

And a populate method with all kinds of page specific stuff.

I think you might want to abstract ContextMenuController to be more generic, and make a PageContextMenuController for WebCore::Page, and use the newly clean base ContextMenuController for the inspector.

[Antoine Quint]

Thanks for the thorough review Joe. There's actually another approach that we could take that appears to be simpler. We could "simply" call ContextMenuController::showContextMenuAt() when we're asked to show a context menu in something other than a contextmenu event handler. This would generate a contextmenu event at the same location as the mouse event and let the regular context menu behaviour operate. This adds fairly little code to ContextMenuController::showContextMenu(Event* event):

#if USE(ACCESSIBILITY_CONTEXT_MENUS)
    if (event->type() != eventNames().contextmenuEvent && event->isMouseEvent()) {
        Frame* frame = event->target()->toNode()->document().frame();
        MouseEvent* mouseEvent = toMouseEvent(event);
        IntPoint mousePoint = IntPoint(mouseEvent->clientX(), mouseEvent->clientY());
        showContextMenuAt(frame, mousePoint);
    }
#endif

Maybe we should do something like this. Perhaps we could add a new             InspectorFrontendHost.dispatchContextMenuEvent that would automatically be called from WebInspector.ContextMenu.prototype.show() in the cases where the event that was passed in was not a mouse event.

[Antoine Quint]

Created attachment 217678 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 217678 [details]
Patch

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

r=me, but I do have enough questions that I'd be happy to look at an updated patch if you have enough changes.

> Source/WebCore/inspector/InspectorFrontendHost.cpp:295
> +    m_frontendPage->contextMenuController().showContextMenuAt(frame, mousePoint);

By using Page's ContextMenuController, does that mean you get things like "Inspect Element" added to the context menu? Or is the accessibility context menu path special?

> Source/WebCore/inspector/InspectorFrontendHost.idl:65
> +    [Custom] void simulateContextmenuEvent(MouseEvent event);

Does this need to be custom because of MouseEvent? I think showContextMenu needed to be custom because of "any".

I'm not sure about the name, but I could live with it. I'd rather see correct camel casing though. Here are some suggestions:

    void simulateContextMenuEvent(...)
    void simulateContextMenuEventAndShowContextMenu(...)
    void dispatchContextMenuEventWithEvent(...)
    void showContextMenuWithoutEvent(...)
    void showContextMenuAtLocation(x, y)

Will this be difficult to implement in RWIInspectorFrontendHost? Should that provide a slightly different API?

> Source/WebInspectorUI/UserInterface/ContextMenu.js:199
> +        this._menuObject = this._buildDescriptor();

I'd like to see:

    console.assert(!this._menuObject);

And the menuObject is deleted when it it no longer needed.

> Source/WebInspectorUI/UserInterface/ContextMenu.js:205
> +            if (this._event.type !== "contextmenu" && typeof InspectorFrontendHost.simulateContextmenuEvent === "function") {
> +                this._event.target.addEventListener("contextmenu", this, true);
> +                InspectorFrontendHost.simulateContextmenuEvent(this._event);

Seeing as this only works with a MouseEvent, we should test and ensure that the event is a context event or a mouse event. Perhaps an assert. You might be able to just check "instanceof MouseEvent".

> Source/WebInspectorUI/UserInterface/ContextMenu.js:207
> +                InspectorFrontendHost.showContextMenu(this._event, this._menuObject);

this can delete this._menuObject because it is done with it.

> Source/WebInspectorUI/UserInterface/ContextMenu.js:218
> +        InspectorFrontendHost.showContextMenu(event, this._menuObject);

And this can delete this._menuObject.

[Antoine Quint]

(In reply to comment #5)
> (From update of attachment 217678 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=217678&action=review
> 
> r=me, but I do have enough questions that I'd be happy to look at an updated patch if you have enough changes.

I'll have an updated patch shortly for you to look at.

> > Source/WebCore/inspector/InspectorFrontendHost.cpp:295
> > +    m_frontendPage->contextMenuController().showContextMenuAt(frame, mousePoint);
> 
> By using Page's ContextMenuController, does that mean you get things like "Inspect Element" added to the context menu? Or is the accessibility context menu path special?

"Inspect Element" is indeed added.

> > Source/WebCore/inspector/InspectorFrontendHost.idl:65
> > +    [Custom] void simulateContextmenuEvent(MouseEvent event);
> 
> Does this need to be custom because of MouseEvent? I think showContextMenu needed to be custom because of "any".

Yes, it was the MouseEvent. I'll change it to Event which removes the requires for the [Custom].

> I'm not sure about the name, but I could live with it. I'd rather see correct camel casing though. Here are some suggestions:
> 
>     void simulateContextMenuEvent(...)
>     void simulateContextMenuEventAndShowContextMenu(...)
>     void dispatchContextMenuEventWithEvent(...)
>     void showContextMenuWithoutEvent(...)
>     void showContextMenuAtLocation(x, y)

I actually went with dispatchEventAsContextMenuEvent(…).

> Will this be difficult to implement in RWIInspectorFrontendHost? Should that provide a slightly different API?

There is nothing to be done with the RWIInspectorFrontendHost, showing a contextual menu outside of a contextmenu event has always worked in the RWI because the host provides a very different handling of the context menu.

> > Source/WebInspectorUI/UserInterface/ContextMenu.js:199
> > +        this._menuObject = this._buildDescriptor();
> 
> I'd like to see:
> 
>     console.assert(!this._menuObject);
> 
> And the menuObject is deleted when it it no longer needed.

I'm actually reverting to only setting this._contextMenu in the code branch where we end up using InspectorFrontendHost.dispatchEventAsContextMenuEvent.

> > Source/WebInspectorUI/UserInterface/ContextMenu.js:205
> > +            if (this._event.type !== "contextmenu" && typeof InspectorFrontendHost.simulateContextmenuEvent === "function") {
> > +                this._event.target.addEventListener("contextmenu", this, true);
> > +                InspectorFrontendHost.simulateContextmenuEvent(this._event);
> 
> Seeing as this only works with a MouseEvent, we should test and ensure that the event is a context event or a mouse event. Perhaps an assert. You might be able to just check "instanceof MouseEvent".

I'm adding this assert at the top of the function:

        console.assert(this._event instanceof MouseEvent);

> > Source/WebInspectorUI/UserInterface/ContextMenu.js:207
> > +                InspectorFrontendHost.showContextMenu(this._event, this._menuObject);
> 
> this can delete this._menuObject because it is done with it.

I'll be deleting this._menuObject in handleEvent.

> > Source/WebInspectorUI/UserInterface/ContextMenu.js:218
> > +        InspectorFrontendHost.showContextMenu(event, this._menuObject);
> 
> And this can delete this._menuObject.

Right.

[Antoine Quint]

Created attachment 217800 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 217800 [details]
Patch

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

r=me, thanks

> Source/WebCore/inspector/InspectorFrontendHost.cpp:296
> +#endif

You should add the follow for non-ACCESSIBILITY_CONTEXT_MENUS compiles:

    #else
        UNUSED_PARAM(event);
    #endif

[Antoine Quint]

Created attachment 217866 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 217866 [details]
Patch for landing

Clearing flags on attachment: 217866

Committed r159780: <http://trac.webkit.org/changeset/159780>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.