853700 - Allow adding context menu items to native UI

Status: RESOLVED INCOMPLETE

(Firefox for Android Graveyard :: General, defect)

Description

[Wesley Johnston (:wesj)]

It would be pretty darn neat (and remove a lot of java code complexity) if you could add context menu items to the native UI. Something along the lines of:

NativeWindow.contextmenus.add("Test", NativeWindow.URLBAR, function() {
  alert("Test");
});

or more advanced (part 2?):

NativeWindow.contextmenus.add("Test", NativeWindow.AWESOMEBAR_ITEM.when(function(item) {
  return item.url == myURL;
}, function() {
  alert("Test");
});

We can still be very careful and only expose UI pieces that we want. Functions like the "when" bit above would likely vary for different UI pieces?

Comment 1

[Wesley Johnston (:wesj)]

Attachment: WIP Patch

Just a beginning piece for this. Doesn't all work yet.

Comment 2

[Wesley Johnston (:wesj)]

Attachment: Patch v1

This seems to work well for me.

Comment 3

[Wesley Johnston (:wesj)]

Attachment: Patch - Add awesomebar screen

And a second little patch to show how easy it is to add new native ui chunks.

Comment 4

[Wesley Johnston (:wesj)]

Attachment: Patch - Add awesomebar screen

Grr. I am going back to all mc. Trying to juggle repos is not something I am good at.

Comment 5

[Wesley Johnston (:wesj)]

Attachment: Patch v1

Comment 6

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 728381 [details] [diff] [review]
Patch v1

Missing NativeWindow.java ?

I need to think a little more about the round trip that happens with "NativeWindow:ContextMenu" and "NativeWindow:ContextMenu:Result"

Comment 7

[Wesley Johnston (:wesj)]

Attachment: Patch

Whoops. Added NativeWindow.java.

I am a bit nervous about the message-return style passing here as well. In practice (on my S3), Gecko returns quickly enough that everything is seamless, but there is a race. For instance, if I do this during startup, before Gecko is ready, the menu items are still added (once gecko is started), but they appear "below the fold" and you have to scroll to see them. There's a flash of the scroll bar when they're added to indicate something happened, but that's it. I looked a bit for a way to tell Java to resize the menu, but didn't see anything.

Alternatively, we could store a list of the items in Java and use that fill things in. That makes things like "Show this action only on Google sites" harder to do (but we could still allow passing up a regexp or something...). Its also more book-keeping for addons as they might have to actively enable/disable even if the user never opens the context menu.

Comment 8

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 729151 [details] [diff] [review]
Patch

In general I like this approach. I think we need to on the way we make distinction between different native contextmenus. Some nits:
* NativeWindow.contextmenu.urlBar
* NativeUI("urlbar")

Comment 9

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 728380 [details] [diff] [review]
Patch - Add awesomebar screen

This one will need re-worked with the new Home page.

Instead of "AWESOMEBAR_ROW", I think we'd want:
* "historyitem", "bookmarkitem", etc...

Comment 10

[Wesley Johnston (:wesj)]

Attachment: Patch v2

Refreshed patch. This works NativeWindow.java a bit differently. Here NativeWindow just holds a HashMap mapping native ui elements to a hashtable of menuitems that can be shown. Syntax is something like:

var id = NativeWindow.contextmenus.add("Test", NativeWindow.contextmenus.urlbar, callback);
var id = NativeWindow.contextmenus.add("Test", NativeUI("URLBAR"), callback);
NativeWindow.contextmenus.remove(id);

Comment 11

[Wesley Johnston (:wesj)]

Attachment: Patch

Wrong patch.

Comment 12

[Wesley Johnston (:wesj)]

Attachment: Patch

Adds the ability to add some "states" to check before showing the item. i.e.

var id = NativeWindow.contextmenus.add("Test", NativeUI("URLBAR").addState("url", ".*google.*"), callback);
NativeWindow.contextmenus.remove(id);

will only show on pages that include the word "google" somewhere in their url. We could add more state functions for things in the awesomebar like "bookmarked" or in the tabs tray like "selected"?

Also, I should note that I named this NativeWindow because I'm hoping we can move more of our menu registration code in here and

1.) Unify all the paths we have to add menu items, bringing a uniform set of features across them. Context menus should be able to support checked items or submenus even?

2.) Fix up some of the application lifetime issues we have. Currently, items added to menus are stored in the menus themselves, which means they follow the Activity lifetime, not the Applications. Since these are stored on a static version of NativeWindow, they should follow the Application lifecycle.

Comment 13

[Wesley Johnston (:wesj)]

friendly review ping

Comment 14

[Mark Finkle (:mfinkle) (use needinfo?)]

My recent traumatic reaction to letting JS have too much control over the Java UI is making me want to avoid this set of patches.

1. Are we adding this for add-ons? I don't think we want to move our own UI from Java to JS. Java is where the explicit UI should be.
2. Is picking a specific part of the Java UI to attach the menu (using URLBAR, AWESOMEBAR_ITEM or HISTORY or BOOKMARK) too narrowly focused? Should we think about Actions.jsm where Actions could be for Page, History or Bookmarks, and then the Java side will display the actions when the context is right.

For some reason I am not getting a good feeling for this feature.

Comment 15

[Mark Finkle (:mfinkle) (use needinfo?)]

It also adds complexity to our contextmenu system, an already complex system.

Comment 16

[Wesley Johnston (:wesj)]

(In reply to Mark Finkle (:mfinkle) from comment #14)
> 1. Are we adding this for add-ons? I don't think we want to move our own UI
> from Java to JS. Java is where the explicit UI should be.
Yes, this is mostly for addons. Although, I don't mind if some of our actions are in js. Many of them don't apply until Gecko has been loaded anyway. Many of them just send a message to js to tell it to do something as soon as they're clicked.

> 2. Is picking a specific part of the Java UI to attach the menu (using
> URLBAR, AWESOMEBAR_ITEM or HISTORY or BOOKMARK) too narrowly focused? Should
> we think about Actions.jsm where Actions could be for Page, History or
> Bookmarks, and then the Java side will display the actions when the context
> is right.

I think I would rather that these drive context's if we're going to go that route. i.e. we already have this somewhat. "Open in new tab" applies to things that have uri's assoicated with them. "Pause" applies if we have media that's not playing. Ideally, we'd have one "Share" action that was queried and shown in situations where the object looks like something sharable. That makes more sense to me in our embedding context, where Page, History, or Bookmarks might not apply. And if Java can register "Actions" as well, we may not have to update 6 places every time we change share anymore...

We already partially have this system in place. i.e. context menu items in Gecko register themselves not on UI elements, but for what contexts they apply in. We could try to send that information to Java. So for instance, an image embedded in a link might have:

{
  type: "Action:Add",
  title: "Open in new tab",
  applies: {IMAGE: {}, URL: {OPENABLE}},
}

(playing with some sort of parent-child relationship here to simplify...) and then in Java places where we show context menus we could query:

MenuItem[] items = Actions.get(EnumSet(URL_OPENABLE));

That feel any better?

> For some reason I am not getting a good feeling for this feature.

I don't know what this means. Its been in pretty high demand from add-on authors.

Comment 17

[Wesley Johnston (:wesj)]

Comment on attachment 777633 [details] [diff] [review]
Patch

I don't know where this bug is going, but its not going there any time soon I guess. I have some different ideas about how to do this now based on registering "Actions" from JS that can show up in a number of different places....

Comment 18

[BMO Automation]

We have completed our launch of our new Firefox on Android. The development of the new versions use GitHub for issue tracking. If the bug report still reproduces in a current version of [Firefox on Android nightly](https://play.google.com/store/apps/details?id=org.mozilla.fenix) an issue can be reported at the [Fenix GitHub project](https://github.com/mozilla-mobile/fenix/). If you want to discuss your report please use [Mozilla's chat](https://wiki.mozilla.org/Matrix#Connect_to_Matrix) server https://chat.mozilla.org and join the [#fenix](https://chat.mozilla.org/#/room/#fenix:mozilla.org) channel.