WebExtension bookmarks context menu items should appear in bookmarks sidebar and library window

RESOLVED FIXED in Firefox 66

Status

defect
P5
normal
RESOLVED FIXED
2 years ago
2 months ago

People

(Reporter: ntim, Assigned: robwu)

Tracking

(Blocks 1 bug, {dev-doc-complete})

unspecified
mozilla66
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox66 fixed)

Details

Attachments

(4 attachments, 2 obsolete attachments)

(Reporter)

Description

2 years ago
The API introduced in bug 1370499 should work for the library window and the bookmarks sidebar

Updated

2 years ago
Priority: -- → P5

Updated

11 months ago
Product: Toolkit → WebExtensions
Comment hidden (mozreview-request)

Updated

10 months ago
Attachment #8993306 - Flags: review?(mixedpuppy)

Updated

10 months ago
Attachment #8993307 - Flags: review?(mixedpuppy)
(Reporter)

Comment 3

10 months ago
Thanks for working on this Peter!
Assignee: nobody → pts+bmo
Sorry for the delay.  I checked in with product to ensure that these sidebars were not going to have any significant changes in the foreseeable future.  mconcoa got me the ok.  I'll get to reviews soon.

Comment 5

10 months ago
mozreview-review
Comment on attachment 8993307 [details]
Bug 1419195: Show items from WebExtensions in Places Library context menu

https://reviewboard.mozilla.org/r/258078/#review268814

::: browser/components/extensions/parent/ext-menus.js:753
(Diff revision 1)
> +      }
> +    }
> +  },
> +
> +  uninit() {
> +    Services.ww.unregisterNotification(this);

Do we also need to remove the popupshowing listener during unregister?
Attachment #8993307 - Flags: review?(mixedpuppy)

Comment 6

10 months ago
mozreview-review
Comment on attachment 8993306 [details]
Bug 1419195: Show items from WebExtensions in bookmarks sidebar context menu

https://reviewboard.mozilla.org/r/258076/#review268816

::: browser/components/extensions/parent/ext-menus.js:752
(Diff revision 1)
> +    const browser = window.document.getElementById("sidebar");
> +    browser.addEventListener("load", menuTracker.onSidebarLoad,
> +                             {capture: true}); // Load events don't bubble.
> +    if (window.sidebar.document.readyState === "complete") {
> +      menuTracker.onSidebarLoad({currentTarget: browser});
> +    }

I'm not sure of the long term use of window.sidebar, just use use "window.SidebarUI.*" (see browser-sidebar.js)

::: browser/components/extensions/parent/ext-menus.js:765
(Diff revision 1)
> +  cleanupWindow(window) {
> +    for (const id of this.menuIds) {
> +      const menu = window.document.getElementById(id);
> +      menu.removeEventListener("popupshowing", this);
> +    }
> +    const browser = window.document.getElementById("sidebar");

window.SidebarUI.browser

::: browser/components/extensions/parent/ext-menus.js:768
(Diff revision 1)
> +    const URL = window.document.getElementById("viewBookmarksSidebar")
> +                               .getAttribute("sidebarurl");
> +    if (window.sidebar.location.href === URL) {

Just check window.SidebarUI.currentID == "viewBookmarksSidebar".  Ditto in onSidebarLoad.
Attachment #8993306 - Flags: review?(mixedpuppy)

Comment 7

9 months ago
Posted patch sidebar.patchSplinter Review
Attachment #8993306 - Attachment is obsolete: true
Attachment #9006037 - Flags: review?(mixedpuppy)

Comment 8

9 months ago
Posted patch library.patchSplinter Review
Depends on the sidebar patch
Attachment #8993307 - Attachment is obsolete: true
Attachment #9006038 - Flags: review?(mixedpuppy)

Comment 9

9 months ago
Rebasing was a bit tricky, but I *think* I got everything.

(In reply to Tim Nguyen :ntim from comment #3)
> Thanks for working on this Peter!

Happy to :)

(In reply to Shane Caraveo (:mixedpuppy) from comment #5)
> Do we also need to remove the popupshowing listener during unregister?

Yes, you're right.  I just didn't think about the library window being open during unregister, but of course it could be.
Comment on attachment 9006037 [details] [diff] [review]
sidebar.patch

I'm good with this, just going to ask rob to take a second look since he's heavily involved in the context menu code.
Attachment #9006037 - Flags: review?(rob)
Attachment #9006037 - Flags: review?(mixedpuppy)
Attachment #9006037 - Flags: review+
Comment on attachment 9006038 [details] [diff] [review]
library.patch

I'm good with this, just going to ask rob to take a second look since he's heavily involved in the context menu code.
Attachment #9006038 - Flags: review?(rob)
Attachment #9006038 - Flags: review?(mixedpuppy)
Attachment #9006038 - Flags: review+
(Assignee)

Comment 12

9 months ago
Comment on attachment 9006037 [details] [diff] [review]
sidebar.patch

Review of attachment 9006037 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/extensions/parent/ext-menus.js
@@ +777,5 @@
>        const menu = window.document.getElementById(id);
>        menu.addEventListener("popupshowing", menuTracker);
>      }
> +
> +    const browser = window.SidebarUI.browser;

SidebarUI.browser is a lazy getter, so let's try to avoid touching it if not needed (not just this line, but in your whole patch):
https://searchfox.org/mozilla-central/rev/5a18fb5aeeec99f1ca1c36a697082c221189a3b9/browser/base/content/browser-sidebar.js#38-44

Instead of the "load" event on browser, use the SidebarShown event on the sidebar-switcher-target element to detect when the sidebar is shown, and then attach the popupshowing listener to the placesContext if needed.

@@ +806,5 @@
> +    const window = event.currentTarget.ownerGlobal;
> +    if (window.SidebarUI.currentID === "viewBookmarksSidebar") {
> +      const menu = window.SidebarUI.browser.contentDocument
> +                         .getElementById("placesContext");
> +      menu.addEventListener("popupshowing", menuTracker);

Use `menuTracker.handleSidebarCtxMenu` (please rename this, e.g. to handleBookmarksSidebarMenu). Then you don't need the extra check in the handleEvent method below.
Attachment #9006037 - Flags: review?(rob)
(Assignee)

Comment 13

9 months ago
Comment on attachment 9006038 [details] [diff] [review]
library.patch

Review of attachment 9006038 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/extensions/parent/ext-menus.js
@@ +764,5 @@
> +    Services.ww.registerNotification(this);
> +
> +    // See WindowTrackerBase#*browserWindows in ext-tabs-base.js for why we
> +    // can't use the enumerator's windowtype filter.
> +    let e = Services.wm.getEnumerator("");

Please use a for-of loop. We recently added support for ES6 iterators in: https://bugzilla.mozilla.org/show_bug.cgi?id=1484496

@@ +781,5 @@
> +  // cleanupWindow is called on any library window that's open.
> +  uninit(cleanupWindow) {
> +    Services.ww.unregisterNotification(this);
> +
> +    let e = Services.wm.getEnumerator(this.libraryWindowType);

Also here, please use a for-of loop.

@@ +785,5 @@
> +    let e = Services.wm.getEnumerator(this.libraryWindowType);
> +    while (e.hasMoreElements()) {
> +      let window = e.getNext();
> +      try {
> +        cleanupWindow(window);

You should also unregister the "load" listeners that you've added in init/observe.

@@ +886,5 @@
>    },
>  
> +  onLibraryOpen(window) {
> +    const menu = window.document.getElementById("placesContext");
> +    menu.addEventListener("popupshowing", menuTracker);

Similarly to the other patch: If you pass the event (menuTracker.handlePlacesXXX) directly, then you don't need to have a special case in handleEvent.

::: browser/components/extensions/test/browser/browser_ext_contextMenus.js
@@ +623,5 @@
> +        browser.contextMenus.create({
> +          title: "Get bookmark",
> +          contexts: ["bookmark"],
> +        }, resolve));
> +      browser.test.sendMessage("bookmark-created", newBookmark.id);

Move this after the browser.contextMenus.onClicked listener. Otherwise we will face intermittent test failures.
Attachment #9006038 - Flags: review?(rob) → review-
(Assignee)

Comment 14

8 months ago
Peter, have you seen the above review response? Just a couple of changes and then it's good to go.
Flags: needinfo?(pts+bmo)
(Reporter)

Comment 15

8 months ago
(In reply to Rob Wu [:robwu] from comment #12)
> Comment on attachment 9006037 [details] [diff] [review]
> sidebar.patch
> 
> Review of attachment 9006037 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/components/extensions/parent/ext-menus.js
> @@ +777,5 @@
> >        const menu = window.document.getElementById(id);
> >        menu.addEventListener("popupshowing", menuTracker);
> >      }
> > +
> > +    const browser = window.SidebarUI.browser;
> 
> SidebarUI.browser is a lazy getter, so let's try to avoid touching it if not
> needed (not just this line, but in your whole patch):
> https://searchfox.org/mozilla-central/rev/
> 5a18fb5aeeec99f1ca1c36a697082c221189a3b9/browser/base/content/browser-
> sidebar.js#38-44
> 
> Instead of the "load" event on browser, use the SidebarShown event on the
> sidebar-switcher-target element to detect when the sidebar is shown, and
> then attach the popupshowing listener to the placesContext if needed.

This might not be needed to address with bug 1494103.

Comment 16

8 months ago
(In reply to Rob Wu [:robwu] from comment #14)
Thanks, Rob.  I'll have a chance to fix these up (and read bug 1494103) this weekend.  (It was going to be last weekend, but we've just had a major power outage that scrapped all my normal plans.)
(Assignee)

Comment 17

7 months ago
Hi Peter,

Firefox 64 will branch off soon (https://wiki.mozilla.org/Release_Management/Calendar), so to get this functionality in Firefox 64, the patches need to land next week.

Do you have time time finish these patches next week?
If not, then I am willing to take over and fix the last bits so they can land in 64.

Comment 18

7 months ago
(In reply to Rob Wu [:robwu] from comment #17)
> Hi Peter,
> 
> Firefox 64 will branch off soon
> (https://wiki.mozilla.org/Release_Management/Calendar), so to get this
> functionality in Firefox 64, the patches need to land next week.
> 
> Do you have time time finish these patches next week?
> If not, then I am willing to take over and fix the last bits so they can
> land in 64.
Does anyone know if this happened?
(Assignee)

Comment 19

6 months ago
^ That has not happened. This feature can land in 65, at the earliest.
(Reporter)

Comment 20

6 months ago
Hi Rob, Is this something you could finish up ?
Flags: needinfo?(rob)
(Assignee)

Comment 21

6 months ago
I'm awaiting Peter's response. In the interest of moving forwards: If there is no response within two weeks, I'll finish the patches.
Flags: needinfo?(rob)
(Assignee)

Comment 22

5 months ago
Nothing is happening here, and the patch slipped yet another release.
I'll take over the bug to ensure that the fixes become part of Firefox 66.
Assignee: pts+bmo → rob
(Assignee)

Comment 25

4 months ago

I've moved Peter's patches to Phabricator, rebased on the latest checkout, without modifications.
I'll then apply my new (minimal) changes, to make review easier.

Flags: needinfo?(pts+bmo)
(Assignee)

Updated

4 months ago
See Also: → 1515810

Comment 26

4 months ago
Pushed by rob@robwu.nl:
https://hg.mozilla.org/integration/autoland/rev/ee53bdc5b1d4
Show items from WebExtensions in bookmarks sidebar context menu r=mixedpuppy
https://hg.mozilla.org/integration/autoland/rev/732184f122e3
Show items from WebExtensions in Places Library context menu r=mixedpuppy

Comment 28

4 months ago
Backout by nerli@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2f97d93c3a2d
Backed out changeset 732184f122e3 for failing in browser_ext_contextMenus.js
(Assignee)

Updated

4 months ago
See Also: → 1520047
(Assignee)

Comment 29

4 months ago

The patch includes a new test that triggers the failures of comment 27, but the cause is pre-existing. I can reproduce the leaks without any extension code - see bug 1520047.

I'm going to reland the patches with the test being disabled on debug + TV builds.

Flags: needinfo?(rob)

Comment 30

4 months ago
Pushed by rob@robwu.nl:
https://hg.mozilla.org/integration/autoland/rev/d9a81de35ac3
Show items from WebExtensions in bookmarks sidebar context menu r=mixedpuppy
https://hg.mozilla.org/integration/autoland/rev/c04c2376a213
Show items from WebExtensions in Places Library context menu r=mixedpuppy
(Assignee)

Comment 31

4 months ago

Patches are relanding, with one specific part of a test task disabled due to bug 1520047 .

Green try run is here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4ff4d4b1c3ba2c566c31cc76793b141bd2c601d9

Backed out 3 changesets (bug 1515810, bug 1419195) for failing at /browser/browser_ext_contextMenus.js on a CLOSED TREE

Backout link: https://hg.mozilla.org/integration/autoland/rev/65b6d0b670b410489153b2086d8dc1a1b66a3231

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&revision=437003de9fffafd6656b79ea4bcbeab0aa652eef

Log link: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=221984836&repo=autoland&lineNumber=4290

Log snippet:

06:40:03 INFO - Extension loaded
06:40:03 INFO - Console message: Warning: attempting to write 7793 bytes to preference extensions.webextensions.uuids. This is bad for general performance and memory usage. Such an amount of data should rather be written to an external file. This preference will not be sent to any content processes.
06:40:03 INFO - Buffered messages logged at 06:38:49
06:40:03 INFO - Console message: [JavaScript Error: "TypeError: tree.boxObject.getCellAt is not a function" {file: "chrome://browser/content/parent/ext-menus.js" line: 1044}]
06:40:03 INFO - Buffered messages finished
06:40:03 INFO - TEST-UNEXPECTED-FAIL | browser/components/extensions/test/browser/browser_ext_contextMenus.js | Test timed out -
06:40:03 INFO - Not taking screenshot here: see the one that was previously logged
06:40:03 INFO - TEST-UNEXPECTED-FAIL | browser/components/extensions/test/browser/browser_ext_contextMenus.js | no tasks awaiting on messages - Got ["test-finish"], expected []
06:40:03 INFO - Stack trace:
06:40:03 INFO - chrome://mochikit/content/browser-test.js:test_is:1318
06:40:03 INFO - chrome://mochikit/content/tests/SimpleTest/ExtensionTestUtils.js:ExtensionTestUtils.loadExtension/<:31
06:40:03 INFO - chrome://mochikit/content/browser-test.js:nextTest:707
06:40:03 INFO - chrome://mochikit/content/browser-test.js:timeoutFn:1205
06:40:03 INFO - setTimeout handlerchrome://mochikit/content/browser-test.js:Tester_execTest:1167
06:40:03 INFO - chrome://mochikit/content/browser-test.js:nextTest/<:997
06:40:03 INFO - chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<:803
06:40:03 INFO - Not taking screenshot here: see the one that was previously logged
06:40:03 INFO - TEST-UNEXPECTED-FAIL | browser/components/extensions/test/browser/browser_ext_contextMenus.js | Extension left running at test shutdown -
06:40:03 INFO - Stack trace:
06:40:03 INFO - chrome://mochikit/content/browser-test.js:test_ok:1307
06:40:03 INFO - chrome://mochikit/content/tests/SimpleTest/ExtensionTestUtils.js:ExtensionTestUtils.loadExtension/<:109
06:40:03 INFO - chrome://mochikit/content/browser-test.js:nextTest:707
06:40:03 INFO - chrome://mochikit/content/browser-test.js:timeoutFn:1205
06:40:03 INFO - setTimeout handler
chrome://mochikit/content/browser-test.js:Tester_execTest:1167
06:40:03 INFO - chrome://mochikit/content/browser-test.js:nextTest/<:997
06:40:03 INFO - chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<:803
06:40:03 INFO - GECKO(835) | MEMORY STAT | vsize 4637MB | residentFast 614MB | heapAllocated 99MB
06:40:03 INFO - TEST-OK | browser/components/extensions/test/browser/browser_ext_contextMenus.js | took 90139ms
06:40:03 INFO - Not taking screenshot here: see the one that was previously logged
06:40:03 INFO - TEST-UNEXPECTED-FAIL | browser/components/extensions/test/browser/browser_ext_contextMenus.js | Found a tab after previous test timed out: http://mochi.test:8888/browser/browser/components/extensions/test/browser/context.html -
06:40:03 INFO - GECKO(835) | ++DOCSHELL 0x10b91f800 == 1 [pid = 844] [id = {c8913f40-b046-034d-afa7-d2c731357112}]
06:40:03 INFO - GECKO(835) | ++DOMWINDOW == 1 (0x10b987c00) [pid = 844] [serial = 40] [outer = 0x0]
06:40:03 INFO - GECKO(835) | ++DOMWINDOW == 2 (0x10b9ebc00) [pid = 844] [serial = 41] [outer = 0x10b987c00]
06:40:03 INFO - GECKO(835) | ++DOMWINDOW == 3 (0x11afcdc00) [pid = 844] [serial = 42] [outer = 0x10b987c00]
06:40:03 INFO - checking window state

Flags: needinfo?(rob)
(Assignee)

Comment 33

4 months ago

The failure is caused by the refactor in bug 1482389. Patches for that bug landed between my try push and autoland.

I'm rebasing the patch and will reland if try is green again.

Flags: needinfo?(rob)

Comment 34

4 months ago
Pushed by rob@robwu.nl:
https://hg.mozilla.org/integration/autoland/rev/6b7a9a7afa56
Show items from WebExtensions in bookmarks sidebar context menu r=mixedpuppy
https://hg.mozilla.org/integration/autoland/rev/8b35181c3ccc
Show items from WebExtensions in Places Library context menu r=mixedpuppy

Comment 35

4 months ago
bugherder
Status: NEW → RESOLVED
Last Resolved: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66

Comment 36

4 months ago
Flags: needinfo?(rob)
(Assignee)

Comment 37

4 months ago

Thanks kernp25. This should also be documented in the Add-ons section of the Firefox 66 for developers article @ https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/66#Changes_for_add-on_developers

Flags: needinfo?(rob)
Keywords: dev-doc-needed

Comment 38

3 months ago

Will this bug require manual validation from the QA team? I've looked over the description but not sure where exactly the context menu should appear for the sidebar and library, could you please provide more details? Thanks.

Flags: needinfo?(rob)
(Assignee)

Comment 39

3 months ago

Sidebar: Ctrl-B to open bookmark sidebar, expand a bookmark folder and right-click on a bookmark.
Library: Ctrl-Shift-O (or Bookmarks > Show All Bookmarks), right-click on a bookmark.

To test, you could use any add-on that registers a bookmark menu item, e.g. https://addons.mozilla.org/en-US/firefox/addon/open-bookmark-in-container-tab/

The feature has automated tests, and I just verified on Nightly that the menu appears as expected, so qe-verify-.

Flags: needinfo?(rob) → qe-verify-

Comment 40

2 months ago

Added the following paragraph to the context menus page:

In Firefox 66 and later, context menus defined by a an extension can also appear in the Bookmarks Sidebar (Ctrl + B) or the Library window (Ctrl + Shift + B). For example, the extension "Open bookmark in Container Tab" allows the user to open a bookmark URL in a new container tab

I have included the GitHub link for the extension as requested in IRC and added an image showing the Open in New Container Tab context menu opened over the Bookmarks sidebar.

I also added this to the Firefox 66 release notes under API changes/Menus:

"Context menus added by an extension will appear in the Bookmarks sidebar (Ctrl + B) or Library window (Ctrl + Shift + B) (bug 1419195)."

Let me know if this is enough.

Flags: needinfo?(rob)
(Assignee)

Comment 41

2 months ago

Thanks for the pararaph and screenshot at https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/user_interface/Context_menu_items

I moved the version-specific information to the "bookmarks" type at https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/menus/ContextType (here is the difference), and reworded the paragraph at the "Context menu items" tutorial to be a bit more generic (and reference the ContextType article for further reference).

I also tweaked the release notes to add a reference to the ContextType article to improve its discoverability:
"Extension menu items of the "bookmark" type will also appear in the Bookmarks sidebar (Ctrl + B) and Library window (Ctrl + Shift + B) (bug 1419195)."

Flags: needinfo?(rob) → needinfo?(ismith)
(Assignee)

Updated

2 months ago
Flags: needinfo?(ismith)

Comment 42

2 months ago

(In reply to Rob Wu [:robwu] from comment #41)

Thanks for the pararaph and screenshot at https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/user_interface/Context_menu_items

I moved the version-specific information to the "bookmarks" type at https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/menus/ContextType (here is the difference), and reworded the paragraph at the "Context menu items" tutorial to be a bit more generic (and reference the ContextType article for further reference).

I also tweaked the release notes to add a reference to the ContextType article to improve its discoverability:
"Extension menu items of the "bookmark" type will also appear in the Bookmarks sidebar (Ctrl + B) and Library window (Ctrl + Shift + B) (bug 1419195)."

Thank you!

You need to log in before you can comment on or make changes to this bug.