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

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

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.

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.

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 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

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...

And this is the correct patch...

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+

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.

(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

Updated to new API.

Comment on attachment 571668 [details] [diff] [review] Patch v3 File a bug for making a contextcommands.js file for the built-in menus.

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

(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.

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

contextcommands.js is actually bug 699537.

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

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

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

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?

(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.

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.