Disabled page actions still show extension context menu items

RESOLVED FIXED in Firefox 59

Status

defect
RESOLVED FIXED
Last year
Last year

People

(Reporter: ke5trel, Assigned: adw)

Tracking

60 Branch
mozilla60
Dependency tree / graph
Bug Flags:
in-testsuite +
qe-verify -

Firefox Tracking Flags

(firefox58 wontfix, firefox59 fixed, firefox60 fixed)

Details

Attachments

(3 attachments)

Reporter

Description

Last year
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

Last year
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
Assignee

Comment 2

Last year
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 hidden (mozreview-request)
Assignee

Updated

Last year
Blocks: 1412170
No longer blocks: 1413574
Comment hidden (mozreview-request)
Assignee

Comment 5

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

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

Comment 6

Last year
mozreview-review
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]
Assignee

Comment 7

Last year
(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

Last year
mozreview-review
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+
Assignee

Comment 9

Last year
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

Updated

Last year
Assignee: nobody → adw
Status: NEW → ASSIGNED

Comment 10

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

Comment 11

Last year
bugherder
https://hg.mozilla.org/mozilla-central/rev/7ea4725adfc3
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Please request uplift to beta when you get a chance.
Flags: needinfo?(adw)
Assignee

Comment 13

Last year
Posted 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.
Assignee

Comment 18

Last year
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.

Updated

Last year
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.