Skip to content

Conversation

@mrobinson
Copy link
Member

Add context menu options for images, links, and editable text areas. In
addition add the ability to show menu options that are disabled. This
also improves the visual style of the context menu in egui as part of
supporting disabled options.

Testing: This has been manually tested, but we could we should be able to
easily add unit tests when enriching the API with information about the
active element under context menus.

@servo-highfive servo-highfive added the S-awaiting-review There is new code that needs to be reviewed. label Nov 8, 2025
@TimvdLippe
Copy link
Contributor

Does this supersede #39291 as well?

@mrobinson mrobinson marked this pull request as draft November 9, 2025 10:57
@mrobinson
Copy link
Member Author

I believe this does supersede #39291.

We probably need to do a bit more work here:

  1. Some actions should likely be done on the embedder process side so that can be executed even after a page navigation.
  2. We need to ensure that actions executed after navigation have no effect (ie you shouldn't be able to select all text on an input box of a page that is no longer active).
  3. We should probably add information about the currently selected item (which makes the first thing easier).
  4. It likely should have at least a rudimentary WebView API test.

@webbeef
Copy link
Contributor

webbeef commented Nov 9, 2025

I expected that embedders would receive a set of items that just describe the context based on hit test and selection, like:

  • url of image
  • url of link
  • selected text
  • etc.

And then the embedder decides what commands end up in the context menu. Correct me if I'm wrong but this patch seems to put all the decision process on the script side. If so I don't think that's the right approach in terms of layering and flexibility. So basically, your 1. point with s/Some/All :)

This change also introduce a bunch of hardcoded strings, which is fixable once we decide on a solution for #35893 but still not ideal.

@mrobinson
Copy link
Member Author

I expected that embedders would receive a set of items that just describe the context based on hit test and selection, like:

* url of image
* url of link
* selected text

This is point 3 on the list above. Some of this information would be provided to the embedder, but not all of it can be provided in a way that accurately reflects the page or is performant. For instance, the selected text can change after the menu is open. In addition, in order to implement rich copy and paste support, it isn't really performant to always send all of the data to the embedder. For instance, to allow copy and pasting image data, we should not be sending the image data to the embedder every time a context menu is opened. If the user selects the entire text of the page, I'm not sure it makes sense to send the entire DOM to the embedder (which would be necessary to put HTML content on the pasteboard).

Instead, we provide a pre-built menu to the embedder with certain pre-defined actions that can only be performed on the script side as well as some extra information in order to allow the embedder to implement extensions to the menu functionality. In the end the embedder decides which menu entries to show to the user. Embedders can add and remove menu elements as they choose.

This API is based on the WebKit API which is probably the most flexible API that exists for web engine context menus: https://webkitgtk.org/reference/webkit2gtk/2.5.1/WebKitWebView.html#WebKitWebView-context-menu

This change also introduce a bunch of hardcoded strings, which is fixable once we decide on a solution for #35893 but still not ideal.

Yes, the strings are hardcoded for now, but we need to implement localization to fix this as you. When we introduce localization, it's quite likely the strings will still exist as they do now, but wrapped in some kind of macro.

@webbeef
Copy link
Contributor

webbeef commented Nov 9, 2025

In your model, how to you implement something like "Search for ..." when some text is selected, knowing that this should build a url based on the opensearch template for the user's search engine? Or a "Open in private browsing tab" choice? Some of the commands look more like end user product choices than runtime constraints.

This API is based on the WebKit API which is probably the most flexible API that exists for web engine context menus: https://webkitgtk.org/reference/webkit2gtk/2.5.1/WebKitWebView.html#WebKitWebView-context-menu

In Gecko each embedder can implement child and parent (equivalent to script vs. constellation/embedder in Servo) side actors that will manage the whole interaction as they wish. This looks a lot more flexible than what is proposed here.

@mrobinson
Copy link
Member Author

mrobinson commented Nov 10, 2025

In your model, how to you implement something like "Search for ..." when some text is selected, knowing that this should build a url based on the opensearch template for the user's search engine? Or a "Open in private browsing tab" choice? Some of the commands look more like end user product choices than runtime constraints.

You could implement this by exposing whether or not the item associated with the hit test contained a selection and then using the evaluate JavaScript API to fetch the selected contents when the context item option is selected.

In Gecko each embedder can implement child and parent (equivalent to script vs. constellation/embedder in Servo) side actors that will manage the whole interaction as they wish. This looks a lot more flexible than what is proposed here.

WebKit also has an API like this, but it requires loading code into the content process. I think it's a little bit orthogonal to the basic context menu API. What extension points specifically for context menus does Gecko provide in the content process?

Still, I think it's a bit much to require creating a content process plugin simply to show a context menu. This kind of API is enough for the majority of embedders.

Add context menu options for images, links, and editable text areas. In
addition add the ability to show menu options that are disabled. This
also improves the visual style of the context menu in egui as part of
supporting disabled options.

Signed-off-by: Martin Robinson <mrobinson@igalia.com>
Co-authored-by: Rakhi Sharma <atbrakhi@igalia.com>
@atbrakhi atbrakhi marked this pull request as ready for review November 10, 2025 17:33
@webbeef
Copy link
Contributor

webbeef commented Nov 10, 2025

I think we will just have to agree that we disagree, but I feel bad about limiting creativity of embedders this way. The runtime should serve them better, and the existing browsers are not the ceiling to aim for .

You could implement this by exposing whether or not the item associated with the hit test contained a selection and then using the evaluate JavaScript API to fetch the selected contents when the context item option is selected.

Knowing that the hit test contains a selection is not enough for a good UX if you need another roundtrip to fetch the selected content... either that will slow down showing the menu or you have to display incomplete information first.

On content process extensibility, yes it's a tricky thing. It's easier in Gecko because child actors are more or less content scripts that run privileged JS, and have access to the platform either through xpcom or custom objects to send messages back & forth with the parent process.
But I'm not advocating to build something like that for context menu support - just that the whole data required be passed to the embedder, and that actions stay on the embedder side, not the runtime one.

@mrobinson
Copy link
Member Author

Knowing that the hit test contains a selection is not enough for a good UX if you need another roundtrip to fetch the selected content... either that will slow down showing the menu or you have to display incomplete information first.

In other browsers they show a subset of the selection in the menu for the "Search for ..." option. Providing the embedder with a subset is probably doable. What we cannot do, I think, is to provide absolutely everything from the web content. The plan is to design the API in a way that's extensible enough to send more information if it's commonly needed and then to provide affordances for embedders that need more information.

A round trip when selecting a menu option is likely not such a big deal though. Consider that opening a new WebView or navigating is many times more expensive.

But I'm not advocating to build something like that for context menu support - just that the whole data required be passed to the embedder, and that actions stay on the embedder side, not the runtime one.

The issue is that context sensitive actions cannot be performed on the embedding side. For instance how would you implement Select All inside <input>? In the end this had to happen in script. Consider also that script can also easily get out of sync with the embedder side.

@webbeef
Copy link
Contributor

webbeef commented Nov 10, 2025

In other browsers they show a subset of the selection in the menu for the "Search for ..." option. Providing the embedder with a subset is probably doable.

You display a subset, but you need the full string to build the search url. So you will need to some other api to fetch the full selected content.

What we cannot do, I think, is to provide absolutely everything from the web content.

Gecko does it, and that seems to work fine.

A round trip when selecting a menu option is likely not such a big deal though. Consider that opening a new WebView or navigating is many times more expensive.

It's not when selecting a menu option, you need the data when building the menu option.

I agree that some actions can only be done on the script side. I challenge the assumption that "Select All" in an input field is used more the "Search XYZ..." though.

@mrobinson
Copy link
Member Author

What we cannot do, I think, is to provide absolutely everything from the web content.

Gecko does it, and that seems to work fine.

Do you happen to have a link to this code? If Gecko provides the entire page selection when opening a context menu we may indeed be able to provide this as well. In any case, I think we are 90% in agreement here. The plan really is to provide more information to the embedder. We just plan to do it in a followup (probably today). 😁

@servo-highfive servo-highfive removed the S-awaiting-review There is new code that needs to be reviewed. label Nov 11, 2025
@mrobinson mrobinson added this pull request to the merge queue Nov 11, 2025
@servo-highfive servo-highfive added the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Nov 11, 2025
Merged via the queue into servo:main with commit 1e4feea Nov 11, 2025
32 checks passed
@mrobinson mrobinson deleted the context-menu-part-2 branch November 11, 2025 12:16
@servo-highfive servo-highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Nov 11, 2025
@webbeef
Copy link
Contributor

webbeef commented Nov 11, 2025

Gecko does it, and that seems to work fine.

Do you happen to have a link to this code? If Gecko provides the entire page selection when opening a context menu we may indeed be able to provide this as well.

The child side gets the selection here: https://searchfox.org/firefox-main/rev/931471037b8a4f16ce3ccfa864310540cac255c1/browser/actors/ContextMenuChild.sys.mjs#621 and sends it to the parent at https://searchfox.org/firefox-main/rev/931471037b8a4f16ce3ccfa864310540cac255c1/browser/actors/ContextMenuChild.sys.mjs#724

Looking more closely at the selection utils, the length is clamped at 16k at https://searchfox.org/firefox-main/rev/931471037b8a4f16ce3ccfa864310540cac255c1/toolkit/modules/SelectionUtils.sys.mjs#144 for the full selected text and very to a very small 150 chars for the trimmed selection. So... not what I thought!

Testing on https://en.wikipedia.org/wiki/Tartan selecting all the page and pasting the text is ~400KB but that seems to directly use the system clipboard.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants