Bug 828113 Support accesskey (also on overflow menu), correct outdated comment, fix... #723

Merged
merged 2 commits into from Sep 11, 2014

Projects

None yet

4 participants

@brettz9
Contributor
brettz9 commented Jan 12, 2013

Code fix: Fix context menus to allow key activation to activate the "click" event (i.e., use "command" event). I preserved the naming of "click" for content scripts so they could continue to work with "click" events.
Comment fix: Correct outdated comment (Separator is also part of baseItemRules)
New Feature: Support accesskey on context-menus (also on overflow menu)

For changes I did NOT make:

  1. I did not incorporate accesskey along with label in toString() descriptions though perhaps that would be good.
  2. I did not change the context listener to allow users to dynamically provide an accesskey in their "context" event handler, though perhaps these user events could allow return of an array, with one as the label, another as the accesskey (instead of just a string label, as I understand it is now).
@brettz9 brettz9 Support accesskey (also on overflow menu), correct outdated comment, …
…fix context menus to allow key activation to activate the "click" event
59fa075
Contributor
brettz9 commented Jan 12, 2013

I also did not add Docs. I'd be happy to update docs too, but thought it may be premature without a review that might change things anyways.

@Mossop Mossop commented on the diff Jan 14, 2013
lib/sdk/context-menu.js
@@ -300,8 +309,8 @@ let ContextWorker = Worker.compose({
// that is either a string or a value that evaluates to true. If all of the
// listeners returned false then returns false. If there are no listeners
// then returns null.
- getMatchedContext: function getCurrentContexts(popupNode) {
- let results = this._contentWorker.emitSync("context", popupNode);
+ getMatchedContext: function getCurrentContexts(popupNode, data) {
+ let results = this._contentWorker.emitSync("context", popupNode, data);
Mossop
Mossop Jan 14, 2013 Member

This change seems unrelated, was it included by accident?

brettz9
brettz9 Jan 15, 2013 Contributor

You are right, that change is unrelated and got added to the branch by accident. Github didn't seem to work for me with committing after a hard branch reset, so do you want me to just undo the changes and commit, start a new pull request, or keep it as is? This change (and the whole commit) is meant to provide the "context" event with access to the same data property available in the "click" event.

brettz9
brettz9 Jan 15, 2013 Contributor

My use case for exposing the data property to the "context" event, by the way, was to expose locale info and user preferences to the content script so that it could decide whether to execute immediately rather than wait for the context menu to open (and then use that data). It might also be useful to supply preferences which could alter the context event function return result.

Mossop
Mossop May 14, 2013 Member

Sorry this got dropped. Yeah if you could separate out the individual changes here that would be very useful. All of it looks ok but I'd rather see them land individually to keep history clean.

Member
Mossop commented Jan 14, 2013

Looks good to me it would be really good to add a test though. @gozala do you need to see this API change? Just adds an accesskey property.

Contributor
brettz9 commented Jan 15, 2013

In addition to the accesskey property (and the accidental commit adding the "data" property I just mentioned), it also allows context menus to be activated by key commands as well (e.g., if you right-click to open a context menu then use the arrow keys and hit return to select a menu item) as previously, with the SDK, context menu items suffer from the problem that they can only be activated by a mouse click.

Contributor
brettz9 commented Jan 15, 2013

Not feeling too well with a cold now, but I can look to provide a test later if needed as well as documentation.

Contributor
brettz9 commented Apr 27, 2013

Sorry, my hands are full, so not sure when I may be able to getting to a test

Contributor
brettz9 commented Jul 17, 2013

I worked on an updated commit (though I would still need to doc and add tests) at brettz9@3428337 but when I tried to test it with cfx -o run, no context menus show up for some reason (It would be very helpful if you had a cfx -o xpi too, by the way, as I am getting no debugging feedback with run).

Member
Mossop commented Jul 18, 2013

cfx --force-use-bundled-sdk xpi should help

Contributor
brettz9 commented Jul 18, 2013

Cool, thanks! (Just a note though that my add-on failed silently until I realized that the force-bundled XPI should be run against Nightly instead of Firefox 22--not sure if better error reporting could have worked here).

Should I make a pull request to add mention of that flag to the docs?

Also, while I saw docs on running tests on a module, how do I run the tests on the SDK itself? Running the SDK's own test command against itself is not working and I need to know how to use this if I will add my own tests to this commit.

Finally, sorry for the basic question, but what is the protocol on updating outdated commits without opening a new branch/pull request? Resetting the tree works locally but not remotely, so I don't see how to easily discard my earlier commit on this branch in favor of the latest branch...

Member

To run the test on the SDK itself, be sure to specify the Nightly binary (as some features in the SDK may not work against newer versions), use the local SDK rather than the one built in as well:

cfx test -o -b /path/to/nightly -v will test all the tests in the ./test directory

cfx testall -o -b /path/to/nightly -v will test all the tests, as well as the cfx module itself, and full add-on tests

To update outdated commits, force push to your branch that the PR is bound to, like git push brettz9 +context-menu-accesskey

Contributor
brettz9 commented Jul 18, 2013

Thanks very much. I will see about adding some tests.

Contributor
brettz9 commented Jul 19, 2013

Sorry, but I am still getting a duplicate package error...I am using this command:

cfx test -o -b "C:\Program Files (x86)\Nightly" -v

I get the error if I call activate.bat from the 1.14 version of the SDK I have downloaded and then change the directory in the resulting command window to the Git version of the SDK. The same error occurs when I instead call activate.bat on the Git version for the 1.14 copy (or try running the same version of either while changing the command window working directory to that same version's directory).

Contributor
brettz9 commented Jul 18, 2014

This still doesn't work for me with SDK 1.16, on Windows, and Python 2.7.8. Could someone just look at the old PR and apply? It is pretty straightforward.... This is holding me back from upgrading my non-restartless Copy Link Text add-on...

Member

Can you rebase this against current master? Doesn't apply successfully (most likely since many things have changed since then)

@erikvold erikvold merged commit 28aec6c into mozilla:master Sep 11, 2014

1 check failed

default The Travis build failed
Details
Contributor
brettz9 commented Sep 12, 2014

Much appreciation and apologies for my dropping off here...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment