1435992 - Disabled page actions still show extension context menu items

Status: RESOLVED FIXED

(WebExtensions :: Frontend, defect)

Description

[Kestrel]

Attachment: disabled-page-action-context-menu.png

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.

Comment 1

[:Gijs (he/him)]

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

Comment 2

[Drew Willcoxon :adw]

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

Comment 3

[Drew Willcoxon :adw]

Attachment: Bug 1435992 - Disabled page actions still show extension context menu items.

Review commit: https://reviewboard.mozilla.org/r/218148/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/218148/

Comment 4

[Drew Willcoxon :adw]

Comment on attachment 8948751 [details]
Bug 1435992 - Disabled page actions still show extension context menu items.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/218148/diff/1-2/

Comment 5

[Drew Willcoxon :adw]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=71a08ee89e7f

This will have eslint failures that I fixed in the second revision.

Comment 6

[Code Review Bot [:reviewbot]]

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]

Comment 7

[Drew Willcoxon :adw]

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

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8948751 [details]
Bug 1435992 - Disabled page actions still show extension context menu items.

https://reviewboard.mozilla.org/r/218148/#review223990

Comment 9

[Drew Willcoxon :adw]

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

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7ea4725adfc3
Disabled page actions still show extension context menu items. r=mixedpuppy

Comment 11

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/7ea4725adfc3

Comment 12

[Julien Cristau [:jcristau]]

Please request uplift to beta when you get a chance.

Comment 13

[Drew Willcoxon :adw]

Attachment: Beta patch

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 14

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8949100 [details] [diff] [review]
Beta patch

One-liner fix with an automated test included. Approved for 59b8.

Comment 15

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/releases/mozilla-beta/rev/08a592e1b2ec

Comment 16

[Andrei Vaida [:avaida]]

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

Comment 17

[Michal Stanke (Mozilla.cz) [:mstanke][:MikkCZ]]

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.

Comment 18

[Drew Willcoxon :adw]

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.