The default bug view has changed. See this FAQ.

Implement onBeforeShow event

NEW
Assigned to
(NeedInfo from)

Status

()

Toolkit
WebExtensions: Frontend
P5
normal
2 years ago
15 days ago

People

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

Tracking

({good-first-bug})

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [contextMenus]triaged)

Attachments

(2 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

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

Updated

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

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

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)
(Reporter)

Comment 5

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

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

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

a year ago
Whiteboard: [contextMenus][good first bug] → [contextMenus][good first bug]triaged
(Reporter)

Comment 10

a year ago
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)
(Reporter)

Updated

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

Updated

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

Updated

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

Updated

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

Updated

4 months ago
Duplicate of this bug: 1322488

Comment 15

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