Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature Request: [ContextMenus] Include <canvas>s in the "page" ContextType (or it's own ContextType) #717

Open
hicallmeal opened this issue Oct 30, 2024 · 11 comments
Labels
needs-triage: chrome Chrome needs to assess this issue for the first time opposed: firefox Opposed by Firefox supportive: safari Supportive from Safari

Comments

@hicallmeal
Copy link

Currently, the only way for a ContextMenu item to appear on a <canvas> click, is with the ["all"] ContextType.
(From testing on Firefox and Chromium).

This adds menu items to places they aren't needed (e.g. Popup), leading to potential user confusion.

An example use case might be a "Resize this image" menu item, which would ordinarily need ["image", "page" (SVGs)] ContextTypes, but "all" for canvases (for cases where a canvas has an image).

My request/proposal is <canvas> inclusion in the "page" context, or it's own (if there are security concerns).

@github-actions github-actions bot added needs-triage: chrome Chrome needs to assess this issue for the first time needs-triage: firefox Firefox needs to assess this issue for the first time needs-triage: safari Safari needs to assess this issue for the first time labels Oct 30, 2024
@maxnikulin
Copy link

I do not need namely CANVAS: "canvas" in contextMenus.ContextType, but related Firefox menus extensions look promising. Unfortunately my reading of #188 is that Chrome and Safari teams are against of features in that spirit.

A more general idea was posted to the chromium-extensions mailing list (likely by @robbi73): CSS selectors to specify HTML elements meaningful for context menu actions, see Why not brush up the contextMenu API a little? Sun, 2 Jul 2023 14:52:27 -0700 (PDT).

To avoid context menu item in extension action popups and side panels, there is a workaround, see Simeon Vincent. Re: [crx] Context menu appears on the pages of other extensions. It's a normal behaviour? Thu, 6 Jul 2023 15:07:39 -0700:

documentUrlPatterns value to match *://*/* ... will only match against HTTP and HTTPS pages

This approach does not allow however an opposite condition: to limit a menu items to side panels and action popups, but to avoid it in any regular tabs. Firefox has additional CreateProperties.viewTypes field paired with onClickData.viewType. Possible values: extension.ViewType. The "tab" value should prevent menu item appearance in action popups.

A PDF file may be a more tricky case for approaches other than CSS selector. Perhaps simple documentUrlPatterns is not enough in the case of an <iframe src="https://..."> embedded into a side panel.

Notice that context menu action for <canvas> element assumes scripting, but the activeTab permission is not enough in the case of a cross-origin frame. For accidental actions on random sites an extension must request either for rather dangerous <all_urls> or for more limited, by still permanent host permissions.

@xeenon xeenon added supportive: safari Supportive from Safari and removed needs-triage: safari Safari needs to assess this issue for the first time labels Nov 1, 2024
@xeenon
Copy link
Collaborator

xeenon commented Nov 1, 2024

I'm supportive of a canvas type. I don't think it should count as page.

@maxnikulin
Copy link

I'm supportive of a canvas type.

Do you plan to add some fields to OnClickData specific to canvas? From my point of view "canvas" scope is rather narrow and adding it may cause requests to add more HTML elements to context types. Unfortunately I can not estimate implementation and maintenance cost of a more flexible selector.

Just to express it explicitly: this issue depends on #716.

(Unfortunately compatibility data for onClickData on MDN are incomplete, so it is not obvious what fraction of extensions in respect to Chrome is supported by Safari. E.g. linkText is present, but viewType is not.)

@hicallmeal
Copy link
Author

documentUrlPatterns value to match *://*/* ... will only match against HTTP and HTTPS pages

@maxnikulin Admittedly this hadn't occured to me, in the context of this issue, as work-around, so I'm grateful you've mentioned it!

With regards to your second comment, when I created this issue I had treated/viewed it as unrelated to #716, as they were things I encountered that both happened to fall within the ContextMenus namespace.

But I can see what you're saying; I'd also love to see support for extended onClickData properties, but I would consider that mostly out of scope for this issue, especially when taking in to account the issue Fregante linked and comment by Rob Wu, in #716.

@Rob--W
Copy link
Member

Rob--W commented Nov 7, 2024

I'm not convinced of the need to add "canvas" in the contextMenus API. The contextMenus.onClicked event itself would not see much useful data. I'd be more supportive of a CSS selector method.

Note that in Firefox, we have also implemented the menus.onShown / menus.onHidden events, and together with the menus.refresh() and menus.getTargetElement() methods, this offers a very general way to allow extensions to dynamically update context menus for whatever use case extension developers can come up with.

@Rob--W Rob--W added opposed: firefox Opposed by Firefox and removed needs-triage: firefox Firefox needs to assess this issue for the first time labels Nov 7, 2024
@maxnikulin
Copy link

Note that in Firefox, we have also implemented the menus.onShown / menus.onHidden events, and together with the menus.refresh() and menus.getTargetElement() methods, this offers a very general way to allow extensions to dynamically update context menus for whatever use case extension developers can come up with.

These methods should allow to hide menu items if the frame is not reachable by content scripts (tabId = -1 for side panel or action popup, url is not exposed on first click) in the absence of onClickData.viewType. I described above <iframe src="https://..."> case matched by documentUrlPatterns: *://*/*.

In my opinion, running context menus must not require <all_urls> or host permissions. It should be enough to request activeTab (or not yet existing contextMenus.activeFrame for cross-origin frames). Firefox-128 properly grants activeTab when a menu item is clicked, so onShown may not have permissions to execute a content script yet. It would be a surprise for users if some extension runs scripts before its menu item is clicked.

menus.onShown and menus.refresh are not substitute for more precise declarative specification of menu item context.

@maxnikulin
Copy link

The contextMenus.onClicked event itself would not see much useful data.

Just brain-storming, since I have no details related to reporter's case: current canvas image might be exposed as Blob through onClickData.srcUrl. I anticipate obstacles since ideally it should be lazy synchronous getter. It makes "canvas" a kind of "image".

@Rob--W
Copy link
Member

Rob--W commented Nov 8, 2024

Note that in Firefox, we have also implemented the menus.onShown / menus.onHidden events, and together with the menus.refresh() and menus.getTargetElement() methods, this offers a very general way to allow extensions to dynamically update context menus for whatever use case extension developers can come up with.

These methods should allow to hide menu items if the frame is not reachable by content scripts

These events are not in content scripts but in the extension context, e.g. background script. If extensions want to they can hide menu items. Here is an example of dynamic context menu toggling without using any content scripts: https://github.com/Rob--W/bookmark-container-tab/blob/master/background.js

In my opinion, running context menus must not require <all_urls> or host permissions.

For common cases it would make sense to not require it. The contextMenus.onShown event does not grant activeTab, contextMenus.onClicked does.

In the reporter's use case, I think that the extension cannot meaningfully act on the context menu event without host permissions.

menus.onShown and menus.refresh are not substitute for more precise declarative specification of menu item context.

I'm in favor of declarative properties when they are more generally useful. We cannot add a declarative member for every single niche use case though, because that increases the complexity of the API.

The contextMenus.onClicked event itself would not see much useful data.

Just brain-storming, since I have no details related to reporter's case: current canvas image might be exposed as Blob through onClickData.srcUrl. I anticipate obstacles since ideally it should be lazy synchronous getter. It makes "canvas" a kind of "image".

This is not feasible. A canvas does not have a URL. And if you are thinking of generating a blob:-URL: for the serialization of the image bit map: that would mean that whenever the context menu is opened, that we'd perform an expensive operation before dispatching the event to an extension. That doesn't sound appealing to me.

Another issue with exposing this blindly is that a canvas can be tainted with cross-origin data. Exposing the content would basically require <all_urls> in the general case.

@hicallmeal
Copy link
Author

hicallmeal commented Nov 8, 2024

I agree with pretty much all the points outlined here and in the recent meeting.

I apologise, I really should have initially provided a better example (specifically, my use case).

So, in my case, I have a context menu item which is essentially "Scan this QR Code", which on click, obtains the QR image and decodes it.
Most commonly (since many QR codes are dynamically generated), they appear as <img> and <canvas> elements (and some sometimes as others - svg, div..)

Currently that means using the "image" and "page" context type (great to use), but "all" for canvas. And as noted initially, that adds the menu item to places it's not needed and could cause confusion.

That's what lead me to suggest it be included in "page", as it's simply another element. I included the seperate type in case that was preferred by vendors

On some of the points raised -

  • Definitely agree that it's niche, my particular use case wouldn't require it needing the cType
  • I believe I with executeScript, paired with getTargetElement, I won't need <all_urls> host permissions (unless tainted of course, for which I have a workaround of sorts)
  • A CSS selector method would work well too, I would be supportive of that as an alternative
  • I've had a look at the menus methods and I think this would work well with those too. I'm not awfully keen on having to check on every onShown event, but I'm not super against it either.

I should also say that this is admittedly quite reliant on #716, so I'm happy to pause discussion here until that is concluded.

@maxnikulin
Copy link

These events are not in content scripts but in the extension context, e.g. background script.

I feel some sort of confusion, but I can not figure out its source.

In Firefox it is possible to implement a context menu item visible only for <canvas> elements. It requires execution of a content script from menus.onShown background listener

browser.scripting.executeScript({
  target: {
    tabId: tab.id,
    allFrames: false,
    frameIds: [ info.frameId ?? 0 ],
  },
  func: id => browser.menus.getTargetElement(id)?.nodeName,
  args: [ info.targetElementId ],
})

However in the case of an iframe embedded into a side panel or an action popup tab.id = -1, so the extension will not be able to to recognize a QR presented by another extension.

A more serious issue, from my point of view, is that the extension must require the <all_urls> host permission, otherwise it can not execute the script from menus.onShown. I consider it as unacceptable in respect to privacy for an extension aimed to recognize a QR code or a similar purpose. That is why I wrote that menus.onShown and menus.refresh are not an alternative to the requested feature.

Even if CSS context matching or "canvas" type was implemented, activeTab would not be enough for cross-origin frames due to Chrome 40432579 (523572) and Firefox 1838753 bugs. Minimal privileges granted through menu item should be enough, developers should not request <all_urls> to provide reasonable usage experience.

@hicallmeal
Copy link
Author

@maxnikulin (This has come up in some testing) I just wanted to note, in reference to

documentUrlPatterns value to match :///* ... will only match against HTTP and HTTPS pages

I misspoke in regards to this -

This adds menu items to places they aren't needed (e.g. Popup), leading to potential user confusion.

I actually meant contextMenu clicking the browser action itself (i.e. the "action" ContextType equivalent), not Popup or general extension pages.
Meaning its appearance there isn't influenced by documentUrlPatterns

I generally agree with your last comment.
The firefox method would be great - I have to keep behaviour consistent across browsers, so it's not something I can implement for now (for my case).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-triage: chrome Chrome needs to assess this issue for the first time opposed: firefox Opposed by Firefox supportive: safari Supportive from Safari
Projects
None yet
Development

No branches or pull requests

4 participants