795522 - context-menu should support tab context

Status: RESOLVED INCOMPLETE

(Add-on SDK Graveyard :: General, defect, P2)

Comment 2

[Erik Vold [:erikvold] (please needinfo? me)]

If this looks hard I'll abandon it, but hopefully it's not.

Comment 3

[Sachin Hosmani [:sachin]]

By tab context, you mean a tab in the tab bar, correct?
Adding an item there would place the item along with the current ones: "Reload tab", "Pin tab", "Close tab" etc. ?

Comment 4

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

(In reply to Sachin Hosmani [:sachin] from comment #3)
> By tab context, you mean a tab in the tab bar, correct?
> Adding an item there would place the item along with the current ones:
> "Reload tab", "Pin tab", "Close tab" etc. ?

Yes.

Comment 5

[Sachin Hosmani [:sachin]]

Attachment: Link to pull request

Comment 6

[Erik Vold [:erikvold] (please needinfo? me)]

Comment on attachment 8344153 [details]
Link to pull request

This looks fine, it needs tests though before we can land it.  If you don't think you can get to it, or want help, then let us know!

You can find the context-menu tests here https://github.com/mozilla/addon-sdk/blob/master/test/test-context-menu.js

Comment 7

[Sachin Hosmani [:sachin]]

I updated my pull request.

Comment 8

[Erik Vold [:erikvold] (please needinfo? me)]

Comment on attachment 8344153 [details]
Link to pull request

So we need a few more tests:

    We should test that using a TabContext in combination with other context types also works
    I'm concerned that if one uses the TabContext, then adds a content script to listen to the 'click' event then they will have access to the tab dom element/node which would effectively give the user chrome permissions (without the need for using require('chrome') which is a security hole. Either way we need tests that this is not possible.

Comment 9

[Sachin Hosmani [:sachin]]

Yeah, so I could altogether prevent firing click events on tabs or I could do that only if require('chrome') wasn't used.
Which do you suggest? And is it possible to check if the user has taken chrome permissions via require('chrome')?

Comment 10

[Erik Vold [:erikvold] (please needinfo? me)]

(In reply to Sachin Hosmani [:sachin] from comment #9)
> Yeah, so I could altogether prevent firing click events on tabs or I could
> do that only if require('chrome') wasn't used.
> Which do you suggest? And is it possible to check if the user has taken
> chrome permissions via require('chrome')?

Firing the click event is fine, we should just pass null instead of the tab element though.

Comment 11

[Erik Vold [:erikvold] (please needinfo? me)]

hmm another option is to pass a tab instance from the sdk/tabs module, but that would create an extra dependency I think.

What would you like done Irakli?

Comment 12

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

(In reply to Erik Vold [:erikvold] [:ztatic] from comment #11)
> hmm another option is to pass a tab instance from the sdk/tabs module, but
> that would create an extra dependency I think.
> 
> What would you like done Irakli?

How about ignoring just ignoring `contentScript` option if tab context is used. We could also extend
options passed to `PredicateContext` https://developer.mozilla.org/en-US/Add-ons/SDK/High-Level_APIs/context-menu#PredicateContext%28predicateFunction%29 add tab.id and maybe some more info if
necessary.

Comment 13

[Sachin Hosmani [:sachin]]

Sorry for the delay. I've updated the pull request.

- As Mossop suggested, I've removed TabContext. parentMenu attribute must be used instead.
- contentScript and contentScriptFile are ignored when the tab context is used.
- tab context works with other contexts.

Comment 14

[Dave Townsend [:mossop]]

Comment on attachment 8344153 [details]
Link to pull request

Nice work, this looks great!

Comment 15

[Erik Vold [:erikvold] (please needinfo? me)]

Comment on attachment 8344153 [details]
Link to pull request

I don't see any tests that clicking a menu items will fire some event that developers can do something with.  Afaict if we don't allow the contentScript we can add menu items that are displayed, but I don't see how one can react to the context menu click event.

Comment 16

[Sachin Hosmani [:sachin]]

Since contentScript is ignored here (as Irakli suggested), no handler code can be associated with a context click. So what do we do? Pass a tab instance from sdk/tabs?

Comment 17

[Erik Vold [:erikvold] (please needinfo? me)]

(In reply to Sachin Hosmani [:sachin] from comment #16)
> Since contentScript is ignored here (as Irakli suggested), no handler code
> can be associated with a context click. So what do we do? Pass a tab
> instance from sdk/tabs?

Hmm I'm not sure that I like omitting the contentScript here.  What if I wanted to add a context menu item to tabs which parsed the document in the tab for some data and used that for something, like parsing the document for address and opening google map tabs for each location.

Afaict we can only add doing nothing menu items to the tab context, which is not useful unless we can react to a click on the menu item.

Perhaps we can add a onClick handler for content menus in general, which never gets any arguments, or we can not omit the contentScripts and merely not pass any argument to them for the context, click, etc events.  We could also do both, or something else?

Irakli what would you prefer?

Comment 18

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

(In reply to Erik Vold [:erikvold] [:ztatic] from comment #17)
> (In reply to Sachin Hosmani [:sachin] from comment #16)
> > Since contentScript is ignored here (as Irakli suggested), no handler code
> > can be associated with a context click. So what do we do? Pass a tab
> > instance from sdk/tabs?
> 
> Hmm I'm not sure that I like omitting the contentScript here.  What if I
> wanted to add a context menu item to tabs which parsed the document in the
> tab for some data and used that for something, like parsing the document for
> address and opening google map tabs for each location.
> 
> Afaict we can only add doing nothing menu items to the tab context, which is
> not useful unless we can react to a click on the menu item.
> 
> Perhaps we can add a onClick handler for content menus in general, which
> never gets any arguments, or we can not omit the contentScripts and merely
> not pass any argument to them for the context, click, etc events.  We could
> also do both, or something else?
> 
> Irakli what would you prefer?

There is a different bug to expose more saner API where some JSON-ified info is passed
to a handler when context-menu is about to be displayed. I think that's a way to go
forward. I don't think we should block this for that other bug though.

Comment 19

[Erik Vold [:erikvold] (please needinfo? me)]

(In reply to Irakli Gozalishvili [:irakli] [:gozala] [@gozala] from comment #18)
> (In reply to Erik Vold [:erikvold] [:ztatic] from comment #17)
> > (In reply to Sachin Hosmani [:sachin] from comment #16)
> > > Since contentScript is ignored here (as Irakli suggested), no handler code
> > > can be associated with a context click. So what do we do? Pass a tab
> > > instance from sdk/tabs?
> > 
> > Hmm I'm not sure that I like omitting the contentScript here.  What if I
> > wanted to add a context menu item to tabs which parsed the document in the
> > tab for some data and used that for something, like parsing the document for
> > address and opening google map tabs for each location.
> > 
> > Afaict we can only add doing nothing menu items to the tab context, which is
> > not useful unless we can react to a click on the menu item.
> > 
> > Perhaps we can add a onClick handler for content menus in general, which
> > never gets any arguments, or we can not omit the contentScripts and merely
> > not pass any argument to them for the context, click, etc events.  We could
> > also do both, or something else?
> > 
> > Irakli what would you prefer?
> 
> There is a different bug to expose more saner API where some JSON-ified info
> is passed
> to a handler when context-menu is about to be displayed. I think that's a
> way to go
> forward. I don't think we should block this for that other bug though.

I think we need to block on this then, otherwise shipping this patch as it is would mean tab context menuitems are possible to display but there will be no onclick handler for them.

Am I missing something?

Comment 20

[Erik Vold [:erikvold] (please needinfo? me)]

Comment on attachment 8344153 [details]
Link to pull request

I'll have to r- this patch as it is since I see no way to react to clicks on these menuitems in a safe way.  Let's see what solution Irakli suggests.

My best suggestion to land asap is to allow contentScripts, emit the click event to them, and do not pass them any information.  As best I can tell this is what Irakli is suggesting too.

Comment 21

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Ok I've marked this one as dependent on Bug 773914, which is to expose onClick handler on the context menu API. Also note that with Bug 922558 we do have a PredicateContext that is passed serialized information about the node being clicked, so basically onClick can be easily added by emitting click events with the
same data as given to predicate context.

Comment 22

[Erik Vold [:erikvold] (please needinfo? me)]

Hey Irakli, do we need to keep this one open?

Comment 23

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Yes, if we want to get fix it, although at this point I'd target context-menu@2. Anyhow you reported it so I think it's your call if you still want it.

Comment 24

[Firefox Bug Husbandry Bot]

https://bugzilla.mozilla.org/show_bug.cgi?id=1399562

Comment 25

[Firefox Bug Husbandry Bot]

https://bugzilla.mozilla.org/show_bug.cgi?id=1399562