836318 - Menu does not work with SelectorContext

Status: RESOLVED FIXED

(Add-on SDK Graveyard :: General, defect)

Description

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

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) (:⚡︎)]

Repro example:

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

The first case does not work, the second does.

Comment 2

[Dave Townsend [:mossop]]

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]]

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) (:⚡︎)]

Where are the other comments? I've only seen the one google group thread.

Comment 5

[Dave Townsend [:mossop]]

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

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

Attachment: Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/751/files

Pointer to Github pull-request

Comment 8

[[github robot]]

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]]

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)