Closed Bug 1435992 Opened 3 years ago Closed 3 years ago

Disabled page actions still show extension context menu items

Categories

(WebExtensions :: Frontend, defect)

60 Branch
defect
Not set
normal

Tracking

(firefox58 wontfix, firefox59 fixed, firefox60 fixed)

RESOLVED FIXED
mozilla60
Tracking Status
firefox58 --- wontfix
firefox59 --- fixed
firefox60 --- fixed

People

(Reporter: ke5trel, Assigned: adw)

References

Details

Attachments

(3 files)

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.
Component: Toolbars and Customization → WebExtensions: Frontend
Product: Firefox → Toolkit
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
Blocks: 1412170
No longer blocks: 1413574
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
Attachment #8948751 - Flags: review?(mixedpuppy) → review+
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
Assignee: nobody → adw
Status: NEW → ASSIGNED
Pushed by dwillcoxon@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7ea4725adfc3
Disabled page actions still show extension context menu items. r=mixedpuppy
https://hg.mozilla.org/mozilla-central/rev/7ea4725adfc3
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Please request uplift to beta when you get a chance.
Flags: needinfo?(adw)
Attached patch Beta patchSplinter Review
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
Flags: needinfo?(adw)
Attachment #8949100 - Flags: approval-mozilla-beta?
Comment on attachment 8949100 [details] [diff] [review]
Beta patch

One-liner fix with an automated test included. Approved for 59b8.
Attachment #8949100 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(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.
Flags: qe-verify-
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.
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.