Closed Bug 1381983 Opened 7 years ago Closed 7 years ago

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

Categories

(GeckoView :: General, enhancement)

51 Branch
All
Android
enhancement
Not set
normal

Tracking

(firefox56 fixed)

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: esawin, Assigned: esawin)

References

Details

Attachments

(3 files, 1 obsolete file)

We should provide a way for a GeckoView client to handle long-press events on certain content types like links and images.
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.
Attachment #8887651 - Flags: feedback?(snorp)
Blocks: 1365868
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.
Attachment #8887651 - Flags: feedback?(snorp) → feedback-
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).
Attachment #8887651 - Attachment is obsolete: true
Attachment #8888032 - Flags: review?(snorp)
Attachment #8888032 - Flags: review?(snorp) → review+
Attachment #8888033 - Flags: review?(snorp) → review+
(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?
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.
Attachment #8888072 - Flags: review?(snorp)
Attachment #8888072 - Flags: feedback?(jh+bugzilla)
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.
Attachment #8888072 - Flags: feedback?(jh+bugzilla) → feedback+
Attachment #8888072 - Flags: review?(snorp) → review+
Pushed by esawin@mozilla.com:
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
Blocks: 1398067
Product: Firefox for Android → GeckoView
Target Milestone: Firefox 56 → mozilla56
You need to log in before you can comment on or make changes to this bug.