3.73 KB, application/x-xpinstall
14.50 KB, patch
|Details | Diff | Splinter Review|
2.10 KB, application/x-xpinstall
+++ This bug was initially created as a clone of Bug #1211368 +++ (In reply to Bill McCloskey (:billm) from bug 1190667 comment #2) > Thanks for pointing this out. We should add an onBeforeShow event. If the > extension takes too long to respond, we can show the menu and add any > extension-created items later if they appear.
Created attachment 8700407 [details] content DOM-orientated contextMenu test extension I am adding a test extension with the relevant functionality. Short workflow description: 1) add a mousedown|contextmenu listener on the content DOM 2) the listener generates data based on event target 3) data is sent to the background process 4) background process uses it to generated the menu However, the displayed menu does not contain the generated items. Presumably, due to a race condition.
Kris, as you added the "good first bug" tag, could you please add some details about the scope of the bug and some guidance how and where to fix this?
I added the tag because the bug had a mentor assigned, but Gabor is the mentor, not me.
(In reply to Kris Maglione [:kmag] from comment #0) > +++ This bug was initially created as a clone of Bug #1211368 +++ > > (In reply to Bill McCloskey (:billm) from bug 1190667 comment #2) > > Thanks for pointing this out. We should add an onBeforeShow event. If the > > extension takes too long to respond, we can show the menu and add any > > extension-created items later if they appear. Since there is a little bit of API design here, let's settle that first before Matt start working on this. We can do two things here: 1. While the context menu is shown we can keep a reference to the menu and store the context as well, and when an extension adds a new element to the menu or modifies an item, we can just do a quick update on the menu. We need to test this if this looks acceptable but if it does, this would be as simple as it can get and there is no need for a new API. 2. Add an onBeforeShow event sink to contextMenus and fire the event whenever a contextmenu pops up as the bug suggests. This should be blocking the contextMenu to be shown while an add-on code runs, and I'm not too happy about this. Not the mention out of process add-ons, which can further complicate things here because that would mean that there is a cross process blocking behavior. If it's done right it would not block the process just the contextmenu ofc but still... Anyway probably this can get tricky. 3. We can even add an extra event at DOM level, but I'm not a big fan of event without specs, so I don't even go there... I'm leaning toward 1 but I want to hear your opinion first Bill, before I give some helping instructions how to implement this.
I was looking into this with Matt the other day, and it seemed like the easiest thing would be to call an onBeforeShow handler in the extension with the same (or similar) context data as we get from on-build-contextmenu. It gets more complicated if the extension needs more context than that, admittedly. As for blocking the context menu, yeah, that's not ideal. I'm not sure how much OOP add-ons affect it, since we need to message the content process for context menus in any case (and we still rely a lot on CPOWs). Bill mentioned in the comment where he suggested adding the event that we could impose a timeout on the reply, and then update the menu when we get it if it's already been shown. That doesn't sound like good first bug territory, but it could be done in a follow-up, once we actually have OOP extensions.
(In reply to Kris Maglione [:kmag] from comment #5) > call an onBeforeShow handler in the extension with the same (or similar) > context data as we get from on-build-contextmenu. (N.B., data sanitized based on the extension's permissions, possibly including content script permissions)
What's your reasoning against 1) exactly? Live updating the menu while it's open should solve all these difficulties, no? (In reply to Kris Maglione [:kmag] from comment #5) > I was looking into this with Matt the other day, and it seemed like the > easiest thing would be to call an onBeforeShow handler in the extension > with the same (or similar) context data as we get from on-build-contextmenu. > It gets more complicated if the extension needs more context than that, > admittedly. > > As for blocking the context menu, yeah, that's not ideal. I'm not sure how > much OOP add-ons affect it, since we need to message the content process for > context menus in any case (and we still rely a lot on CPOWs). > Right now content process sends up a message to the chrome process and the parent process calls into the ext-contextMenus function synchronously. This works now, because the add-on is in the parent process as well, but we need to figure out a new module for OOP add-ons. I'm not comfortable with blocking the parent process at all, and adding a timeout to that is just a hack for damage control. Live updating the menu would fix both issues. The other solution would be to send an async message from the parent process to ext-contextMenus and when it thinks it's ready with everything, message back to the parent that it can open the context menu. This would delay the context menu opening but would not block the parent process. Same timeout can be applied, but we must be careful about various things ofc like an incoming second attempt for opening the context menu. Anyway, if we choose this approach I don't think it's a good first bug. > Bill mentioned in the comment where he suggested adding the event that we > could impose a timeout on the reply, and then update the menu when we get it > if it's already been shown. That doesn't sound like good first bug > territory, but it could be done in a follow-up, once we actually have OOP > extensions. Again, I think it's a really bad idea to block the parent process from the add-on.
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #7) > What's your reasoning against 1) exactly? Live updating the menu while it's > open should solve all these difficulties, no? I have nothing against 1). I'm just not sure it removes the need for an event. Bill's proposal in the original bug was to add an event, wait for a short time for the add-on to respond, and then live update with any changes that happened later. I don't really have anything against skipping the delay either, as long as the UX isn't too awkward. > Right now content process sends up a message to the chrome process and the > parent process calls into the ext-contextMenus function synchronously. This > works now, because the add-on is in the parent process as well, but we need > to figure out a new module for OOP add-ons. I'm not sure OOP add-ons change anything. As I understand it, OOP add-ons will live in the regular content process, so it should be possible to trigger their listeners synchronously before calling out to the parent process. Granted, that could always change and it probably wouldn't be great to design an API that absolutely required it. > I'm not comfortable with blocking the parent process at all, and adding a > timeout to that is just a hack for damage control. I think it's more a matter of trying to balance the overall experience. There's a question of whether we want to add a delay capped to a nearly imperceptible timeout if we know an add-on may want to add entries, or show the context menu immediately and then (after a very short delay) add new entries to it after it opens. I think the latter would be more obvious to the user. And it would require a bit more UI work, because I'm not sure we could get away with it without transitions. I'm not sure whether it would be a good or bad effect, though. I'd be willing to try it and find out. > The other solution would be to send an async message from the parent process > to ext-contextMenus and when it thinks it's ready with everything, message > back to the parent that it can open the context menu. This would delay the > context menu opening but would not block the parent process. Yeah, I was assuming it would be done with something along those lines.
Alright, I think we're on the same page more or less let me summarize the todos here: 1. Add support for live updating the menu 2. Add an onBeforeShow event that blocks 3. Set up a timer with really short timeout that unblocks the context-menu optional after 1: check if without an onBeforeShow event live updating the menu right after it opens looks acceptable or weird. And let's take care of the OOP issue in another bug. 3 should be also carried over to follow up bug, but in the related docs we must mention that onBeforeShow comes with a timeout.
Sounds good to me.
Created attachment 8723939 [details] [diff] [review] Prototype of beforeShowFile (basic functionality + schema + tests) This beforeShowFile prototype shows that the title can be changed and context menu items can be suppressed (the tests pass). There is still a long way to go before it can be merged though. At the very least, the script needs to be moved to a separate thread to prevent bad behaving scripts from blocking the browser process. Kris, could you take a look?
This bug would be still keeping me from transitioning to web-ext. Since there does not seem to be any traction/ideas for it, and no actual time left for waiting, I think I'll have to disable/rethink the related functionality
:zombie proposed I explained my use-case here. In the test pilot containers experiment we are trying to add an assignment feature which is assigning a container based upon a hostname. The user can once they have a container tab open for a site can assign a container with a checkbox menu item "Always open in this container". The state of the check would be based upon if they have assigned to the container. User flow is: 1. User opens banking container 2. User opens myamazingbank.com 3. User right clicks and sees a context menu item "Always open in this container" 4. User clicks menu item 5. User is given a notification they have assigned myamazingbank.com to banking container. 6. User opens another tab in a personal container and types myamazingbank.com 7. Tab closes 8. Tab opens in Banking 9. User is presented with a warning about myamazingbank.com about to open 10. User clicks confirm 11. User is taken to the page The context menu should not show for: - about: urls - non containers - private windows To add a layer of complexity we would also like to hide this option when the current container doesn't match what they have assigned for this hostname. In the userflow 9. would permit them to change what the container they open just for this tab (Assign will still be in place but they open their work gmail just for one tab). So as :zombie suggested we could: - Use documentUrlPatterns to filter based on all assigned URLs - Add a filter for isContextId and isIncognito However this still doesn't allow us to display text/checked state on when a URL pattern matches an assignment to the current userContextId. Having a blocking event handler would not only make the current code much easier (which checks for page loads, contextId assignments, tab changes and window focuses) it would also fix a case where user right clicks to focus into a different window (this actually works in Chrome also without any promise based code etc). See the following for more info on the window focus issue: https://github.com/jonathanKingston/extension-debugging/tree/master/context-menu-window-focus-issue
Created attachment 8856126 [details] email@example.com">firstname.lastname@example.org Added a simpler test case to this bug. It should work on google.com(untested) and google.de if you do a right click on input field. But after playing with this, i have more questions as before. 1.) The context menu create works always on 2nd try and stays present then for next click. And of course if you click outside the input field to remove the menu entry it works also on 2nd try. There is always a delay of one click. 2.) More interesting is for test i called chrome.contextMenus.removeAll() after creation with setTimeout() of 500ms to remove all entries again. You can guess what happened. I see never the menu item, there is enough time to remove it before the menu will be showed. For 900ms i see the menu entry sometimes, but also very often not. This means i have a time interval of 500ms to add and remove the entries and this is not enough. For my feeling 500ms are already a very long period to get all managed but not for this task.
What is the plan here? Fx 57 comes every day one day closer and we do not know what we shall do here. Shall we wait? Shall we switch to HTML5 context menus? Specially on webpages which loads some content later as pageload event fires i would prefer not to use HTML5 context menus, because then we need again additional observers. P5 was removed here, what do you think about this bug?