Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Implement onBeforeShow event

ASSIGNED
Assigned to

Status

()

Toolkit
WebExtensions: Frontend
ASSIGNED
2 years ago
4 days ago

People

(Reporter: kmag, Assigned: robwu, Mentored)

Tracking

({good-first-bug})

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [contextMenus]triaged)

Attachments

(3 attachments)

(Reporter)

Description

2 years ago
+++ 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.
(Reporter)

Updated

2 years ago
Blocks: 1211368

Updated

2 years ago
Flags: blocking-webextensions-

Comment 1

2 years ago
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.
(Reporter)

Updated

2 years ago
Whiteboard: [contextMenus] → [contextMenus][good first bug]
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?
Flags: needinfo?(kmaglione+bmo)
(Reporter)

Comment 3

2 years ago
I added the tag because the bug had a mentor assigned, but Gabor is the mentor, not me.
Flags: needinfo?(kmaglione+bmo) → needinfo?(gkrizsanits)

Updated

2 years ago
Assignee: nobody → mwein2009

Updated

2 years ago
Iteration: --- → 47.1 - Feb 8
(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.
Flags: needinfo?(gkrizsanits) → needinfo?(wmccloskey)
(Reporter)

Comment 5

2 years ago
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.
(Reporter)

Comment 6

2 years ago
(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.
Flags: needinfo?(wmccloskey) → needinfo?(kmaglione+bmo)
(Reporter)

Comment 8

2 years ago
(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.
Flags: needinfo?(kmaglione+bmo)
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.

Updated

2 years ago
Whiteboard: [contextMenus][good first bug] → [contextMenus][good first bug]triaged
(Reporter)

Comment 10

2 years ago
Sounds good to me.
(Assignee)

Comment 11

2 years ago
Here is a similar feature request (with use cases from users) in Chromium: https://crbug.com/60758

At this early stage in API design, try to avoid APIs that are known to have severe disadvantages (performance, OOP support, ...).

Assuming that firing up an isolated JavaScript context in the browser process is inexpensive, I suggest to implement dynamic context menus by executing some static JavaScript code/function in an isolated JavaScript environment (without much privileges). If extensions become OOP, then latency due to IPC between the browser and extension process won't have an impact, since the context menu script can be run in the browser process.
PAC scripts are existing example of this concept: https://en.wikipedia.org/wiki/Proxy_auto-config.

Here is an example of how it could work (you can probably think of better names, but this is to show the idea):

chrome.contextMenus.create({
  id: 'my-context-menu',
  title: 'Call me',
  beforeShowFile: 'doSomething.js',
  onClicked: function(info, tab) {
    alert('');
  },
});

// doSomething.js
function ShouldShowContextMenu(context) {
  if (context.type === 'selection' &&
      context.selectionText.includes('my secret')) {
    return true;
  }
  // ( ... some other things ... )
  return false;
}

Instead of returning false, you could also return an object, which could be used to change the rendered context menu, if feasible/desirable.
Iteration: 47.1 - Feb 8 → 47.2 - Feb 22
Iteration: 47.2 - Feb 22 → ---
Assignee: mwein → nobody
(Assignee)

Updated

a year ago
Assignee: nobody → rob
(Assignee)

Comment 12

a year ago
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?
Flags: needinfo?(kmaglione+bmo)
(Reporter)

Updated

a year ago
Duplicate of this bug: 1280555
Attachment #8723939 - Flags: review?(kmaglione+bmo)

Updated

a year ago
Keywords: good-first-bug
Whiteboard: [contextMenus][good first bug]triaged → [contextMenus]triaged

Updated

10 months ago
Component: WebExtensions: Untriaged → WebExtensions: Frontend
Flags: blocking-webextensions-
Priority: -- → P5
(Reporter)

Updated

9 months ago
Attachment #8723939 - Flags: review?(kmaglione+bmo)
(Reporter)

Updated

7 months ago
Duplicate of this bug: 1322488

Comment 15

6 months ago
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
Priority: P5 → --

Comment 17

3 months ago
Created attachment 8856126 [details]
bug1215376_testcase@mozilla.org.xpi">bug1215376_testcase@mozilla.org.xpi

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.

Comment 18

3 months ago
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?

Comment 19

3 months ago
Played with HTML5 context menu today to get rid of the problems here. All was fine until a reach a IFRAME source - no HTML5 context menu entries on Iframes...no real alternatives here.
(Reporter)

Updated

2 months ago
Duplicate of this bug: 1366579
Further rationale to this bug: https://github.com/mozilla/testpilot-containers/issues/520

For the "tab" context state we would like to show the state of the clicked tab rather than the current tab. This is actually not possible at all.

Comment 22

2 months ago
I found that the HTML5 workaround works also on iframes, if the menu is placed in the iframe and not in the levels above. And if i use where i need it but not on jsfiddle(tried it for testing, but no way). Will switch all here to HTML5 context menus now.
Hi Gabor,

What's needed to finish implementation of this bug?

I've been looking for mentored bugs outside screenshots to work on, and this seems pretty important for webextensions in general, and for two test pilot experiments in particular (min vid and containers).

I'd be happy to steal from :robwu, provided I can get an updated TODO list - I'm sure a lot has changed in the year this has been sitting idle, particularly with e10s and OOP webextensions.

(:robwu, please let me know if you'd like to finish this one or work together to finish it)

Thanks!

Jared
Flags: needinfo?(rob)
Flags: needinfo?(gkrizsanits)
(Assignee)

Comment 24

a month ago
I have thought of multiple alternatives to implement this feature, each with pros and cons. I'll discuss this (with :mixedpuppy) at the SF all-hands. If you want to join the discussion, mail me and I will let you know when we meet.

Briefly, among my thoughts:
1. whether and how to block creation of the context menu
2. or whether to allow context menu labels to be updated after they are shown
3. whether to have a default (fallback) label that is updated after the menu item is shown.
Flags: needinfo?(rob)
(In reply to Jared Hirsch [:_6a68] (please use 'needinfo' :-) from comment #23)
> Hi Gabor,
> 
> What's needed to finish implementation of this bug?
> 
> I've been looking for mentored bugs outside screenshots to work on, and this
> seems pretty important for webextensions in general, and for two test pilot
> experiments in particular (min vid and containers).
> 
> I'd be happy to steal from :robwu, provided I can get an updated TODO list -
> I'm sure a lot has changed in the year this has been sitting idle,
> particularly with e10s and OOP webextensions.
> 
> (:robwu, please let me know if you'd like to finish this one or work
> together to finish it)
> 
> Thanks!
> 
> Jared

It's been a while since I worked on anything add-on related, so I don't think I'm the right person to make any decision here. Based on :robwu's comment I'm removing the needinfo flag, but if you need any help during the SF allhands, let me know. Although I'm pretty sure :kmag would be a lot more knowledgeable person here to ask for guidance than me :)
Flags: needinfo?(gkrizsanits)
Hey Kris - I guess you've inherited this bug! Any thoughts on which of Rob's suggestions in comment 24 might be the best starting point?
I got here by this thread that I opened https://discourse.mozilla-community.org/t/change-label-in-the-contextmenu-before-the-opening/16620

There is a chance to have that feature soon?

Updated

21 days ago
Flags: needinfo?(kmaglione+bmo)
An example of extension that will use this feature (based on the previous discourse thread) https://github.com/mte90/gulpease
(Assignee)

Comment 29

21 days ago
There won't be a blocking onBeforeShown event.
I'm going to implement the following two events instead:

- browser.contextMenus.onShown
- browser.contextMenus.onHidden

And the following API extensions:

- The contextMenus.create/update/remove/removeAll APIs can be used while a contextMenu is being shown (e.g. during/after the onShown event).
- Context menu items can be updated at any time, with no restrictions if possible (i.e. already-shown menu items can be updated).
- For adding/removal, it depends on whether the menu is already shown.
- When the menu is hidden, the behavior will remain unchanged (items can freely be added/removed).
- When the menu is shown, new items are appended to the end (and when there are too many items, I will either throw an error or combine the menu items, whichever is easier/makes more sense).
- When the menu is shown, items in the top-level menu CANNOT BE REMOVED, to avoid bad UX by unexpected shifting of menu items. Items in submenus can freely be removed (even if this means that menu items would shift around. UX is a lesser concern here because it is presumably clear to the user that the menu is managed by the add-on).

I will not start with the implementation right away, so if this API does not meet your needs, please speak up now before the API is set in stone.
Status: NEW → ASSIGNED
I think that in this way is more cool and more powerful that the initial suggestion :-)
(In reply to Rob Wu [:robwu] from comment #29)
> There won't be a blocking onBeforeShown event.
> ...

> - When the menu is shown, items in the top-level menu CANNOT BE REMOVED, to
> avoid bad UX by unexpected shifting of menu items. Items in submenus can
> freely be removed (even if this means that menu items would shift around. UX
> is a lesser concern here because it is presumably clear to the user that the
> menu is managed by the add-on).
> 
> I will not start with the implementation right away, so if this API does not
> meet your needs, please speak up now before the API is set in stone.

I would use this API to dynamically remove/add my ONLY extension item (=top level menu item) to the top level context menu. If you were to impose restrictions on the "top level" context menu like this, the API isn't usable for me.

Examples:
* I want to add a context menu item to "a" elements that contain a certain string (domain). If they do not contain that string, I want to hide/remove the context menu item (which is probably in the top level context menu).
* I want to check for the current page URL. If it contains "addons.mozilla.org", "about:" or something internal, I want to hide/remove my context menu item. (On a side note, I would like to note that not allowing context menu items by default on such pages would break many extensions that can still do something useful without a content script)
Flags: needinfo?(rob)
(Assignee)

Comment 32

19 days ago
(In reply to Geoffrey De Belie (Smile4ever) from comment #31)
> (In reply to Rob Wu [:robwu] from comment #29)
> > - When the menu is shown, items in the top-level menu CANNOT BE REMOVED, to
> > avoid bad UX by unexpected shifting of menu items. Items in submenus can
> > freely be removed (even if this means that menu items would shift around. UX
> > is a lesser concern here because it is presumably clear to the user that the
> > menu is managed by the add-on).
> 
> I would use this API to dynamically remove/add my ONLY extension item (=top
> level menu item) to the top level context menu. If you were to impose
> restrictions on the "top level" context menu like this, the API isn't usable
> for me.
> 
> Examples:
> * I want to add a context menu item to "a" elements that contain a certain
> string (domain). If they do not contain that string, I want to hide/remove
> the context menu item (which is probably in the top level context menu).
> * I want to check for the current page URL. If it contains
> "addons.mozilla.org", "about:" or something internal, I want to hide/remove
> my context menu item. (On a side note, I would like to note that not
> allowing context menu items by default on such pages would break many
> extensions that can still do something useful without a content script)

All of your use cases can be implemented by *adding* menu items in the onShown event (and to clean up, removing the menus in the onHidden event).
Flags: needinfo?(rob)
Wont we see menu items shuffle around a lot on show if the events are not blocking or would it execute all the handler before showing (Ie promises won't finish but normal code will)?
You need to log in before you can comment on or make changes to this bug.