Last Comment Bug 836318 - Menu does not work with SelectorContext
: Menu does not work with SelectorContext
Status: RESOLVED FIXED
:
Product: Add-on SDK
Classification: Client Software
Component: General (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: ---
Assigned To: Dave Townsend [:mossop]
:
:
Mentors:
Depends on:
Blocks: 836564 836613
  Show dependency treegraph
 
Reported: 2013-01-30 08:28 PST by Jeff Griffiths (:canuckistani) (:⚡︎)
Modified: 2013-01-30 20:20 PST (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/751/files (373 bytes, text/html)
2013-01-30 15:40 PST, Dave Townsend [:mossop]
evold: review+
Details

Description Jeff Griffiths (:canuckistani) (:⚡︎) 2013-01-30 08:28:06 PST
From this message:

https://groups.google.com/d/msg/mozilla-labs-jetpack/Dqn9KWncpsU/U3fKdxF4ldEJ

There seems to be a problem with the context-menu module. The .Menu constructor is ignoring the context option - even when I set the context to .PageContext(). When I create a top-level menu with two items and set its context, the menu doesn't show up at all. However, when I put both items on the top-level, without the menu, the context set on them individually works ok for them. I thought maybe it's something with my code, but this example from the documentation doesn't work either:

var cm = require("sdk/context-menu");
var googleItem = cm.Item({
  label: "Google",
  data: "http://www.google.com/search?q="
});
var wikipediaItem = cm.Item({
  label: "Wikipedia",
  data: "http://en.wikipedia.org/wiki/Special:Search?search="
});
var searchMenu = cm.Menu({
  label: "Search With",
  context: cm.SelectorContext("a[href]"),
  contentScript: 'self.on("click", function (node, data) {' +
                 '  var searchURL = data + node.textContent;' +
                 '  window.location.href = searchURL;' +
                 '});',
  items: [googleItem, wikipediaItem]
});
Comment 1 Jeff Griffiths (:canuckistani) (:⚡︎) 2013-01-30 08:32:33 PST
Repro example:

https://builder.addons.mozilla.org/package/171931/latest/

The first case does not work, the second does.
Comment 2 Dave Townsend [:mossop] 2013-01-30 09:34:57 PST
This is caused by two changes that were made to the behaviour of the context-menu module.

First items that are children of menus still support the context property and without one they default to the PageContext. So when context clicking on a link the Google and Wikipedia items get hidden.

Second if a menu contains no visible item it gets hidden, so the menu gets hidden in this example.
Comment 3 Dave Townsend [:mossop] 2013-01-30 13:34:59 PST
We've seen a few comments about this now so I'm wondering if we should take a change to make it more compatible. If the default for child menu items was visible rather than using the PageContext then add-ons would behave as they did previously but you could still use a context on them when you wanted.
Comment 4 Jeff Griffiths (:canuckistani) (:⚡︎) 2013-01-30 14:22:20 PST
Where are the other comments? I've only seen the one google group thread.
Comment 5 Dave Townsend [:mossop] 2013-01-30 14:31:39 PST
(In reply to Jeff Griffiths (:canuckistani) from comment #4)
> Where are the other comments? I've only seen the one google group thread.

Mardeg came in to IRC with the same issue and I can only imagine that many examples of existing context-menu usage would be affected by this which makes me worred about automatic repacks.

The reason I added support for contexts in inner-menu items was because it seemed to be useful and at the time I couldn't see a reason not to support it. I still think it is useful but I think if we default to the previous behaviour we still get the usefulness but don't have as much of a compatibility problem.
Comment 6 izydor 2013-01-30 14:54:32 PST
(In reply to Dave Townsend (:Mossop) from comment #5)
> The reason I added support for contexts in inner-menu items was because it
> seemed to be useful and at the time I couldn't see a reason not to support
> it. I still think it is useful but I think if we default to the previous
> behaviour we still get the usefulness but don't have as much of a
> compatibility problem.

I think that's a great idea, Dave, especially that this change affects any addon with a top-level context-menu created by the .Menu() constructor. Given that up to SDK 1.12 the context option in sub-items was specifically ignored, most developers (probably) ignored it as well - I know I did, and the context menu disappeared in my addon after repacking with 1.13 without any error indication. So defaulting to the previous behaviour is probably the best option here.
Comment 7 Dave Townsend [:mossop] 2013-01-30 15:40:05 PST
Created attachment 708369 [details]
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/751/files

Pointer to Github pull-request
Comment 8 [github robot] 2013-01-30 20:06:49 PST
Commits pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/926d4fce9cda8c095028e6d9324b7dcc257f75bb
Bug 836318: Items in submenus default to visible rather than using the PageContext.

https://github.com/mozilla/addon-sdk/commit/8ecc5decb9cd85c8c8eebebbb1c7fba15b039930
Merge pull request #751 from Mossop/bug836318

Bug 836318: Items in submenus default to visible rather than using the PageContext r=@erikvold
Comment 9 [github robot] 2013-01-30 20:20:33 PST
Commit pushed to stabilization at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/0651ece3d98bfdf6307f3ed1c020fe475ce1c50c
Merge pull request #751 from Mossop/bug836318

Bug 836318: Items in submenus default to visible rather than using the PageContext r=@erikvold(cherry picked from commit 8ecc5decb9cd85c8c8eebebbb1c7fba15b039930)

Note You need to log in before you can comment on or make changes to this bug.