If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Add PredicateContext class to let an addon function determine menu item visibility

RESOLVED FIXED

Status

Add-on SDK
General
P2
enhancement
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: peter.liljenberg, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

46 bytes, text/x-github-pull-request
mossop
: review+
Details | Review | Splinter Review
(Reporter)

Description

4 years ago
Currently (SDK 1.14) it is only possible to decide if a context menu item should appear either by a content script, or by using one of a set of predefined FooContext classes in sdk/context-menu.

I want to enable a context menu item only when the clipboard contain specific data types, but that I can only do from the add-on, not from a content script (as far as my experimentation has shown).  I'd like to define my own Context class in main.js, e.g. like this:

    var ImageClipboardContext = Class({
	extends: contextMenu.Context,
	
	isCurrent: function isCurrent(popupNode) {
	    return hasClipboardImage();
	},
    });

(This particular example calls a method that uses nsIClipboard.hasDataMatchingFlavors.)


It would be easy to support this in the SDK, just export Context from sdk/context-menu:

--- lib/sdk/context-menu.js~	2013-03-25 16:00:00.000000000 +0100
+++ lib/sdk/context-menu.js	2013-09-23 16:54:47.850087432 +0200
@@ -96,6 +96,9 @@
   }
 });
 
+// PATCH: let addons implement their own context class
+exports.Context = Context;
+
 // Matches when the context-clicked node doesn't have any of
 // NON_PAGE_CONTEXT_ELTS in its ancestors
 let PageContext = Class({


This works nicely, but was (understandably, since it was a patched SDK) refused in the addon review.  Are there any API reasons to restrict addons to not define their own context classes, or is it just that no-one has had the need before?
(Reporter)

Comment 1

4 years ago
Alternative request: a PredicateContext(func) class would do just as fine, if there are concerns with exposing popupNode to the addon.
(Reporter)

Comment 2

4 years ago
Of course, I'm happy to code this and contribute it, but would appreciate a nod from the SDK gurus on what approach is preferred.
Dave, Irakli... what do you think of this?

Needinfo-ing you two instead of waiting for triage since that's not happening for a week or more.
Flags: needinfo?(rFobic)
Flags: needinfo?(dtownsend+bugmail)
(In reply to peter.liljenberg from comment #1)
> Alternative request: a PredicateContext(func) class would do just as fine,
> if there are concerns with exposing popupNode to the addon.

Yeah this. Exposing the node is not e10s safe which we probably still need to think about unfortunately.
Flags: needinfo?(dtownsend+bugmail)
(In reply to Dave Townsend (:Mossop) from comment #4)
> (In reply to peter.liljenberg from comment #1)
> > Alternative request: a PredicateContext(func) class would do just as fine,
> > if there are concerns with exposing popupNode to the addon.
> 
> Yeah this. Exposing the node is not e10s safe which we probably still need
> to think about unfortunately.

+1, we were actually talking about this before it would be great if we could extend
`ContextMenu` API to support:

Item({
  label: "body context",
  // If predicate returns true context menu item is present.
  context: function(data) {
    data.documentType
    data.documentURL
    data.targetName     // element name
    data.targetID       // element id if has one
    data.isEditable     // A flag indicating whether the element is editable (text input, textarea, etc.). 
    data.selectionText  // The text for the context selection, if any. 
    data.srcURL         // Will be present for elements with a 'src' URL. 
    data.linkURL        // If the element is a link, the URL it points to. 
    data.value          // If the element is input, this it's value
  },
  // On click is provided with a same data info
  onClick: function(data) {
  }
});
Flags: needinfo?(rFobic)

Updated

4 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: -- → P2
(In reply to peter.liljenberg from comment #2)
> Of course, I'm happy to code this and contribute it, but would appreciate a
> nod from the SDK gurus on what approach is preferred.

I think Dave is willing and able to help you out with working on this.
Flags: needinfo?(dtownsend+bugmail)
(Reporter)

Comment 7

4 years ago
I already got to this story in our company sprint, so I started on it.  Here's a basic PredicateContext:
https://github.com/commonsmachinery/addon-sdk/commit/c7123e824b7cb2e87816e7582ff991b7ca4559b0

It seemed a large change to just allow plain functions, given the class system and checking in place, and I'm not sure if that was what was suggested.  Most of the data fields are filled in, but I wasn't sure about what data.documentType should be, and didn't dig into getting the selectionText.

There's unit tests, documentation etc left, but if you think this is the right track I can have a go at that too.
(Reporter)

Comment 8

4 years ago
Unit tests and documentation written.  I figured that even if PredicateContext itself wouldn't be used in favour of plain functions, the data population and unit tests would still be useful.

The code is over in this branch:
https://github.com/commonsmachinery/addon-sdk/tree/922558

Of the fields in comment #5, documentType is not set since I'm not sure what was intended. (Should it be the doctype name, if any?)

Dave, do you want to change something before I submit a pull request?
(In reply to peter.liljenberg from comment #8)
> Unit tests and documentation written.  I figured that even if
> PredicateContext itself wouldn't be used in favour of plain functions, the
> data population and unit tests would still be useful.
> 
> The code is over in this branch:
> https://github.com/commonsmachinery/addon-sdk/tree/922558
> 
> Of the fields in comment #5, documentType is not set since I'm not sure what
> was intended. (Should it be the doctype name, if any?)
> 
> Dave, do you want to change something before I submit a pull request?

Awesome work, put it into a pull request and we'll do the review there.
Flags: needinfo?(dtownsend+bugmail)
(Reporter)

Updated

4 years ago
Summary: Allow addons to define custom contextMenu.Context classes → Add PredicateContext class to let an addon function determine menu item visibility
Blocks: 936189
Created attachment 8334645 [details] [review]
pull request
Attachment #8334645 - Flags: review?(dtownsend+bugmail)
Comment on attachment 8334645 [details] [review]
pull request

Waiting on responses in the pull request here.
Attachment #8334645 - Flags: review?(dtownsend+bugmail)
(Reporter)

Comment 12

4 years ago
Apologies for the delay, but now I had time to address the comments.  Pull request updated with additional patches.

Not sure what to do about the NS_ERROR_FAILURE I had to work around, though.  See comment here:
https://github.com/commonsmachinery/addon-sdk/commit/16183967db08e661dde95890b66d0eb0f8978f68
Attachment #8334645 - Flags: review?(dtownsend+bugmail)

Comment 13

4 years ago
Commit pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/8f80dfcf09621dc13ae6b4fc2214378fa7fe7ead
Bug 922558: Add contextMenu.PredicateContext. r=Mossop
Comment on attachment 8334645 [details] [review]
pull request

Thanks and sorry for the delays here, been fighting colds and vacations for the past few weeks.
Attachment #8334645 - Flags: review?(dtownsend+bugmail) → review+

Comment 15

4 years ago
Commit pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/dc6000a8442e882400c099a0153b5cb9ea6a537e
Revert "Bug 922558: Add contextMenu.PredicateContext. r=Mossop"

This reverts commit 8f80dfcf09621dc13ae6b4fc2214378fa7fe7ead.
Unfortunately I had to back this out due to test failures on Windows. See https://tbpl.mozilla.org/?tree=Jetpack&rev=5133e7cc3775
(Reporter)

Comment 17

4 years ago
So, selected text in an input text field on Windows is not included in the data object when running the unittest testPredicateContextSelectionInTextBox.  However, the addon code below using the feature _does_ work for me in a release-built Windows Firefox:

var contextMenu = require('sdk/context-menu');

var test = contextMenu.Item({
    label: 'Test selection',
    data: 'unset',
    
    context: contextMenu.PredicateContext(function(data) {
        console.log('selectionText: ' + data.selectionText);
        test.data = data.selectionText;
        return true;
    }),

    contentScript: ('self.on("click", function(node, data) {' +
		    '  alert("data: " + data); });')
});

Opening a web page with a plain input text field, entering some text and marking a part of it, then right-clicking and choosing "Test selection" shows the selected text in the alert box.  Right-clicking outside the input unmarks the selection, and shows null.

Unfortunately I don't have any windows environment right now where I can run the test suite myself (for lack of python and capability to install it), so I could only test this with the addon above.

It's a bit cheeky suggesting it, but might it be a problem with the way the context menu is triggered in the unit tests?  I.e., the trigger code below might result in the Windows implementation of the text input field interpreting it as a regular mouse click and dropping the selection:

        sendMouseEvent("contextmenu",
                       rect.left + (rect.width / 2),
                       rect.top + (rect.height / 2),
                       2, 1, 0);

Before adding the PredicateContext test cases, there wasn't any context menu test cases involving text input fields, so it may not have been noticed before.

So the code seems to work on Windows, but the unit test doesn't, and I don't have the necessary environment at hand to dig into it.  Any ideas on how we can proceed?
(Reporter)

Comment 18

4 years ago
FWIW, I did an experiment where I forced the firefox window running the test suite to have a height of only about 200 pixels, so most of the test page is not visible (on Linux, pretty easy with i3wm).  This causes the tests to freeze up, but they can be pushed forward by moving the pointer in and out of the window.  This all results in lots of exceptions and failed tests.

That probably doesn't happen in the automated windows build here since only one specific test fails, but it could still be an issue to address.
(In reply to peter.liljenberg from comment #17)
> It's a bit cheeky suggesting it, but might it be a problem with the way the
> context menu is triggered in the unit tests?  I.e., the trigger code below
> might result in the Windows implementation of the text input field
> interpreting it as a regular mouse click and dropping the selection:
> 
>         sendMouseEvent("contextmenu",
>                        rect.left + (rect.width / 2),
>                        rect.top + (rect.height / 2),
>                        2, 1, 0);
> 
> Before adding the PredicateContext test cases, there wasn't any context menu
> test cases involving text input fields, so it may not have been noticed
> before.

What node is that event getting sent to. If it's not the input element then that could well be a problem, changing that might be the first thing to do.

> So the code seems to work on Windows, but the unit test doesn't, and I don't
> have the necessary environment at hand to dig into it.  Any ideas on how we
> can proceed?

I'll see if one of the engineers has windows available to test this out and figure out what is going wrong. I will have access early next week so I can look then if someone else can't sooner.

(In reply to peter.liljenberg from comment #18)
> FWIW, I did an experiment where I forced the firefox window running the test
> suite to have a height of only about 200 pixels, so most of the test page is
> not visible (on Linux, pretty easy with i3wm).  This causes the tests to
> freeze up, but they can be pushed forward by moving the pointer in and out
> of the window.  This all results in lots of exceptions and failed tests.
> 
> That probably doesn't happen in the automated windows build here since only
> one specific test fails, but it could still be an issue to address.

Window size is always going to play a factor in tests, probably not a great deal to be done about that.
Flags: needinfo?(dtownsend+bugmail)
(Reporter)

Comment 20

4 years ago
(In reply to Dave Townsend (:Mossop) from comment #19)
> What node is that event getting sent to. If it's not the input element then
> that could well be a problem, changing that might be the first thing to do.

I added checks for that as well as a test for selection in textfields in the bug branch:
https://github.com/commonsmachinery/addon-sdk/commit/7d60bf9e01516b244ed12af0ffed82c56b6e7506

On Linux it is the textbox or textfield that is focused for the event.

> I'll see if one of the engineers has windows available to test this out and
> figure out what is going wrong. I will have access early next week so I can
> look then if someone else can't sooner.

Thanks!  I'll try to get access to a test environment on my side too.

Comment 21

4 years ago
Commit pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/151df2b01655a9073bdcd315dce8b4dfb0b2a4dd
Bug 922558: Add contextMenu.PredicateContext. r=Mossop
For some reason Windows cares about whether the textbox is focused or not. I added a call to focus the textbox and the problems went away.

We've since removed the documentation from github, can you make your documentation changes here: https://developer.mozilla.org/en-US/Add-ons/SDK/High-Level_APIs/context-menu
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Flags: needinfo?(dtownsend+bugmail)
Resolution: --- → FIXED
(Reporter)

Comment 23

4 years ago
Thanks Dave!  

I'll add the documentation over on that site.  I just wonder if it should somehow be indicated that this API will be available starting with 1.16 (which I presume is the next version)?
(In reply to peter.liljenberg from comment #23)
> Thanks Dave!  
> 
> I'll add the documentation over on that site.  I just wonder if it should
> somehow be indicated that this API will be available starting with 1.16
> (which I presume is the next version)?

Will, can you answer Peter's question?
Flags: needinfo?(wbamberg)
(In reply to peter.liljenberg from comment #23)
> Thanks Dave!  
> 
> I'll add the documentation over on that site.  I just wonder if it should
> somehow be indicated that this API will be available starting with 1.16
> (which I presume is the next version)?

This new feature will be released in Firefox 29, and yes when we document it we will annotate the documentation with the range of Firefox versions it is compatible with. 

FYI going forward we will always introduce new apis in Firefox itself, and we will only release new versions of the SDK ( eg a possible SDK 1.16 ) if we need to make changes to the command-line tools.
Peter: thanks for offering to document this! 

As Jeff says, SDK APIs ship in Firefox now, so they are tracked by Firefox versions.

Here's how I indicate in MDN that a particular thing is new in some recent version of Firefox: I insert a little note in the "callout" style in the MDN editor (https://developer.mozilla.org/en-US/docs/Project:MDN/Contributing/Editor_guide/Editing#Formatting_styles). For example: https://developer.mozilla.org/en-US/docs/Tools/Debugger#Pretty-print_a_minified_file.

For API documentation, please use the style documented here: https://wiki.mozilla.org/Jetpack/SDK/Writing_Documentation, so we can make sure the docs stay consistent.
Flags: needinfo?(wbamberg)
(Reporter)

Comment 27

4 years ago
Oh, I had already written the documentation in the original patch, I just needed a pointer on how to add it in the new system. :)  And that is now done: https://developer.mozilla.org/en-US/Add-ons/SDK/High-Level_APIs/context-menu$compare?to=509897&from=509099

Comment 28

4 years ago
hi Peter, is this feature already implemented in Mozilla Firefox 26.0 public release? I am trying to use it but it is not working. The documents tag this feature as NEW IN FIREFOX 29 (but it is not released). 

Anyway, this simple code fails for me, but it might be because the feature is to be released only in Firefox 29:

var cm = require("sdk/context-menu");
cm.Item({
  label: "PREDICATE CONTEXT TEST",
  data: 'unset',
  context: cm.PredicateContext(function(data) {
        console.log('--------------------> selectionText: ' + data.selectionText);
		test.data = data.selectionText;
		return true;
    }),
   contentScript: ('self.on("click", function(node, data) {' +
		    '  alert("data: " + data); });')
});


thanks and regards
Flags: needinfo?(peter.liljenberg)
(Reporter)

Comment 29

4 years ago
(In reply to Walter from comment #28)

I'm not that close to the Firefox release planning, so I just know what Jeff said above about it being in the stream for 29.  Version 26 definitely doesn't have it, though.
Flags: needinfo?(peter.liljenberg)

Comment 30

3 years ago
Hello,

I am having a Problem with linkURL. linkURL is only set when an 'A'-node is clicked directly. If the node is for example an 'IMG'-node whose parent node is a 'A'-node the linkURL will not be set (compare context-menu.js#L262), but I think it should be. A simple fix would be something like that: 

data.linkURL = node.href || node.parentNode.href || null;

Although this doesn't fix it for all situations. Maybe checking all parentNodes for a href-attribute would be a better solution.

What do you think? Any better ideas or objections?

Regards
Ben

Comment 31

3 years ago
Hello,

so I thought about it again and really think checking all parentNodes is the way to go. So here is a quick (untested) implementation:

  let currentNode = node;
  while (!data.linkURL && !(currentNode instanceof Ci.nsIDOMDocument)) {
    data.linkURL = currentNode.href || null;
    currentNode = currentNode.parentNode;
  }

Anyone with an opinion? Should I file a new bug report for this?

Regards
Ben
hi Ben,

your problem doesn't look like it is the same as the original issue in this bug, so it's probably better to file a separate bug.

Comment 33

3 years ago
Separate bug report filed: https://bugzilla.mozilla.org/show_bug.cgi?id=1010616
You need to log in before you can comment on or make changes to this bug.