1381983 - Add context menu/long-press handler to GeckoView's content delegate

Status: RESOLVED FIXED

(GeckoView :: General, enhancement)

Description

[Eugen Sawin [:esawin]]

We should provide a way for a GeckoView client to handle long-press events on certain content types like links and images.

Comment 1

[Eugen Sawin [:esawin]]

Attachment: 0001-Bug-1381983-1.0-Add-context-menu-event-handler-to-th.patch

With this patch we expose the "contextmenu" through GeckoView.ContentDelegate.onContextMenu, providing the screen coordinates of the long press/right click, a basic type ("link", "image") of the context and its URI.

This should be enough for basic context menu implementations.

Comment 2

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8887651 [details] [diff] [review]
0001-Bug-1381983-1.0-Add-context-menu-event-handler-to-th.patch

Review of attachment 8887651 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/chrome/geckoview/GeckoViewContent.js
@@ +73,5 @@
>      switch (aEvent.type) {
> +      case "contextmenu":
> +        let context = null;
> +        let uri = null;
> +        if (aEvent.target.href) {

Fennec shows two "tabs" with both the link and image options if you click a linked image. We'd need to support that same sort of functionality if we want to replicate what Fennec does.

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoView.java
@@ +1175,5 @@
>          void onFullScreen(GeckoView view, boolean fullScreen);
> +
> +
> +        void onContextMenu(GeckoView view, int screenX, int screenY,
> +                           String type, String uri);

Don't use a string for the type, use an enum or at least an integer with some constants defined.

Comment 3

[Eugen Sawin [:esawin]]

Attachment: 0001-Bug-1381983-1.1-Add-context-menu-event-handler-to-th.patch

With this patch, we support links (with nested nodes), images and image-links (images nested in link nodes), which should be sufficient to replicate Fennec's behavior.

I've also removed the context type since it can be implied from the provided callback arguments (for links uri will be set, for images imageSrc).

Comment 4

[Eugen Sawin [:esawin]]

Attachment: 0002-Bug-1381983-2.1-Add-default-stub-implementations-of-.patch

Comment 5

[Jan Henning [:JanH]]

(In reply to Eugen Sawin [:esawin] from comment #3)
> With this patch, we support links (with nested nodes), images and
> image-links (images nested in link nodes), which should be sufficient to
> replicate Fennec's behavior.

Not quite - what about media elements?

Comment 6

[Eugen Sawin [:esawin]]

Attachment: 0003-Bug-1381983-3.0-Add-context-menu-support-for-media-e.patch

Good point, Jan!

We should enable it for all media elements.
At this point, we might also want to provide some attributes besides the source.

Comment 7

[Jan Henning [:JanH]]

Comment on attachment 8888072 [details] [diff] [review]
0003-Bug-1381983-3.0-Add-context-menu-support-for-media-e.patch

Seems fine for now, and I guess the picture of what else might be needed will become clearer anyway once we actually attempt to use this.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/60f7e2cb890c
[1.1] Add context menu event handler to the content delegate. r=snorp
https://hg.mozilla.org/integration/mozilla-inbound/rev/21a44de78be3
[2.1] Add default/stub implementations of the context menu handlers. r=snorp
https://hg.mozilla.org/integration/mozilla-inbound/rev/e00f60085ca6
[3.0] Add context menu support for media elements. r=snorp

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/60f7e2cb890c
https://hg.mozilla.org/mozilla-central/rev/21a44de78be3
https://hg.mozilla.org/mozilla-central/rev/e00f60085ca6