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

Inconsistent support for [context]menus.getTargetElement() #716

Open
hicallmeal opened this issue Oct 30, 2024 · 10 comments
Open

Inconsistent support for [context]menus.getTargetElement() #716

hicallmeal opened this issue Oct 30, 2024 · 10 comments
Labels
follow-up: chrome Needs a response from a Chrome representative implemented: firefox Implemented in Firefox needs-triage: chrome Chrome needs to assess this issue for the first time supportive: safari Supportive from Safari

Comments

@hicallmeal
Copy link

There are cases where an extension might need the element a contextMenu was opened (and actioned) on.
A classic example case might be to block/remove said element
Another, might be "Copy this element".

For this, Firefox has implemented getTargetElement(), paired with targetElementId.

For browsers without this, the best current solution (from what I've found) is to inject a content script with a contextMenu listener and get the target, i.e.

function contextMenuListener(evt) {
  let targetElement = evt.target
  copyElement(targetElement) ..
}

The inherent problem with this is the necessity for global content script injection, preventing dynamic injection based on user action.

Another solution is injection onClicked, retrieving the activeElemet, but for the above cases, this wouldn't suffice (with non-focusable elements and such).

As for implementation, I believe Firefox's approach is great for this.

Note - I'm not sure whether this falls under inconsistency or Feature request, though I believe it's implementation is warranted, so I've gone with inconsistency.

@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
@fregante
Copy link

Last discussion related to menus inconsistencies:

@hicallmeal
Copy link
Author

hicallmeal commented Oct 30, 2024

@fregante Thanks! I hadn't seen that one. I'm actually not sure how to word that in the post, so I'll say here (just generally speaking) -

Given the examples listed, and the desire for in-page actions based on a menu item click to be (I'd think) common enough, I'd say its addition to the ContextMenu APIs would be largely beneficial.

I definitely can't comment on other Menus v ContextMenus differences though!

@Rob--W
Copy link
Member

Rob--W commented Oct 30, 2024

Note: I designed and implemented menus.getTargetElement() in Firefox. There were multiple reasons for restricting it to the menus namespace without exposing it in the contextMenus namespace:

  1. The API would become available in content scripts. If exposed as contextMenus, it would have been more likely to break extensions if there is code like this:
    // common.js
    if (chrome.contextMenus) {
      // Script assumes that it is in the background page.
      chrome.contextMenus.onClicked.addListener(...);
    }
  2. The content script runs in every page. Restricting it to one namespace minimizes the overhead of exposing another namespace to the page.

Originally I wanted to put it behind the "menus.getTargetElement" permission to minimize exposure in content scripts, because the "contextMenus" permission is very commonly requested. Ultimately the permission was dropped because it was not really necessary.

@hicallmeal
Copy link
Author

I see, that certainly does make sense. I apologise, I hadn't considered that aspect of it.
And I definitely wouldn't want to cause complexities like that - I'm definitely with you on this!

Thanks for explaining

@hicallmeal
Copy link
Author

hicallmeal commented Oct 30, 2024

Oh, it just occured to me that perhaps coordinates could work?
Along with other standard OnClickData, another field "pointPosition" or something, could simply return the coordinates of the click.

Then a user can pass those to the page, and (in the example cases) pass to document.elementFromPoint and get the element that way. That can be done via dynamic injection.

Perhaps it could enable some new functionalities as well, for devs wanting to chart/mark points in the viewport.

Though I realise that's a bit of a stretch, and: it's yet another property, and can definitely be considered niche.
Also, Firefox would then essentially have duplicate behaviours. (I'm also not sure how pixel/accurate it would be compared to getTargetElement)

But still, thoughts on that?

@maxnikulin
Copy link

I miss getTargetElement() in Chrome contextMenus as well. It would allow to avoid workarounds that are far from being perfect.

  • Injecting content script with a polyfill to every page and frame is against declared mv3 purpose to reduce browser memory footprint.
  • document.activeElement works for elements that may be focused only, so it fails e.g. for images.
  • Guess if window.getSelection().focusNode related to context menu is unreliable since in Chrome click on an image may leave earlier selection position somewhere in text.
  • Heuristics based on trying to match document elements against onClickData.srcUrl value may give ambiguous result and is not applicable at all e.g. for <div>.

The question was raised earlier on the chromium-extension mailing list: How to get the field where a keyboard shortcut or context menu was called from? Mon, 12 Feb 2024 04:53:35 -0800 (PST) (handling subframes using commands even more tricky than with context menus).

It should work for purely keyboard navigation aka caret browsing ([F7] to activate then [Shift+F10] or [Menu] on Linux, see Firefox and Chrome help pages, unsure if macOS has a keyboard shortcut to open context menu). Non-text elements may be an issue. E.g. in Chrome menu for image context may be invoked by selecting image by cursor movement holding [Shift], Firefox does not allow it and even simple text case is affected by the bug 1835211.

Coordinates of click position are unreliable. During time interval between opening menu and selection of an item more images may be loaded causing reflow and shifting of target element to another position. (Side note: even user gesture timeout may be expired in DOM context in the case of delay after opening a menu.)

Since getTargetElement() is useful in content scripts only, security model should be updated or another permission should be introduced. activeTab gives access to top level page. To inject some code into a cross-origin <iframe> element, extension must request either <all_urls> or permanent host permission. It is tracked as Chrome 40432579 (523572) and Firefox 1838753 bugs.

To avoid issues with chrome.contextMenus as a test to distinguish background vs. content script environment in shared code, getTargetElement may be added to runtime.

@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 adding getTargetElement() and targetElementId to Safari. Tacking bug.

@maxnikulin
Copy link

I'm supportive of adding getTargetElement() and targetElementId to Safari.

Sorry for insistence, what is you opinion concerning either expanding the activeTab permission to the context menu frame or adding a new permission activeTab.contextMenus/contextMenus.frame for scripting? A dedicated issue may be created, but I am afraid, the Google team may reject the idea of target element.

@hicallmeal
Copy link
Author

@maxnikulin Good point re: coordinates, agree that it certainly falls short in some instances (which isn't optimal!)

As an end-user/dev I'd be happy to see any implementation that addresses the target retrieval, so for me personally I'll put my 👍 on any suggestion/issue

@Rob--W Rob--W added follow-up: chrome Needs a response from a Chrome representative implemented: firefox Implemented in Firefox and removed needs-triage: firefox Firefox needs to assess this issue for the first time labels Nov 7, 2024
@Rob--W
Copy link
Member

Rob--W commented Nov 7, 2024

@oliverdunk follow-up: chrome Needs a response from a Chrome representative because you were going to check whether there is interest at Chrome in implementing the getTargetElement API.

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

No branches or pull requests

5 participants