| CARVIEW |
Navigation Menu
-
-
Notifications
You must be signed in to change notification settings - Fork 3.4k
script: Add context-based context menu options #40501
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
Conversation
|
Does this supersede #39291 as well? |
|
I believe this does supersede #39291. We probably need to do a bit more work here:
|
|
I expected that embedders would receive a set of items that just describe the context based on hit test and selection, like:
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. |
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
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. |
|
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.
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. |
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.
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>
9e5c6d5 to
aae74c5
Compare
|
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 .
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. |
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.
The issue is that context sensitive actions cannot be performed on the embedding side. For instance how would you implement Select All inside |
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.
Gecko does it, and that seems to work fine.
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. |
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). 😁 |
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. |
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.