Status

()

Firefox for Android
General
P4
normal
VERIFIED FIXED
6 years ago
a year ago

People

(Reporter: elan, Assigned: wesj)

Tracking

(Blocks: 1 bug)

unspecified
ARM
Android
Points:
---
Dependency tree / graph
Bug Flags:
in-litmus +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

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

    Chrome menus
(Reporter)

Comment 1

6 years ago
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
(Assignee)

Updated

6 years ago
Assignee: nobody → wjohnston
(Assignee)

Comment 2

6 years ago
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.
Priority: P1 → P4

Updated

6 years ago
Blocks: 697265
(Assignee)

Comment 3

6 years ago
Created attachment 569524 [details] [diff] [review]
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.

Updated

6 years ago
Blocks: 697807
(Assignee)

Comment 4

6 years ago
Created attachment 570118 [details] [diff] [review]
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.
Attachment #569524 - Attachment is obsolete: true
Attachment #570118 - Flags: review?(mark.finkle)

Updated

6 years ago
Blocks: 698836
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
Attachment #570118 - Flags: review?(mark.finkle) → review-
(Assignee)

Comment 6

6 years ago
Created attachment 571398 [details] [diff] [review]
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...
Attachment #570118 - Attachment is obsolete: true
Attachment #571398 - Flags: review?(mark.finkle)
(Assignee)

Comment 7

6 years ago
Created attachment 571400 [details] [diff] [review]
Patch v2

And this is the correct patch...
Attachment #571398 - Attachment is obsolete: true
Attachment #571398 - Flags: review?(mark.finkle)
Attachment #571400 - Flags: review?(mark.finkle)
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+
Attachment #571400 - Flags: review?(mark.finkle) → review-
(Assignee)

Comment 9

6 years ago
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
(Assignee)

Comment 11

6 years ago
Created attachment 571668 [details] [diff] [review]
Patch v3

Updated to new API.
Attachment #571400 - Attachment is obsolete: true
Attachment #571668 - Flags: review?(mark.finkle)
Comment on attachment 571668 [details] [diff] [review]
Patch v3

File a bug for making a contextcommands.js file for the built-in menus.
Attachment #571668 - Flags: review?(mark.finkle) → review+
(Assignee)

Comment 13

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

Updated

6 years ago
Duplicate of this bug: 699717
(Assignee)

Updated

6 years ago
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED

Updated

6 years ago
Depends on: 700283
(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)
Status: RESOLVED → VERIFIED
(Assignee)

Comment 17

6 years ago
contextcommands.js is actually bug 699537.
Blocks: 699537
(Assignee)

Comment 18

6 years ago
These patches were backed while investigating Talos failures.  Now that tests are green again, we will need to reland.
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
backout was backed out https://hg.mozilla.org/projects/birch/rev/6f925b45a547
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED
20111114041052
http://hg.mozilla.org/projects/birch/rev/859ecdfe0168
Samsung Galaxy SII (Android 2.3.4)
Status: RESOLVED → VERIFIED
Flags: in-litmus?(fennec)
Whiteboard: [QA+]

Comment 21

6 years ago
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.

Comment 23

6 years ago
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.
Flags: in-litmus?(fennec) → in-litmus+
Whiteboard: [QA+]
You need to log in before you can comment on or make changes to this bug.