Bug 176760 - [GTK] Need WebKitContextMenuItemType to open emoji picker
Summary: [GTK] Need WebKitContextMenuItemType to open emoji picker
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: Other
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-09-11 18:17 PDT by Michael Catanzaro
Modified: 2019-05-17 08:10 PDT (History)
10 users (show)

See Also:


Attachments
Patch (60.74 KB, patch)
2019-05-16 06:14 PDT, Carlos Garcia Campos
mcatanzaro: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Catanzaro 2017-09-11 18:17:37 PDT
We need a WebKitContextMenuItemType to open the emoji picker.

I'm not planning to work on this soon, just reporting it.
Comment 1 Cassidy James Blaede 2019-04-30 11:43:37 PDT
Michael, would the keyboard shortcut in GTK to open the emoji-picker popover in text entries be the same issue, or a new one?
Comment 2 Michael Catanzaro 2019-04-30 15:05:48 PDT
Please report a different issue for the keyboard shortcut!
Comment 3 Carlos Garcia Campos 2019-05-16 06:14:37 PDT
Created attachment 370039 [details]
Patch
Comment 4 EWS Watchlist 2019-05-16 06:17:25 PDT
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
Comment 5 Michael Catanzaro 2019-05-16 08:54:42 PDT
Comment on attachment 370039 [details]
Patch

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

I've asked the GTK devs to expose this for GTK 4, so maybe we'll only need to copy it for GTK 3 in the future.

Matthias suggests that we check the master branch for recent fixes to the emoji chooser to make it populate incrementally, which are still pending backport to GTK 3.

> Source/WebKit/ChangeLog:9
> +        context menu or keyboard shortcuts. GtkEmojiChooser is proivate in GTK, so we include our own copy, adapted to

private

> Source/WebKit/ChangeLog:11
> +        content. I'm going to add public API in a follow up patch to be able to use your own chooser, or event prevent

even

> Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:232
> +    CompletionHandler<void(String)> emojiChooserCompletionHandler;
> +    RunLoop::Timer<WebKitWebViewBasePrivate> releaseEmojiChooserTimer;

What happens if the WebKitWebViewBase is destroyed before the timer fires? I assume the callback will not execute. So we will not gtk_widget_destroy() the emoji chooser in that case. But that should be fine, right? Because it's pointing relative to the WebKitWebViewBase, it should be disposed when the parent widget is destroyed?

What happens if the WebKitWebViewBase is destroyed before the emojiChooserCompletionHandler is called, i.e. when emojiChooserCompletionHandler is nonnull at time of destruction? I think the CompletionHandler destructor will assert and crash the UI process, right? We need to make sure the CompletionHandler is guaranteed to be called.

> Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:1797
> +    priv->releaseEmojiChooserTimer.startOneShot(2_min);

Irrelevant: I love this timer syntax. You can't screw up the units!
Comment 6 Carlos Garcia Campos 2019-05-17 01:35:07 PDT
(In reply to Michael Catanzaro from comment #5)
> Comment on attachment 370039 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=370039&action=review
> 
> I've asked the GTK devs to expose this for GTK 4, so maybe we'll only need
> to copy it for GTK 3 in the future.

Yes, I asked about it some time ago too.

> Matthias suggests that we check the master branch for recent fixes to the
> emoji chooser to make it populate incrementally, which are still pending
> backport to GTK 3.

Ok, I'll include fixes in a follow up then.

> > Source/WebKit/ChangeLog:9
> > +        context menu or keyboard shortcuts. GtkEmojiChooser is proivate in GTK, so we include our own copy, adapted to
> 
> private
> 
> > Source/WebKit/ChangeLog:11
> > +        content. I'm going to add public API in a follow up patch to be able to use your own chooser, or event prevent
> 
> even
> 
> > Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:232
> > +    CompletionHandler<void(String)> emojiChooserCompletionHandler;
> > +    RunLoop::Timer<WebKitWebViewBasePrivate> releaseEmojiChooserTimer;
> 
> What happens if the WebKitWebViewBase is destroyed before the timer fires? I
> assume the callback will not execute. So we will not gtk_widget_destroy()
> the emoji chooser in that case. But that should be fine, right? Because it's
> pointing relative to the WebKitWebViewBase, it should be disposed when the
> parent widget is destroyed?

Yes, the timer si stopped in its destructor, so it won't fire if the web view is destroyed. In that case the popover is destroyed with its parent.

> What happens if the WebKitWebViewBase is destroyed before the
> emojiChooserCompletionHandler is called, i.e. when
> emojiChooserCompletionHandler is nonnull at time of destruction? I think the
> CompletionHandler destructor will assert and crash the UI process, right? We
> need to make sure the CompletionHandler is guaranteed to be called.

I'm not sure how can that happen because the popover is modal, so you need to close it first, to close the tab or window. I guess it could still be destroyed programatically somehow, so I'll complete the handler in the dispose.

> > Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:1797
> > +    priv->releaseEmojiChooserTimer.startOneShot(2_min);
> 
> Irrelevant: I love this timer syntax. You can't screw up the units!
Comment 7 Carlos Garcia Campos 2019-05-17 05:39:37 PDT
Committed r245460: <https://trac.webkit.org/changeset/245460>