Implement onBeforeShow event

NEW
Assigned to
(NeedInfo from)

Status

()

Toolkit
WebExtensions: Frontend
2 years ago
3 days ago

People

(Reporter: kmag, Assigned: robwu, Mentored, NeedInfo)

Tracking

({good-first-bug})

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [contextMenus]triaged)

Attachments

(3 attachments)

+++ 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.
Blocks: 1211368

Updated

2 years ago
Flags: blocking-webextensions-

Comment 1

a year 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.
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)
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

a year ago
Assignee: nobody → mwein2009

Updated

a year 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)
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.
Flags: needinfo?(wmccloskey) → needinfo?(kmaglione+bmo)
(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

a year ago
Whiteboard: [contextMenus][good first bug] → [contextMenus][good first bug]triaged
Sounds good to me.
(Assignee)

Comment 11

a year 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)
Duplicate of this bug: 1280555
Attachment #8723939 - Flags: review?(kmaglione+bmo)

Updated

10 months ago
Keywords: good-first-bug
Whiteboard: [contextMenus][good first bug]triaged → [contextMenus]triaged

Updated

8 months ago
Component: WebExtensions: Untriaged → WebExtensions: Frontend
Flags: blocking-webextensions-
Priority: -- → P5
Attachment #8723939 - Flags: review?(kmaglione+bmo)
Duplicate of this bug: 1322488

Comment 15

4 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

2 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

a month 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

26 days 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.
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.
You need to log in before you can comment on or make changes to this bug.