Bug 70528 – [Qt][WK2] Add a QML based solution for Context Menus

• Component: WebKit Qt
• Status: RESOLVED INVALID

[Zeno Albisser]

ssia

[Nandor Huszka]

I have started implementing the API, then I'd like to do the other things.

[Nandor Huszka]

Created attachment 143525 [details]
WIP patch

I would need some tip about this task. Originally I'd like to use ContextMenu in Snowshoe, this is why I have started dealing with this. Context menu API should be similar to existing itemSelector, I know this from INdT guys.
But I don't know exactly which files should I modify, however I have made a WIP patch.

WebContextMenuProxyQt, ContextMenuQt, and ContextMenuItemQt have a lot of unimplemented methods, should I complete these? E.g. createNativeMenuFromItems is missing from Qt api, but I don't know whether we have to use these methods.

There is an other question: do I need for example a ContextMenuItemModel and a ContextMenuContextObject class in WebContextMenuProxyQt, and do the things similar to WebPopupMenuProxyQt (in it can be found the itemSelector's stuffs)? Or should I follow the other platforms' WebContextMenuProxy implementation without these new classes (as I have started in the patch)?

What will be the correct way broadly, which things should I implement?

[Kenneth Rohde Christiansen]

Comment on attachment 143525 [details]
WIP patch

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

> Source/WebKit2/UIProcess/API/qt/tests/qmltests/DesktopBehavior/tst_contextMenu.qml:9
> +// FIXME: Moved to Desktop tests because we want to have mouseClick() to open the <select> tag. We can move it back
> +// when TestCase starts supporting touch events, see https://bugreports.qt.nokia.com/browse/QTBUG-23083.
> +TestWebView {

Can't you use the new testing frame work that I did. experimental.test.touchTapEvent() etc?

> Source/WebKit2/UIProcess/API/qt/tests/qmltests/DesktopBehavior/tst_contextMenu.qml:18
> +    experimental.contextMenu: Item {
> +        Component.onCompleted: {
> +        }
> +    }

So how do I use it?

x: requestedPos.x ?

how to show the items etc?

[Nandor Huszka]

(In reply to comment #3)
> (From update of attachment 143525 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=143525&action=review
> 
> > Source/WebKit2/UIProcess/API/qt/tests/qmltests/DesktopBehavior/tst_contextMenu.qml:9
> > +// FIXME: Moved to Desktop tests because we want to have mouseClick() to open the <select> tag. We can move it back
> > +// when TestCase starts supporting touch events, see https://bugreports.qt.nokia.com/browse/QTBUG-23083.
> > +TestWebView {
> 
> Can't you use the new testing frame work that I did. experimental.test.touchTapEvent() etc?
> 
> > Source/WebKit2/UIProcess/API/qt/tests/qmltests/DesktopBehavior/tst_contextMenu.qml:18
> > +    experimental.contextMenu: Item {
> > +        Component.onCompleted: {
> > +        }
> > +    }
> 
> So how do I use it?
> 
> x: requestedPos.x ?
> 
> how to show the items etc?

Should this touchTapEvent be a new function? Because I have not found this in WK. To tell the truth first I would like to come aware of the correct API implementing, there is a lot of thing that I don't understand. After that I will try this of course. Thank you for the suggestion.

[Nandor Huszka]

(In reply to comment #4)
> (In reply to comment #3)
> > (From update of attachment 143525 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=143525&action=review
> > 
> > > Source/WebKit2/UIProcess/API/qt/tests/qmltests/DesktopBehavior/tst_contextMenu.qml:9
> > > +// FIXME: Moved to Desktop tests because we want to have mouseClick() to open the <select> tag. We can move it back
> > > +// when TestCase starts supporting touch events, see https://bugreports.qt.nokia.com/browse/QTBUG-23083.
> > > +TestWebView {
> > 
> > Can't you use the new testing frame work that I did. experimental.test.touchTapEvent() etc?
> > 
> > > Source/WebKit2/UIProcess/API/qt/tests/qmltests/DesktopBehavior/tst_contextMenu.qml:18
> > > +    experimental.contextMenu: Item {
> > > +        Component.onCompleted: {
> > > +        }
> > > +    }
> > 
> > So how do I use it?
> > 
> > x: requestedPos.x ?
> > 
> > how to show the items etc?
> 
> Should this touchTapEvent be a new function? Because I have not found this in WK. To tell the truth first I would like to come aware of the correct API implementing, there is a lot of thing that I don't understand. After that I will try this of course. Thank you for the suggestion.

I had a discussion with Zeno, now I understand what would be the correct way, and why starting by the api tests is recommended.

[Nandor Huszka]

Created attachment 151938 [details]
WIP patch

Now the menu appears on right clicking at the actual position of the mouse, and it is filled with elements that depends on the clicked element.
We know the WebCore::ContextMenuItemType and WebCore::ContextMenuAction values also.
The question is: how can be called the appropriate action in ContextMenuContextObject::accept possession of these values? For example when I click on the "Copy" menuitem, its ContextMenuItemType=ActionType and ContextMenuAction=ContextMenuItemTagCopy, but what to do with these things?

[Nandor Huszka]

Created attachment 152520 [details]
Patch

Now the contextmenu appears on right clicking, and by selecting an element the appropriate action is performed. 
All the menuitems are put in the main contextmenu. I think the QML file could handle the clicking on an item which has submenuitems (and draw a new rectangle with these items) in an other patch. Is it OK? 
I would appreciate your opinion about the API too, e.g. what is missing, or what should be implemented in an other way.

[Zeno Albisser]

Comment on attachment 152520 [details]
Patch

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

> Source/WebKit2/UIProcess/qt/WebContextMenuProxyQt.cpp:197
> +    ContextMenuContextObject* contextObject = new ContextMenuContextObject(QPoint(position.x(), position.y()), items);

How about moving the ContextMenuContextObject creation into the "createItem" function as well?
This way you don't even need to create it, in case you don't receive a component.

[Nandor Huszka]

(In reply to comment #8)
> (From update of attachment 152520 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=152520&action=review
> 
> > Source/WebKit2/UIProcess/qt/WebContextMenuProxyQt.cpp:197
> > +    ContextMenuContextObject* contextObject = new ContextMenuContextObject(QPoint(position.x(), position.y()), items);
> 
> How about moving the ContextMenuContextObject creation into the "createItem" function as well?
> This way you don't even need to create it, in case you don't receive a component.

It is unnecessary to create it in that case, I agree with you. 
What's up with the submenus? Do we need a second model object for storing the selected item's submenuitems (then passing it to a second ListView's model property e.g.)?

[Zeno Albisser]

(In reply to comment #9)
> It is unnecessary to create it in that case, I agree with you. 
> What's up with the submenus? Do we need a second model object for storing the selected item's submenuitems (then passing it to a second ListView's model property e.g.)?

You mean such as nested menus?
Looking at other browsers i can't find such menus anywhere.
I think it is fine for the moment to leave it out.

[Nandor Huszka]

(In reply to comment #10)
> (In reply to comment #9)
> > It is unnecessary to create it in that case, I agree with you. 
> > What's up with the submenus? Do we need a second model object for storing the selected item's submenuitems (then passing it to a second ListView's model property e.g.)?
> 
> You mean such as nested menus?
> Looking at other browsers i can't find such menus anywhere.
> I think it is fine for the moment to leave it out.

Yes, I thought about nested menus, that are shown when the mouse is over their parent menuitem. Okay, in this case I will fix the createItem thing.

[Nandor Huszka]

Created attachment 152988 [details]
Patch

Fixed the createItem thing. Context menu can be tried by running the MiniBrowser on desktop for the time being.

[Early Warning System Bot]

Comment on attachment 152988 [details]
Patch

Attachment 152988 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13279517

[Nandor Huszka]

Created attachment 152993 [details]
Patch

I skipped the ContextMenu.qml from the diff, reupload it.

[Balazs Kelemen]

Comment on attachment 152988 [details]
Patch

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

Informal review. Although I don't know these QML things enough to review the patch, basically it seems sane to me.
I think you should add a test for this functionality.

> Source/WebKit2/UIProcess/qt/WebContextMenuProxyQt.cpp:43
> +static QHash<int, QByteArray> createRoleNamesHash();
> +static int menuSize(const Vector<WebContextMenuItemData>&);

Do you need to forward declare these at all? If so, move these below the classes, please.

> Source/WebKit2/UIProcess/qt/WebContextMenuProxyQt.cpp:76
> +        WebCore::ContextMenuItemType type;
> +        WebCore::ContextMenuAction action;

No need to qualify with WebCore:: here I guess, and we normally use using and not qualify.

> Source/WebKit2/UIProcess/qt/WebContextMenuProxyQt.cpp:114
> +ContextMenuContextObject::ContextMenuContextObject(const QPoint& position, const Vector<WebContextMenuItemData>& items)
> +    : m_position(position),
> +      m_itemModel(items)

Style error: you should put the , at the next line.

> Source/WebKit2/UIProcess/qt/WebContextMenuProxyQt.cpp:147
> +    static QHash<int, QByteArray> roles = createRoleNamesHash();

It would look slightly better to my eyes if you move the static var to the function and use DEFINE_STATIC_LOCAL.

> Source/WebKit2/UIProcess/qt/WebContextMenuProxyQt.cpp:179
> +            if (!submenuItemsData.isEmpty())
> +                buildItems(submenuItemsData, menuItems);

I'm not sure we should insert these items at all if we don't implement submenus. What are these items contains? What is the use case of submenus?

> Source/WebKit2/UIProcess/qt/WebContextMenuProxyQt.cpp:241
> +    qRegisterMetaType<WebContextMenuItemData>("WebContextMenuItemData");
> +    connect(contextObject, SIGNAL(contextMenuItemSelected(WebContextMenuItemData)), SLOT(selectContextMenuItem(WebContextMenuItemData)), Qt::QueuedConnection);
> +    connect(contextObject, SIGNAL(rejected()), SLOT(hideContextMenu()), Qt::QueuedConnection);

Why it is necessary to use queued connection? If it really is, than please add a comment why.

[Nandor Huszka]

(In reply to comment #15)
> (From update of attachment 152988 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=152988&action=review
> Do you need to forward declare these at all? If so, move these below the classes, please.

> No need to qualify with WebCore:: here I guess, and we normally use using and not qualify.

> Style error: you should put the , at the next line.

> It would look slightly better to my eyes if you move the static var to the function and use DEFINE_STATIC_LOCAL.

These comments are justified, I've fixed them.

> > Source/WebKit2/UIProcess/qt/WebContextMenuProxyQt.cpp:179
> > +            if (!submenuItemsData.isEmpty())
> > +                buildItems(submenuItemsData, menuItems);
> 
> I'm not sure we should insert these items at all if we don't implement submenus. What are these items contains? What is the use case of submenus?

Submenus contains menuitems that allow us to set e.g. the direction of text, font style and spelling. They are inserted next to their parent menuitem now. So the user can also select them from the main context menu, that is why they are stored too. It isn't very elegant, I know, but I hope now it is passable, and maybe it can be improved in a future patch.

> > Source/WebKit2/UIProcess/qt/WebContextMenuProxyQt.cpp:241
> > +    qRegisterMetaType<WebContextMenuItemData>("WebContextMenuItemData");
> > +    connect(contextObject, SIGNAL(contextMenuItemSelected(WebContextMenuItemData)), SLOT(selectContextMenuItem(WebContextMenuItemData)), Qt::QueuedConnection);
> > +    connect(contextObject, SIGNAL(rejected()), SLOT(hideContextMenu()), Qt::QueuedConnection);
> 
> Why it is necessary to use queued connection? If it really is, than please add a comment why.

If we do not use the third parameter in connect, the MiniBrowser terminates with segfault. What's more, the same happens when we use any other type of connection (except BlockingQueuedConnection, in this case deadlock is caused). In WebPopupMenuProxyQt::createItem the same connection type is used, and there can be found a comment that says:
"We enqueue these because they are triggered by m_itemSelector and will lead to its destruction."
In our case this explanation is also valid, I think, so I will insert it, if it is acceptable in your opinion too.

> I think you should add a test for this functionality.

You are right, api test is missing yet, because simulation of right clicking there isn't implemented yet. I'm afraid of its solution is waiting for me. :) So should I create a bug with that functionality (that will block this bug), or solve it here too?

[Balazs Kelemen]

> These comments are justified, I've fixed them.
> 
> > > Source/WebKit2/UIProcess/qt/WebContextMenuProxyQt.cpp:179
> > > +            if (!submenuItemsData.isEmpty())
> > > +                buildItems(submenuItemsData, menuItems);
> > 
> > I'm not sure we should insert these items at all if we don't implement submenus. What are these items contains? What is the use case of submenus?
> 
> Submenus contains menuitems that allow us to set e.g. the direction of text, font style and spelling. They are inserted next to their parent menuitem now. So the user can also select them from the main context menu, that is why they are stored too. It isn't very elegant, I know, but I hope now it is passable, and maybe it can be improved in a future patch.

I don't really like the idea to implement context menu with fake submenus.
Maybe we would have to rework the whole solution in the future to handle them, so instead I propose to try to implement it properly at first. If you still prefer going incrementally, and skip submenus at first, than it seems better to not show submenu items at all for now.

> 
> > > Source/WebKit2/UIProcess/qt/WebContextMenuProxyQt.cpp:241
> > > +    qRegisterMetaType<WebContextMenuItemData>("WebContextMenuItemData");
> > > +    connect(contextObject, SIGNAL(contextMenuItemSelected(WebContextMenuItemData)), SLOT(selectContextMenuItem(WebContextMenuItemData)), Qt::QueuedConnection);
> > > +    connect(contextObject, SIGNAL(rejected()), SLOT(hideContextMenu()), Qt::QueuedConnection);
> > 
> > Why it is necessary to use queued connection? If it really is, than please add a comment why.
> 
> If we do not use the third parameter in connect, the MiniBrowser terminates with segfault. What's more, the same happens when we use any other type of connection (except BlockingQueuedConnection, in this case deadlock is caused). In WebPopupMenuProxyQt::createItem the same connection type is used, and there can be found a comment that says:
> "We enqueue these because they are triggered by m_itemSelector and will lead to its destruction."
> In our case this explanation is also valid, I think, so I will insert it, if it is acceptable in your opinion too.

Sure, please copy the comment here as well.

> 
> > I think you should add a test for this functionality.
> 
> You are right, api test is missing yet, because simulation of right clicking there isn't implemented yet. I'm afraid of its solution is waiting for me. :) So should I create a bug with that functionality (that will block this bug), or solve it here too?

I think first you can concentrate on implementing it properly, than we can decide how to test. Another bug for the testing stuff is good idea, though.

[Nandor Huszka]

Created attachment 154630 [details]
Patch

(In reply to comment #17)
Submenus are done and everything you've mentioned. I will create a new bug for the API test, that blocks this.

[Simon Hausmann]

Comment on attachment 154630 [details]
Patch

For a different approach that I was thinking of last year see also https://lists.webkit.org/pipermail/webkit-qt/2011-October/001943.html

The basic idea is to let the developer not only specify the component for the context menu itself but also the component for individual items.

The goal in the end should be an API that is really easy to use and that easily combines with APIs such as the Qt Quick Components for MeeGo (regardless of their lifetime the API principles used there still apply and are also used in the Qt Desktop Components).

[Simon Hausmann]

A further thought of my previous comment: In the latest Qt Desktop Components (Context)Menu and MenuItem appear to be nestable:

    ContextMenu {
        MenuItem {
            ...
        }
        Menu {
            MenuItem {
                ...
            }
        ...
     }

that in turn maps directly to the approach of specifying menu item and menu component separately and WebKit can construct the entire menu hierarchy.

One critical question with this approach is: How easy is it to customize the context menu?

[Nandor Huszka]

(In reply to comment #19)
> The basic idea is to let the developer not only specify the component for the context menu itself but also the component for individual items.

I have just realized the importance of this, because for example a checkable menuitem should have a tick, but now we cannot fit up it with one.
However in WebKit we cannot know which items should be put into the contextmenu in compile time, but a javascript function could fill it that uses the API for this. So e.g. in MiniBrowser I'd like to create a contextmenu without any menuitems, and the contextmenu will create these after we ask it.

(In reply to comment #20)
> that in turn maps directly to the approach of specifying menu item and menu component separately and WebKit can construct the entire menu hierarchy.

I see, the implementation needs some redesign.

    ContextMenu {
        MenuItem {
            ...
        }
        Menu {

Will we able to create e.g. this submenu (as single menuitems) when we get an item from the api that have  submenuelements in runtime? The API's or the ContextMenu's role should be this mainly?

            MenuItem {
                ...
            }
        ...
     }

[Simon Hausmann]

(In reply to comment #21)
> (In reply to comment #19)
> > The basic idea is to let the developer not only specify the component for the context menu itself but also the component for individual items.
> 
> I have just realized the importance of this, because for example a checkable menuitem should have a tick, but now we cannot fit up it with one.
> However in WebKit we cannot know which items should be put into the contextmenu in compile time, but a javascript function could fill it that uses the API for this. So e.g. in MiniBrowser I'd like to create a contextmenu without any menuitems, and the contextmenu will create these after we ask it.
> 
> (In reply to comment #20)
> > that in turn maps directly to the approach of specifying menu item and menu component separately and WebKit can construct the entire menu hierarchy.
> 
> I see, the implementation needs some redesign.
> 
>     ContextMenu {
>         MenuItem {
>             ...
>         }
>         Menu {
> 
> Will we able to create e.g. this submenu (as single menuitems) when we get an item from the api that have  submenuelements in runtime? The API's or the ContextMenu's role should be this mainly?

I'm not 100% sure that I understand the question correctly. Note that what is specified in the API is just a _component_, not a concrete item. Think of components as factories to create items of a specific "type" on demand.

So the user of the API specifies the components to use of menu items and menus themselves. If WebKit at run-time decides  that it needs to create a menu item, then it can instantiate one. If it then realizes that it needs to create a menu that can be inserted into an existing menu as a sub-menu, it can simply instantiate a new menu.

Does that make any sense? :)

[Nandor Huszka]

(In reply to comment #22)
> I'm not 100% sure that I understand the question correctly. Note that what is specified in the API is just a _component_, not a concrete item. Think of components as factories to create items of a specific "type" on demand.

The question wasn't exact enough, it's true. I was not aware of the behaviour of the UI part, whether we can create QML types in runtime, but I've found the way of it. I see that e.g. in  our case the ContextMenuItemModel is the component you've mentioned, and with the help of ContextMenuContextObject it can be instantiated, so it can be used for a concrete menuitem in the QML file. But we should have a component for menus also, not only for menuitems, and QML types for them.

> So the user of the API specifies the components to use of menu items and menus themselves. If WebKit at run-time decides  that it needs to create a menu item, then it can instantiate one. If it then realizes that it needs to create a menu that can be inserted into an existing menu as a sub-menu, it can simply instantiate a new menu.
> 
> Does that make any sense? :)

Yes, now it's more clear, thank you for the explanation. :)

[Simon Hausmann]

Comment on attachment 154630 [details]
Patch

Taking this out of the review queue, as the approach needs re-thinking, which is already happening :)

[Nandor Huszka]

Created attachment 157668 [details]
Version 2 of the patch

I've done several modifications, e.g. solving it without ListView, as a result we do not have to use QAbstractListModel. I've separated the QML files, so the menuitems are customizable now. In the API there is an extra class named ContextMenuModel that contains the models of the menuitems. There is a problem with showing a submenu: however it is shown and its size is correct, but the items of the mainmenu are put in it, despite of we pass it them.
I'd like to ask your opinion about this patch. It is far from the final version, I know. :) Do you think something like this in the previous comments?

[Nandor Huszka]

(In reply to comment #25)
> I've done several modifications, e.g. solving it without ListView, as a result we do not have to use QAbstractListModel. I've separated the QML files, so the menuitems are customizable now. In the API there is an extra class named ContextMenuModel that contains the models of the menuitems. There is a problem with showing a submenu: however it is shown and its size is correct, but the items of the mainmenu are put in it, despite of we pass it them.
> I'd like to ask your opinion about this patch. It is far from the final version, I know. :) Do you think something like this in the previous comments?

So, what do you think? Is it the right track aside from the submenu problem?