Bug 80597 – [WK2] Add WKHitTestResultIsContentEditable()

• Component: WebKit2
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

It will be used by the Gtk port to add input methods and unicode menu items to the default context menu for editable elements.

[Carlos Garcia Campos]

Created attachment 130824 [details]
Patch

Patch includes also additions to the GTK API and new test case in GTK+ unit tests.

[WebKit Review Bot]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Alexey Proskuryakov]

Looks good to me. Just one question - how does Mac port achieve the same, and is it inappropriate for Gtk?

[Carlos Garcia Campos]

(In reply to comment #3)
> Looks good to me. Just one question - how does Mac port achieve the same, and is it inappropriate for Gtk?

I don't know if mac has an input methods menu item, ContextMenuItemTagInputMethods enum is specific to GTK+. In any case, the GTK+ API allows the users to ignore the given default context menu and build their own menu using the given hit test result. I think knowing whether hit test is editable content could be useful for other things too.

[Alexey Proskuryakov]

Mac provides a number of menu items in editable context only, and client can configure the menu. So, the next step is to figure out how it's done, so that we don't add multiple APIs for the same thing.

[Carlos Garcia Campos]

So, Anders and Sam should know about how mac does it. In any case the patch simply adds a bool flag to HitTestResult to know whether it's editable content, this is not only used by context menu, but also in didMouseMoveOverElement(), so as I said it could be used for other things too.

[Carlos Garcia Campos]

Actually, all menu items are added to the default context menu by WebCore in the WebProcess, in ContextMenuController, using the hit test result. I guess that's where all editable menu items are added for mac (and other ports). But input methods context menu items are special case, because they are created by GTK+ API (gtk_im_multicontext_append_menuitems), so that needs to be called in the UI process.

[Martin Robinson]

Comment on attachment 130824 [details]
Patch

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

Normally I would not review patches that affect the C WebKit API, but this patch has been sitting for a very long time with no input. The API makes a lot of sense to me, but given that the WebKit2 team hasn't really chimed in, I think it's better to make this API private when landing. You can just move the new API to Source/WebKit2/UIProcess/API/C/WKHitTestResultPrivate.h.

> Source/WebKit2/UIProcess/API/gtk/WebKitHitTestResult.cpp:360
> + * Returns: %TRUE if there's an editable element in the coordinates of the Hit Test,

Maybe @hit_test_result would be better than " the Hit Test"

Should probably say "at the coordinates" instead of "in the coordinates"

[Carlos Garcia Campos]

(In reply to comment #8)
> (From update of attachment 130824 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=130824&action=review
> 
> Normally I would not review patches that affect the C WebKit API, but this patch has been sitting for a very long time with no input. The API makes a lot of sense to me, but given that the WebKit2 team hasn't really chimed in, I think it's better to make this API private when landing. You can just move the new API to Source/WebKit2/UIProcess/API/C/WKHitTestResultPrivate.h.

That would require adding a new file, so I would have the same problem that in bug #79482, I don't have a mac, and I have no idea how to deal with xcode files :-(

> > Source/WebKit2/UIProcess/API/gtk/WebKitHitTestResult.cpp:360
> > + * Returns: %TRUE if there's an editable element in the coordinates of the Hit Test,
> 
> Maybe @hit_test_result would be better than " the Hit Test"

Ok

> Should probably say "at the coordinates" instead of "in the coordinates"

Sure.

Thanks for reviewing!

[Carlos Garcia Campos]

Committed r120297: <http://trac.webkit.org/changeset/120297>