1370499 - Support WebExtensions bookmark context menus

Status: RESOLVED FIXED

(WebExtensions :: General, enhancement, P5)

Description

[Jonathan Kingston [:jkt] he/him]

We get a lot of requests for context menus to control bookmarks in our containers experiment, this currently isn't possible.

Could we consider:

browser.contextMenus.create({
  id: "work-container",
  title: "Open in work container",
  contexts: ["bookmark"]
});

browser.contextMenus.onClicked.addListener(function(info, tab) {
  console.log(info.bookmark); // {url: "blah.com", tags: [...]}
});

Comment 1

[Caitlin Neiman [:caitmuenster]]

Hi Jonathan, this has been added to the agenda for the July 11 WebExtension APIs triage meeting. Would you be able to join us? 

Wiki: https://wiki.mozilla.org/Add-ons/Contribute/Triage#Next_Meeting

Agenda: https://docs.google.com/document/d/1MEMC7EyZ2VVInbWod7FD2SFGp36osoxQkhUm5f0aNSs/edit#

Comment 2

[Bob Silverberg [:bsilverberg]]

In which places where bookmarks are available did you want to be able to add the context menu? Bookmarks are available in a number of places via a number of mechanisms.

Comment 3

[Jonathan Kingston [:jkt] he/him]

Currently the places that have the menu I think are legitimate to add to are:

- Bookmarks (file) menu.
- Bookmarks sidebar
- Bookmarks toolbar
- Bookmarks sidebar

I'm not familiar with any other areas that I know of, however these could be added in stages. I'm not really sure on priority.

All of these places have individual bookmark items for web pages.

I wouldn't expect the menu item to appear on the folders or in the popout bookmark manage window (I assume the manage window is very different code base).

Comment 4

[Bob Silverberg [:bsilverberg]]

(In reply to Jonathan Kingston [:jkt] from comment #3)
> Currently the places that have the menu I think are legitimate to add to are:
> 
> - Bookmarks (file) menu.
> - Bookmarks sidebar
> - Bookmarks toolbar
> - Bookmarks sidebar
> 

Did you accidentally add "Bookmarks sidebar" twice, or were you thinking of something else?

> I'm not familiar with any other areas that I know of, however these could be
> added in stages. I'm not really sure on priority.
> 
> All of these places have individual bookmark items for web pages.
> 
> I wouldn't expect the menu item to appear on the folders or in the popout
> bookmark manage window (I assume the manage window is very different code
> base).

Thanks for the info, Jonathan.

Caitlin, can you please add this to next week's agenda now that we have more information?

Comment 5

[Caitlin Neiman [:caitmuenster]]

Sure thing! 

Wiki: https://wiki.mozilla.org/Add-ons/Contribute/Triage#Next_Meeting

Agenda: https://docs.google.com/document/d/1gWszBunGAyOJ_V8_HMECXJuZ4Gd_HTM_M7xjDSwSxeo/edit#

Comment 6

[Shane Caraveo (:mixedpuppy)]

Attachment: bookmark toolbar.png

Here's one showing the pocket menu

Comment 7

[Jonathan Kingston [:jkt] he/him]

> Did you accidentally add "Bookmarks sidebar" twice, or were you thinking of
> something else?
> 

Whoops looks like I missed the "Browser action" that shane took a picture of also.

Comment 8

[Jonathan Kingston [:jkt] he/him]

Attachment: Selection_714.png

For clarity here is a screenshot of the context menu. I expected to be able to add an item to this somewhere.

Comment 9

[Bob Silverberg [:bsilverberg]]

Jonathan, from whence does your screenshot originate? When I pull down the Bookmarks menu from the main menu, I don't get any context menus at all for the bookmarks in that list. Right-clicking just opens the bookmark.

Comment 10

[Tim Nguyen :ntim]

(In reply to Bob Silverberg [:bsilverberg] from comment #9)
> Jonathan, from whence does your screenshot originate? When I pull down the
> Bookmarks menu from the main menu, I don't get any context menus at all for
> the bookmarks in that list. Right-clicking just opens the bookmark.

This context menu shows up in the old (non-photon) bookmarks menu, the bookmarks sidebar and the bookmarks toolbar.

Comment 11

[Jonathan Kingston [:jkt] he/him]

Attachment: Selection_715.png

Works for me post photon....
I click the new star icon in the top bar and right click on the pane that comes from it.

Comment 12

[Bob Silverberg [:bsilverberg]]

(In reply to Jonathan Kingston [:jkt] from comment #11)
> Created attachment 8885912 [details]
> Selection_715.png
> 
> Works for me post photon....
> I click the new star icon in the top bar and right click on the pane that
> comes from it.

Right, it works from the Bookmarks button in the toolbar, but does not work from the Bookmarks menu in the main menu, not in photon anyway.

Comment 13

[Jonathan Kingston [:jkt] he/him]

(In reply to Bob Silverberg [:bsilverberg] from comment #12)
> (In reply to Jonathan Kingston [:jkt] from comment #11)
> > Created attachment 8885912 [details]
> > Selection_715.png
> > 
> > Works for me post photon....
> > I click the new star icon in the top bar and right click on the pane that
> > comes from it.
> 
> Right, it works from the Bookmarks button in the toolbar, but does not work
> from the Bookmarks menu in the main menu, not in photon anyway.

Hmm not sure which menu you are speaking of, in Linux both of these work for me:
- Hot dogs > Library > Bookmarks > right click item
- Alt+B "top bookmarks menu"

Comment 14

[Andy McKay]

Sounds like a good idea and no-one had any real objection in the meeting, so let's do this.

Comment 16

[Michal Stanke (Mozilla.cz) [:mstanke][:MikkCZ]]

When can we expect this to be available in WebExtensions? How much work it would be in case if I am interested to implement it as my first bug?

Comment 17

[Tim Nguyen :ntim]

(In reply to Michal Stanke (Mozilla.cz) [:MikkCZ][:mstanke] (use needinfo) from comment #16)
> When can we expect this to be available in WebExtensions? How much work it
> would be in case if I am interested to implement it as my first bug?


If you'd like to work on this, the code for the bookmarks context menu is here:

https://dxr.mozilla.org/mozilla-central/source/browser/components/places/content/placesOverlay.xul#95

And the code for the webextension API is here:

https://dxr.mozilla.org/mozilla-central/source/browser/components/extensions/ext-menus.js
https://dxr.mozilla.org/mozilla-central/source/browser/components/extensions/ext-c-menus.js

You can probably look into bug 1268020 for inspiration.

Comment 18

[Tim Nguyen :ntim]

Attachment: Bug 1370499 - Support WebExtensions bookmark context menus.

Review commit: https://reviewboard.mozilla.org/r/201266/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/201266/

Comment 19

[Tim Nguyen :ntim]

The current patch works on: the bookmarks toolbar, the library menu, the bookmarks subview, and the bookmarks menu.

It doesn't work on the bookmarks sidebar because it is in a different frame.

Comment 20

[Tim Nguyen :ntim]

Try is green: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2df57b30f99c376cda4296b4f2b902ad6a5c6037

Comment 21

[Shane Caraveo (:mixedpuppy)]

ntim, it seems to me this bug is about having context menus that operate on the bookmark, but the patch is providing activeTab data.  I don't think activeTab data is appropriate in this context.  

If you feel otherwise, I think we need to clarify the functionality this bug is about.

Comment 22

[Tim Nguyen :ntim]

Comment on attachment 8930083 [details]
Bug 1370499 - Support WebExtensions bookmark context menus.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/201266/diff/1-2/

Comment 23

[Tim Nguyen :ntim]

Fixed this, also it needs the bookmarks permission now.

Comment 24

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8930083 [details]
Bug 1370499 - Support WebExtensions bookmark context menus.

https://reviewboard.mozilla.org/r/201266/#review206482

::: browser/components/extensions/ext-menus.js:239
(Diff revision 2)
>          }
>          // Select the clicked radio item.
>          item.checked = true;
>        }
>  
>        item.tabManager.addActiveTabPermission();

We'll need to be sure that bookmark context menus don't get into some of the command handling code, such as getting active tab permission.  Be sure to evaluate what data is getting out here.  e.g. the tab data following this may also be an issue.

::: browser/components/extensions/test/browser/browser_ext_contextMenus.js:475
(Diff revision 2)
> +      await browser.contextMenus.create({
> +        title: "Get bookmark",
> +        contexts: ["bookmark"],
> +      });
> +      browser.test.sendMessage("bookmark-created");
> +      browser.contextMenus.onClicked.addListener(async ({bookmarkId}) => {

add a test that activeTab is not given.  also assert that any tab/page data is not included in the details..

Comment 25

[Tim Nguyen :ntim]

Comment on attachment 8930083 [details]
Bug 1370499 - Support WebExtensions bookmark context menus.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/201266/diff/2-3/

Comment 26

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8930083 [details]
Bug 1370499 - Support WebExtensions bookmark context menus.

https://reviewboard.mozilla.org/r/201266/#review206576

Thanks!  Lets add a followup bug to address the context menu in the bookmark sidebar

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/09c62da7c2a5
Support WebExtensions bookmark context menus. r=mixedpuppy

Comment 28

[Tiberius Oros[:tiberius_oros]]

https://hg.mozilla.org/mozilla-central/rev/09c62da7c2a5

Comment 29

[Nye Liu]

Will we be able to get a callback on "Open All in Tabs" to implement Tab Replace?

https://addons.mozilla.org/en-US/firefox/addon/tab-replace/

Comment 30

[Tim Nguyen :ntim]

(In reply to Nye Liu from comment #29)
> Will we be able to get a callback on "Open All in Tabs" to implement Tab
> Replace?
> 
> https://addons.mozilla.org/en-US/firefox/addon/tab-replace/

This bug doesn't allow replacing the built-in menu item, but it makes it possible to add a new "Open All and replace current tabs" context menu item, that works like the one in the add-on.

Comment 31

[Nye Liu]

Any chance this can be pushed up to ff58? Seems like a long wait for even the most basic webext functionality for a TON of now broken extensions.

Comment 32

[Will Bamberg [:wbamberg]]

:ntim updated the docs for this: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/menus/ContextType

Comment 33

[Worcester12345]

Any update here?

Comment 34

[Caitlin Neiman [:caitmuenster]]

(In reply to Worcester12345 from comment #33)
> Any update here?

This is riding the Firefox 59 train and should be available in release on March 13. :) See comment 32 for documentation.

Comment 35

[marius.santa]

I see the context menu as displayed in the screenshots for builds before this fix, please provide some STR and the proper webextension(if required), if No set the “qe-verify-“ flag.

Comment 36

[Nye Liu]

(In reply to Tim Nguyen :ntim from comment #30)
> (In reply to Nye Liu from comment #29)
> > Will we be able to get a callback on "Open All in Tabs" to implement Tab
> > Replace?
> > 
> > https://addons.mozilla.org/en-US/firefox/addon/tab-replace/
> 
> This bug doesn't allow replacing the built-in menu item, but it makes it
> possible to add a new "Open All and replace current tabs" context menu item,
> that works like the one in the add-on.

I'm new at writing web extensions, can you just write a simple code fragment to add an entry to every bookmark *folder*? I can add an entry to the Bookmarks menu, but not the folders. I can probably figure out the rest.

Comment 37

[Nye Liu]

I see the issue: this only works on right click, not left click in bookmarks menu. Is there a way to trap the LEFT CLICK "Open all in tabs" behavior, or add a new entry?