Closed
Bug 836318
Opened 11 years ago
Closed 11 years ago
Menu does not work with SelectorContext
Categories
(Add-on SDK Graveyard :: General, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: canuckistani, Assigned: mossop)
References
Details
Attachments
(1 file)
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] });
Reporter | ||
Comment 1•11 years ago
|
||
Repro example: https://builder.addons.mozilla.org/package/171931/latest/ The first case does not work, the second does.
Assignee | ||
Comment 2•11 years ago
|
||
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.
Assignee | ||
Comment 3•11 years ago
|
||
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.
Reporter | ||
Comment 4•11 years ago
|
||
Where are the other comments? I've only seen the one google group thread.
Assignee | ||
Comment 5•11 years ago
|
||
(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.
(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.
Assignee | ||
Comment 7•11 years ago
|
||
Pointer to Github pull-request
Assignee | ||
Updated•11 years ago
|
Attachment #708369 -
Flags: review?(evold)
Comment 8•11 years ago
|
||
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
Updated•11 years ago
|
Attachment #708369 -
Flags: review?(evold) → review+
Updated•11 years ago
|
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Comment 9•11 years ago
|
||
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)
You need to log in
before you can comment on or make changes to this bug.
Description
•