1797176 - "Just Save" context menu entry not appearing

Status: RESOLVED MOVED

(WebExtensions :: Developer Outreach, defect)

Description

[Asif Youssuff]

From https://www.reddit.com/r/firefox/comments/yce3qr/extension_just_save_stopped_working_in_ff106/

Steps to reproduce:

1. Install https://addons.mozilla.org/en-US/firefox/addon/just-save/
2. Navigate to https://getpocket.com/explore/item/the-icy-village-where-you-must-remove-your-appendix?utm_source=pocket-newtab
3. Context click on first image

What happens:

No context menu entry for "Just Save".

Expected result:

As before 1788108 -- I see a "Just Save" context menu entry.

Comment 1

[Asif Youssuff]

3:14.97 INFO: No more integration revisions, bisection finished.
3:14.97 INFO: Last good revision: 7ff360df4ab63ad30cf0eae6dcde56be8ec4aec7
3:14.97 INFO: First bad revision: f1204882a22db0d05e3ddf02f0ea2af94aba62e4
3:14.97 INFO: Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=7ff360df4ab63ad30cf0eae6dcde56be8ec4aec7&tochange=f1204882a22db0d05e3ddf02f0ea2af94aba62e4

Comment 2

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1788108

:rpl, since you are the author of the regressor, bug 1788108, could you take a look? Also, could you set the severity field?

For more information, please visit auto_nag documentation.

Comment 3

[Luca Greco [:rpl] [:luca] [:lgreco]]

I'm going to look into this asap.

Comment 4

[Luca Greco [:rpl] [:luca] [:lgreco]]

I looked into this and it looks that this issue would need to be handled on the extension side, at the moment::

• based on the manifest.json file the extension has been opting in explicitly to event pages (through the background.persistent manifest field being explicitly set to false)
• in Firefox < 106 the event pages were not supported in Firefox and background.persistent false was being ignored (and a warning logged to make the developer aware of that)
• in Firefox >= 106 the event pages support is now enabled by default and background.persistent isn't ignored anymore, and so the existing signed releases for the "Just Save" extension has started to run as an event page also in Firefox
• the extension event page script just-save-js is trying to detect if it is running on chrome (let isChrome = window.browser === undefined;) and based on that flag is currently expecting to be running as an event page in Chrome and as a persistent background page in Firefox
• in particular, when running in Firefox it is currently passing in its call to browser.contextMenus.create the onclick parameter, that is only supported in persistent background pages (because the callback function would be destroyed when the event page is suspended on idle, and so the menus events should be handled by registering a browser.contextMenus.onClicked API event listener, like the extension does here when running on chrome)
• when the extension runs on Firefox 106, the call to browser.contextMenus.create does throw an extension error and so the context menus is never been created successfully (as expected, introduced in Bug 1761814 and also documented on MDN)

Given that the extension error is raised synchronously the extension could use that to detect if it should use the same code it is using to run correctly as an event page in chrome, e.g. the following small tweaks to the just-save.js event page script] were enough to make it work in Firefox 106 (and should still be backward compatible with Firefox < 106 where the event page support is disabled and the script will be running as a persistent background page):

'use strict';

let isChrome = window.browser === undefined;

// Detect if running as an event page in Firefox.
function detectFirefoxEventPageMode() {
  if (isChrome) { return; }
  try {
    browser.contextMenus.create({ id: "test-menu", onclick: () => {} });
  } catch (err) {
    if (err?.message.includes("Property \"onclick\" cannot be used in menus.create")) {
      // Set isChrome to true if Firefox is detected to be running with event pages support enabled.
      isChrome = true;
    }
  } finally {
    browser.contextMenus.remove("test-menu");
  }
}

// adding browser shim for chrome support
if (isChrome) {
  window.browser = chrome;
} else {
  detectFirefoxEventPageMode();
}

... // everything else from just-save.js can stays as is.

It may be worth also calling out this kind of change (onclick property to be rewritten to a contextMenus onClicked API event) needed on calls to browser.menus.create / browser.contextMenus.create (and maybe also how to keep backward compatibility) more explicitly in https://extensionworkshop.com/documentation/develop/manifest-v3-migration-guide/

Comment 5

[Luca Greco [:rpl] [:luca] [:lgreco]]

Reported upstream here: https://github.com/lewisl9029/just-save-webextension/issues/10

Comment 6

[Luca Greco [:rpl] [:luca] [:lgreco]]

Closing as moved (along with marking the issue as dev-doc-needed and filing a followup to consider updating the docs as I also mentioned at the end of comment 4).

Comment 7

[Luca Greco [:rpl] [:luca] [:lgreco]]

Hi Donal, I just noticed the change to the status flags and I think that the proper status-firefox108 is also wontfix as for 107 (the issue has been closed as moved because the investigation determined that it is triggered by a bug to fix on the extension side).

Comment 8

[Donal Meehan [:dmeehan]]

Thanks Luca, correcting now

Comment 9

[rbloor]

Manifest V3 migration guide includes the information "Also, note that the menus.onClicked event or its alias contextMenus.onClicked must be used to handle menu entry click events from an event page, instead of the onclick parameter of the contextMenus.create or contextMenus.update methods. If the onclick property of menus.create or its alias contextMenus.create are used from a call originating from an event page, they throw synchronously."
Do we need any further changes or additions?

Comment 10

[Luca Greco [:rpl] [:luca] [:lgreco]]

That looks good enough for what I mentioned near the end comment 4, and it seems we have covered it as part of https://github.com/mozilla/extension-workshop/pull/1530 in Feb 2023 (which seems to be a few months after this issue was marked dev-doc-needed).

+1 on my side on marking this issue as dev-doc-completed