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

Status: RESOLVED FIXED

(Add-on SDK Graveyard :: General, enhancement, P2)

Description

[peter.liljenberg]

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?

Comment 1

[peter.liljenberg]

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

Comment 2

[peter.liljenberg]

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.

Comment 3

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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.

Comment 4

[Dave Townsend [:mossop]]

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

Comment 5

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

(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) {
  }
});

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 7

[peter.liljenberg]

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.

Comment 8

[peter.liljenberg]

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?

Comment 9

[Dave Townsend [:mossop]]

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

Comment 10

[Dave Townsend [:mossop]]

Attachment: pull request

Comment 11

[Dave Townsend [:mossop]]

Comment on attachment 8334645 [details] [review]
pull request

Waiting on responses in the pull request here.

Comment 12

[peter.liljenberg]

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

Comment 13

[[github robot]]

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 14

[Dave Townsend [: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.

Comment 15

[[github robot]]

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.

Comment 16

[Dave Townsend [:mossop]]

Unfortunately I had to back this out due to test failures on Windows. See https://tbpl.mozilla.org/?tree=Jetpack&rev=5133e7cc3775

Comment 17

[peter.liljenberg]

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?

Comment 18

[peter.liljenberg]

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.

Comment 19

[Dave Townsend [:mossop]]

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

Comment 20

[peter.liljenberg]

(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

[[github robot]]

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

Comment 22

[Dave Townsend [: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

Comment 23

[peter.liljenberg]

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

Comment 24

[Dave Townsend [:mossop]]

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

Comment 25

[Jeff Griffiths (:canuckistani) (:⚡︎)]

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

Comment 26

[Will Bamberg [:wbamberg]]

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.

Comment 27

[peter.liljenberg]

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

[Walter]

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

Comment 29

[peter.liljenberg]

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

Comment 30

[Ben]

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

[Ben]

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

Comment 32

[Tomislav Jovanovic :zombie]

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

[Ben]

Separate bug report filed: https://bugzilla.mozilla.org/show_bug.cgi?id=1010616