Since a disabled page action is not visible in the address bar or clickable, it should be expected that its context menu is also disabled. Currently it is still populated with extension menu items that can be clicked and produce unintended results.

The webextension code governs whether disabled page action items have context menu items populated, so moving over there.

We need to add a `!this.browserPageAction.getDisabled(trigger.ownerGlobal)` check to this conditional here: https://dxr.mozilla.org/mozilla-central/rev/0d806b3230fe4767fa70cdee57db87d1e9a5ba49/browser/components/extensions/ext-pageAction.js#192

https://treeherder.mozilla.org/#/jobs?repo=try&revision=71a08ee89e7f This will have eslint failures that I fixed in the second revision.

Comment on attachment 8948751 [details] Bug 1435992 - Disabled page actions still show extension context menu items. https://reviewboard.mozilla.org/r/218148/#review223980 Code analysis found 2 defects in this patch: - 2 defects found by mozlint You can run this analysis locally with: - `./mach lint path/to/file` (JS/Python) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: browser/components/extensions/test/browser/browser_ext_menus.js:128 (Diff revision 1) > + > + const extension = ExtensionTestUtils.loadExtension({manifest, background}); > + const tab = await BrowserTestUtils.openNewForegroundTab(gBrowser, "http://example.com/"); > + > + await extension.startup(); > + const tabId = await extension.awaitMessage("ready"); Error: 'tabid' is assigned a value but never used. allowed unused vars must match /^(cc|ci|cr|cu|exported_symbols)$/. [eslint: no-unused-vars] ::: browser/components/extensions/test/browser/browser_ext_menus.js:130 (Diff revision 1) > + const tab = await BrowserTestUtils.openNewForegroundTab(gBrowser, "http://example.com/"); > + > + await extension.startup(); > + const tabId = await extension.awaitMessage("ready"); > + > + const menu = await openContextMenuInPageActionPanel(extension); Error: 'opencontextmenuinpageactionpanel' was used before it was defined. [eslint: no-use-before-define]

(In reply to Code Review Bot [:reviewbot] from comment #6) > Code analysis found 2 defects in this patch: > - 2 defects found by mozlint Fixed in the second revision

Comment on attachment 8948751 [details] Bug 1435992 - Disabled page actions still show extension context menu items. https://reviewboard.mozilla.org/r/218148/#review223990

Thanks for the quick review. There were strange, unrelated failures on that try push. I'm 100% sure I pulled a bad tree, but here's one more push with a fresh tree to make sure, before landing this: https://treeherder.mozilla.org/#/jobs?repo=try&revision=12b6e641e60a9e3b2d23ab1b070e311c598813f1

Pushed by dwillcoxon@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/7ea4725adfc3 Disabled page actions still show extension context menu items. r=mixedpuppy

Please request uplift to beta when you get a chance.

Approval Request Comment [Feature/Bug causing the regression]: It's not a regression because this has always been broken since the Photon page action menu was introduced. The original bug implementing context menus in the Photon page action menu is bug 1412170. [User impact if declined]: See comment 0 [Is this code covered by automated tests?]: Yes [Has the fix been verified in Nightly?]: No [Needs manual test from QE? If yes, steps to reproduce]: No [List of other uplifts needed for the feature/fix]: None [Is the change risky?]: No [Why is the change risky/not risky?]: Small, one-line change, covered by a test [String changes made/needed]: None

Comment on attachment 8949100 [details] [diff] [review] Beta patch One-liner fix with an automated test included. Approved for 59b8.

(In reply to Drew Willcoxon :adw from comment #13) > [Is this code covered by automated tests?]: > Yes > > [Has the fix been verified in Nightly?]: > No > > [Needs manual test from QE? If yes, steps to reproduce]: > No Setting qe-verify- based on Drew's assessment on manual testing needs and the fact that this fix has automated coverage.

Is there an exception to show the context menu with the option to pin the page action in the address bar? I know it won't actually appear there when disabled, but rarely active page actions would be very hard to add to the address bar after hidden to the menu, if there is no context menu for them until there is a page that activates them.

Yes, the context menu is actually still shown on disabled actions in the panel, and it has Add to/Remove from Address bar in it. It's just that it won't have the extension's menu items in it.