695460 - Context menus

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect, P4)

Description

[Erin Lancaster [:elan]]

Content menus (copy/share text, links and images from a webpage)

    Chrome menus

Comment 1

[Erin Lancaster [:elan]]

1) do something about the database scheme
2) make sure that all sql happens on a non-main thread
3) clean up mobile components
4) move to a new directory

Comment 2

[Wesley Johnston (:wesj)]

I'm looking at the jetpack stuff to use it here on the js side. There is code in the prompt service patch to handle (my current) method of showing menus. I also have code to allow us to use native android longpress events.

Comment 3

[Wesley Johnston (:wesj)]

Attachment: WIP Patch

This is a WIP in case someone wants to pick this up. I've basically hooked up a GestureEvent listener in Android which gives us native long press events. When we get them in the child, we call into a ContextMenuHelper.

At that point I probably got a little too ambitious for a first round patch. I wrote this little guy that lets you register a context menu handler with a "selector" object. On longpress, your selector gets called and passed the element and can decide whether or not they can handle this. Then, we send the list of commands we have to Java where the menu is shown.

Java returns the index of the selected item, and we call its "callback" property.

That's all in there, but it wasn't all working last I checked. I think I saw the callbacks called, but they were failing. Other times the selectors weren't returning correctly. So.. there's some of debugging to do, and then writing a set of "default" context commands for Fennec.

Comment 4

[Wesley Johnston (:wesj)]

Attachment: Patch v1

This applies on top of my select stuff (bug 695485).

It adds an object NativeWindow.contextmenu which has add({}) and remove(aId) properties. It also adds a gesture listener on GeckoSurfaceView. Gesture:LongPress messages are picked up by the contextmenu object, which then dispatches them to content before showing our native system menu.

I added two commands here. One for "Open in new tab" and one for "Change Input Method". Each command can register a "context" attribute (from jetpack), which is just an object with a "matches" method. The matches method is passed the tapped element when a context menu is shown, and can return true if it should be shown.

If it is selected by the user, the comands "callback" method (made up by me, since jetpack uses fancy scripts-as-strings that can be passed across processes) will be called.

Background tabs aren't really supported by us yet, so "Open in new tab" is buggy.

Comment 5

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

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

>diff --git a/mobile/chrome/content/browser.js b/mobile/chrome/content/browser.js
>--- a/mobile/chrome/content/browser.js
>+++ b/mobile/chrome/content/browser.js
>@@ -483,6 +483,7 @@
>   init: function() {
>     Services.obs.addObserver(this, "Menu:Clicked", false);
>     Services.obs.addObserver(this, "Doorhanger:Reply", false);
>+    this.contextmenus.init();
>   },
> 
>   uninit: function() {
>@@ -1413,3 +1414,214 @@
>     }
>   }
> };
>+
>+
>+NativeWindow.contextmenus = {

Can we merge this up into the main object?

>+  items: [],

is this.menuitems the same as items?

also, changing this to an object might make lookup easier

>+  textContext: null,
>+  linkContext: null,
>+  _prevId: 0,

    _contextId: 0,

>+    this.add({ label: Strings.browser.GetStringFromName("contextmenu.openInNewTab"),
>+               context: this.linkContext,
>+               callback: function(aTarget) {
>+      let url = aTarget.getAttribute("href");
>+      BrowserApp.addTab(url);
>+    }});
>+
>+    this.add({ label: Strings.browser.GetStringFromName("contextmenu.changeInputMethod"),
>+               context: this.textInput,
>+               callback: function(aTarget) {
>+      Cc["@mozilla.org/imepicker;1"].getService(Ci.nsIIMEPicker).show();
>+    }});

Move these to a new ContextMenu object

>+  add: function(aOptions) {

>+    var item = {

let

>+      context: aOptions.context || this.PageContext("*"),

What is PageContext ?

"item" is a bit big. anything we don't need yet?

>+  remove: function(aId) {
>+    for (let i = 0; i < this.items.length; i++) {

This is easier if items is an object

>+  send: function(aX, aY) {
>+    let rootElement = elementFromPoint(aX, aY);
>+
>+    this.menuitems = [];

Different than this.items

>+  getItemForId: function(aId) {

Goes away if items is an object

>+  listContainsItem: function(aList, aItem) {

Goes away if aList is an object

Comment 6

[Wesley Johnston (:wesj)]

Attachment: Patch v2

I initially didn't use objects here (for the menuitems list at least) because the prompt service api I wrote takes an array. This works though, with a little hacking round the problem and looks cleaner IMO. So... thanks!

Also moved the SurfaceView stuff into its own file. And moved to using the ElementTouchHelper to find elements. And did some other little fixes cleanup...

Comment 7

[Wesley Johnston (:wesj)]

Attachment: Patch v2

And this is the correct patch...

Comment 8

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

Comment on attachment 571400 [details] [diff] [review]
Patch v2


>diff --git a/mobile/chrome/content/browser.js b/mobile/chrome/content/browser.js

>+  contextmenus: {

>+    init: function() {

>+      this.add({ label: Strings.browser.GetStringFromName("contextmenu.openInNewTab"),
>+                 context: this.linkContext,
>+                 callback: function(aTarget) {
>+        let url = NativeWindow.contextmenus._getLinkURL(aTarget);
>+        BrowserApp.addTab(url);
>+      }});
>+  
>+      this.add({ label: Strings.browser.GetStringFromName("contextmenu.changeInputMethod"),
>+                 context: this.textContext,
>+                 callback: function(aTarget) {
>+        Cc["@mozilla.org/imepicker;1"].getService(Ci.nsIIMEPicker).show();
>+      }});

I think we could move these to a new JS class as a followup. Maybe even add "Save As PDF" this way?

>+    uninit: function() {
>+      Services.obs.removeObserver(this, "Gesture:LongPress");
>+    },

Do you call this method in the NativeWindow.uninit ?

>+    add: function(aOptions) {

Question: Should we make the menu. doorhanger and contextmenu "add" and "remove" methods similar? That would mean using params or options objects consistently

>+      var item = {

let

>+        label: aOptions.label || "",
>+        items: aOptions.items || [],
>+        context: aOptions.context || this.PageContext("*"),

PageContext is not defined. Remove it


>+    sendToContent: function(aX, aY) {

_sendToContent

>+    show: function(aEvent) {

_show

These are not for public, right?

r-, just so we discuss and consider the API consistency. otherwise I would r+

Comment 9

[Wesley Johnston (:wesj)]

Hmm... I think we're fine going that way. When I initially started writing this I was trying to follow jetpack conventions, hoping to help out those guys. Later I decided that was silly, because they'd likely have to put a compatibility layer on top anyway. I just noticed that item.items is also still hanging around because jetpack supports submenus.

menu is the only guy doing add and remove and they use:
add: function(aName, aIcon, aCallback) { ... }
remove: function(aId) { ... }

I'll implement:
add(aName, aContext, aCallback) { ... }
remove(aId) { ... }

I'm not a huge fan of the name "context" either. I think I'll change it to something like "selector"?

This looks strangely like our old api all of the sudden. Heh.

Comment 10

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

(In reply to Wesley Johnston (:wesj) from comment #9)

> I'll implement:
> add(aName, aContext, aCallback) { ... }
> remove(aId) { ... }
> 
> I'm not a huge fan of the name "context" either. I think I'll change it to
> something like "selector"?

add(aName, aSelector, aCallback) { ... }

sounds good. less is more to start

Comment 11

[Wesley Johnston (:wesj)]

Attachment: Patch v3

Updated to new API.

Comment 12

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

Comment on attachment 571668 [details] [diff] [review]
Patch v3

File a bug for making a contextcommands.js file for the built-in menus.

Comment 13

[Wesley Johnston (:wesj)]

Pushed with wrong bug number:
http://hg.mozilla.org/projects/birch/rev/40e3d6b1122d

Comment 15

[Aaron Train [:aaronmt]]

(In reply to Mark Finkle (:mfinkle) from comment #12)
> Comment on attachment 571668 [details] [diff] [review] [diff] [details] [review]
> Patch v3
> 
> File a bug for making a contextcommands.js file for the built-in menus.

Couldn't find one, but I filed bug 700283 for images - feel free to morph or dupe when a general contextcommands.js bug is filed.

Comment 16

[Aaron Train [:aaronmt]]

20111107040337
http://hg.mozilla.org/projects/birch/rev/22f16ec4052a
Samsung Nexus S (2.3.6)

Comment 17

[Wesley Johnston (:wesj)]

contextcommands.js is actually bug 699537.

Comment 18

[Wesley Johnston (:wesj)]

These patches were backed while investigating Talos failures.  Now that tests are green again, we will need to reland.

Comment 19

[Brad Lassey [:blassey] (use needinfo?)]

backout was backed out https://hg.mozilla.org/projects/birch/rev/6f925b45a547

Comment 20

[Aaron Train [:aaronmt]]

20111114041052
http://hg.mozilla.org/projects/birch/rev/859ecdfe0168
Samsung Galaxy SII (Android 2.3.4)

Comment 21

[Andreea Pod]

I see no share on links and images from a webpage. Copy appears only in text boxes after you type some text. 

I'm trying to write a testcase but I'm not sure which options should appear in context menu for images/links/text boxes. Can somebody help me?

Comment 22

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

(In reply to Andreea Pod from comment #21)
> I see no share on links and images from a webpage. Copy appears only in text
> boxes after you type some text. 
> 
> I'm trying to write a testcase but I'm not sure which options should appear
> in context menu for images/links/text boxes. Can somebody help me?

You should assume the current context menu is _the_ design:
* No share links
* No web content copy (only for text boxes)

New bugs will be filed for any changes to the context menu.

Comment 23

[Andreea Pod]

Existent testcases: 
- https://litmus.mozilla.org/show_test.cgi?id=43151
- https://litmus.mozilla.org/show_test.cgi?id=43213

New testcases for changes will be found at context menu subgroup for every test run.