Closed Bug 1768371 Opened 3 years ago Closed 3 years ago

Missing Context menu on "Bookmarks Toolbar", "Bookmarks Menu" and "Other Bookmarks"

Categories

(WebExtensions :: General, enhancement, P2)

Product:
WebExtensions
Component:
General
Version:
Firefox 100
Type:
enhancement

Tracking

(firefox100 wontfix, firefox101 wontfix, firefox102 verified)

Status:
VERIFIED FIXED
Milestone:
102 Branch
Tracking Flags:
Tracking Status
firefox100 --- wontfix
firefox101 --- wontfix
firefox102 --- verified

People

(Reporter: b3a3imewbt, Assigned: b3a3imewbt, Mentored)

References

Details

Attachments

(6 files, 4 obsolete files)

addon-to-display-context_entry-1.0.0.zip
541 bytes, application/zip
Details
Context_Entry_Missing_Actual_Result_capture-2022-05-08_15:34:00.jpg
160.94 KB, image/jpeg
Details
Expected_Result_Context_Entry_Visible_on_(other_Folder)_capture-2022-05-08_15:34:26.jpg
164.18 KB, image/jpeg
Details
show_context_menu_on_virtual_folders.patch
927 bytes, patch
Details | Diff | Splinter Review
Bug 1768371 - Missing Context menu on "Bookmarks Toolbar", "Bookmarks Menu" and "Other Bookmarks" r=robwu
48 bytes, text/x-phabricator-request
Details | Review
2022-05-30_10h44_45.png
84.28 KB, image/png
Details

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:100.0) Gecko/20100101 Firefox/100.0

Steps to reproduce:

Wanted to create an addon to clear/remove the content of Bookmark Folders by utilizing the Web-Extension 'menus+bookmarks' API triggered by selecting a 'Context Menu' Entry on the folder

Actual results:

When right clicking on the "Bookmarks Toolbar", "Bookmarks Menu" or "Other Bookmarks" Folder the Context Menu Entry is not displayed and can not be used to trigger any kind of action.

Expected results:

The Context Menu should be displayed / available to use as a trigger/action on all Bookmark Folders

The menu does not show because the node is filtered out at https://searchfox.org/mozilla-central/rev/df4c90d4b8c92c99f76334acfe4813c573c12661/browser/components/extensions/parent/ext-menus.js#1200

e.g. node.bookmarkGuid for the "Bookmarks menu" item is "menu_______v",
and for the "Toolbar" it's "toolbar____v".
In contrast, node.bookmarkGuid for a right-click on the bookmarks bar is "toolbar_____".

It's unclear to me at this time whether there is clear & reliable logic to determine whether it's possible to map a "virtual" bookmark to a "real" one, but patches are welcome.

If we introduce a mapping for virtual bookmarks to real ones, then bug 1638334 could also be fixed.

See Also: → 1638334

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

I created this very naive and simple patch and tested it with the current nightly and as far as i could see it works.

I am not sure if it meets the quality standard how exactly , but ... i guess its a start.

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

I created this very simple patch and tested it with the current nightly and as far as i could see it works.
I am not sure if it meets the quality standards or if there are other issues, but i guess its something.

PS: I havent submitted any patches before, so any guidance is appreciated.

Thanks for reading.

Please use Phabricator to submit patches: https://moz-conduit.readthedocs.io/en/latest/phabricator-user.html

Before a patch can land, it needs two things:

  1. unit tests that reproduces the issue before the fix, and passes with the fix
  2. the fix itself

Here is an example of a patch that touches a related feature. The changes to the test may be particularly interesting: https://hg.mozilla.org/mozilla-central/rev/6b7a9a7afa56c0cc0f2399bf378cb6d12b73b86a
The file may be changed, so I suggest to use Searchfox.org to look up the current version of the file, to see whether any significant changes/refactorings have happened to get the test to pass.

That file is rather big, so it could certainly make sense to create a new test file (search for the existing test's file name to get an idea about where the file should be registered, it's an ini file). By putting the new test in a new file, you will be able to run your test faster.

Here are more tips and tricks to develop a patch for extension code in Firefox: https://wiki.mozilla.org/WebExtensions/Contribution_Onramp

Now, on the specifics of your patch. Currently you are doing some string manipulation to convert a virtual ID into a valid ID. While that works at the moment, it is not clear whether this applies to all virtual items. Is there an existing method or map where the virtual ID is converted to the actual ID? I suggest to use Searchfox.org to search through the code base for pointers.

Feel free to ask for more help on any of the parts that I described here.

Hello,

I reproduced the issue on the latest Nightly (102.0a1/20220509190429), Beta (101.0b4/20220508185621) and Release (100.0/20220428192727) under Windows 10 x64 and Ubuntu 16.04 LTS, using the provided extension.

The issue occurs exactly as described in comment 0 i.e. when right clicking on the "Bookmarks Toolbar", "Bookmarks Menu" or "Other Bookmarks" folders, the context menu entry is not displayed.

Status: UNCONFIRMED → NEW
status-firefox100: --- → affected
status-firefox101: --- → affected
status-firefox102: --- → affected
Ever confirmed: true
Assignee: nobody → b3a3imewbt
Mentor: rob

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

I think i have setup the moz-phab to a mostly usable state. At least i was able to create a WIP Patch ... which seems to be correctly linked to this issue.

I also searched on searchfox.org for an existing (mapping) function, but i could not find anything , so i added one myself, which now instead of string stuff uses the existing variables. Hope that is resiliant enough for future changes.

BUT i am not sure if i have sufficient knowledge/insight to create adequate unit tests.
I looked at the example you provided and there is a lot of stuff for action invocations a event handling i am not sure i will get right any time soon (or at all)

I will try a bit longer to undestand but if i fail ... it would be nice if someone would be able to support/help out.

I've looked at your patch and noticed that its whitespace is slightly off. Use eslint to check for styles (I have just appended a new section to the contribution wiki page, at https://wiki.mozilla.org/WebExtensions/Contribution_Onramp#Linting).

I also searched on searchfox.org for an existing (mapping) function, but i could not find anything

I couldn't find anything either, so it seems that a new one is indeed needed.

BUT i am not sure if i have sufficient knowledge/insight to create adequate unit tests.
I looked at the example you provided and there is a lot of stuff for action invocations a event handling i am not sure i will get right any time soon (or at all)

The relevant test task that you can copy and modify is https://searchfox.org/mozilla-central/rev/5f88c1d6977e03e22d3420d0cdf8ad0113c2eb31/browser/components/extensions/test/browser/browser_ext_contextMenus.js#719-741. Here is a dissection of the code:

As said before, you can either add a new add_task call with a test, or create a new file that defines a single test (so you'll be able to just run your new test code, instead of all other existing tests, which may take a while). Creating a new test file is as easy as doing the following:

  1. Create a new file, e.g. browser_ext_contextMenus_bookmarks.js. Put meaningful content in it.
  2. Add it to browser.ini (in this case with support-files to be able to import the bookmarks test helpers), like this: https://searchfox.org/mozilla-central/rev/5f88c1d6977e03e22d3420d0cdf8ad0113c2eb31/browser/components/extensions/test/browser/browser.ini#107-108
  3. Run ./mach build to get the build system to recognize the new file.
  4. Run ./mach test browser/components/extensions/test/browser/browser_ext_contextMenus_bookmarks.js to run your new test.

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

As said before, you can either add a new add_task call with a test, or create a new file that defines a single test (so you'll be able to just run your new test code, instead of all other existing tests, which may take a while). Creating a new test file is as easy as doing the following:

  1. Create a new file, e.g. browser_ext_contextMenus_bookmarks.js. Put meaningful content in it.
  2. Add it to browser.ini (in this case with support-files to be able to import the bookmarks test helpers), like this: https://searchfox.org/mozilla-central/rev/5f88c1d6977e03e22d3420d0cdf8ad0113c2eb31/browser/components/extensions/test/browser/browser.ini#107-108
  3. Run ./mach build to get the build system to recognize the new file.
  4. Run ./mach test browser/components/extensions/test/browser/browser_ext_contextMenus_bookmarks.js to run your new test.

You can also use mach addtest.

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

I've looked at your patch and noticed that its whitespace is slightly off. Use eslint to check for styles (I have just appended a new section to the contribution wiki page, at https://wiki.mozilla.org/WebExtensions/Contribution_Onramp#Linting).

I assume that i can use the ./mach lint command for this.

I also searched on searchfox.org for an existing (mapping) function, but i could not find anything

I couldn't find anything either, so it seems that a new one is indeed needed.

I am just a bit unsure if the place where i put the function is correct. I placed it where i found the variables it uses to resolve the mapping.

BUT i am not sure if i have sufficient knowledge/insight to create adequate unit tests.
I looked at the example you provided and there is a lot of stuff for action invocations a event handling i am not sure i will get right any time soon (or at all)

The relevant test task that you can copy and modify is https://searchfox.org/mozilla-central/rev/5f88c1d6977e03e22d3420d0cdf8ad0113c2eb31/browser/components/extensions/test/browser/browser_ext_contextMenus.js#719-741. Here is a dissection of the code:

As said before, you can either add a new add_task call with a test, or create a new file that defines a single test (so you'll be able to just run your new test code, instead of all other existing tests, which may take a while). Creating a new test file is as easy as doing the following:

  1. Create a new file, e.g. browser_ext_contextMenus_bookmarks.js. Put meaningful content in it.
  2. Add it to browser.ini (in this case with support-files to be able to import the bookmarks test helpers), like this: https://searchfox.org/mozilla-central/rev/5f88c1d6977e03e22d3420d0cdf8ad0113c2eb31/browser/components/extensions/test/browser/browser.ini#107-108
  3. Run ./mach build to get the build system to recognize the new file.
  4. Run ./mach test browser/components/extensions/test/browser/browser_ext_contextMenus_bookmarks.js to run your new test.

I'll try and work my way though this. Thanks for explaining it in a step by step way.

(In reply to kernp25 from comment #11)

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

As said before, you can either add a new add_task call with a test, or create a new file that defines a single test (so you'll be able to just run your new test code, instead of all other existing tests, which may take a while). Creating a new test file is as easy as doing the following:

  1. Create a new file, e.g. browser_ext_contextMenus_bookmarks.js. Put meaningful content in it.
  2. Add it to browser.ini (in this case with support-files to be able to import the bookmarks test helpers), like this: https://searchfox.org/mozilla-central/rev/5f88c1d6977e03e22d3420d0cdf8ad0113c2eb31/browser/components/extensions/test/browser/browser.ini#107-108
  3. Run ./mach build to get the build system to recognize the new file.
  4. Run ./mach test browser/components/extensions/test/browser/browser_ext_contextMenus_bookmarks.js to run your new test.

You can also use mach addtest.

That looks a little easier (at first glance)
I'll definitely check that out.
Thanks for pointing that out.

The cmd should look like:
./mach addtest browser/components/extensions/test/browser/browser_ext_contextMenus_bookmarks.js

(In reply to kernp25 from comment #14)

The cmd should look like:
./mach addtest browser/components/extensions/test/browser/browser_ext_contextMenus_bookmarks.js

mach addtest basically does steps 1 and part of step 2 from my list in comment 10.
The addtest command doesn't automatically add the support-files section necessary for this test, so I still recommend to manually edit the files.

Attached file WIP: Bug 1768371 - Fix eslint issues (obsolete) —

Depends on D145955

Attached file WIP: Bug 1768371 - Add a Test (obsolete) —

Depends on D145982

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

I followed your instructions and created a hopefully adequat test.
There are 2 questions i came across, which i hope you can give me some insight on.

  1. 'mobile____'
    I had an issue with the 'mobile_____' bookmark folder, which is also marked as a virtual folder, but in comparision to 'toolbar,menu and unfiled' it doenst seem to exist initally. That is why i had to exclude it from test. If you know of a way to trigger its creation it can of course re-add it.

  2. Bookmark Library
    Would it make sense to also add a test which checks the existens of the Context Menu Entry inside the Bookmark Library or
    can it be assumed that the Context Menu of the Sidebar is always the same?

PS:
This is an interesting journey.
And thank you again for helping out.

Could you mark the follow-up patches as "Abandoned", and re-use the first one?

For instructions on correctly updating the revision (and setting the reviewer), see https://wiki.mozilla.org/WebExtensions/Contribution_Onramp#Submitting_a_Patch

I had an issue with the 'mobile_____' bookmark folder, which is also marked as a virtual folder, but in comparision to 'toolbar,menu and unfiled' it doenst seem to exist initally. That is why i had to exclude it from test. If you know of a way to trigger its creation it can of course re-add it.

I wouldn't worry about this for now. I haven't checked, but I guess that it only appears when Sync is enabled and synchronizing bookmarks across devices.

Would it make sense to also add a test which checks the existens of the Context Menu Entry inside the Bookmark Library or
can it be assumed that the Context Menu of the Sidebar is always the same?

The library and the sidebar are not always the same. You can look at existing code if you'd like to create a new test if you'd like, but for now let's focus on the current task.

Attachment #9275976 - Attachment is obsolete: true
Attachment #9275888 - Attachment is obsolete: true
Attachment #9275841 - Attachment is obsolete: true

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

Could you mark the follow-up patches as "Abandoned", and re-use the first one?

I rebased everything into one commit and re-supplied that as a new patch with you as the reviewer.
Hope that was ok.

I wouldn't worry about this for now. I haven't checked, but I guess that it only appears when Sync is enabled and synchronizing bookmarks across devices.

Ok, just though i should mention it since it was one of the virtual folders too.

The library and the sidebar are not always the same. You can look at existing code if you'd like to create a new test if you'd like, but for now let's focus on the current task.

Guess that is something i'll need to look into later.
Thanks for letting me know that there is a difference, since from pure manual testing i would have assumed these to always be the same.

Attachment #9276558 - Attachment is obsolete: true
Severity: -- → N/A
Type: defect → enhancement
Priority: -- → P2

It looks like the patch i created reopens this bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1597142
I only noticed it because intened to add a testcase for the "Bookmark Library" anyway.

I already implemented a patch for it which fixes the issue by using:
https://searchfox.org/mozilla-central/source/toolkit/components/places/PlacesUtils.jsm#478
but i am struggeling again with the creating of the test.

I was able to open the bookmarks library by looking at another testcase like this:

add_task(async function test_bookmark_library_sidebar_contextmenu() {
  let bookmarksLibraryOpened = promiseOpenBookmarksLibrary();
  await EventUtils.synthesizeKey("o", { shiftKey: true, accelKey: true });
  let win = await bookmarksLibraryOpened;
  ok(true, "Bookmarks library successfully opened.");

But now i dont quite know how to find the correct selectors in the window object to get to the sidebar element.

If found this: https://searchfox.org/mozilla-central/source/browser/components/places/content/places.xhtml
and i am assuming that i have to do something similar to

win.getElementById('placesList');

but i'm struggeling how to get this right.
If you could give me a hint how to best approch this, i will try and fix it.

Flags: needinfo?(rob)

(In reply to igorlogius from comment #23)

It looks like the patch i created reopens this bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1597142
I only noticed it because intened to add a testcase for the "Bookmark Library" anyway.

I already implemented a patch for it which fixes the issue by using:
https://searchfox.org/mozilla-central/source/toolkit/components/places/PlacesUtils.jsm#478
but i am struggeling again with the creating of the test.

This issue looks like bug 1638334, where I suggested a similar fix at https://bugzilla.mozilla.org/show_bug.cgi?id=1638334#c6. It would be really nice if you update the patch to avoid regressing on bug 1597142 :)

But now i dont quite know how to find the correct selectors in the window object to get to the sidebar element.

The new test added in this patch shows how you can open the library window and access specific tree views within it (also note the addition to globals, to make eslint happy):
https://hg.mozilla.org/mozilla-central/rev/8b35181c3ccc747106037b61a9b52adfbba5ed60#l2.2

The current version of that code can be viewed at https://searchfox.org/mozilla-central/rev/70cf6863bd85af2a3188ec1fe5209a3ec1b2de86/browser/components/extensions/test/browser/browser_ext_contextMenus.js#784,786-816

Flags: needinfo?(rob)

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

Thank your, for the links and hints.
I updated the patch and now it should avoid the mentioned regression.
If you find anything else in it that needs improving, just let me know and i'll fix it up.

Pushed by archaeopteryx@coole-files.de: https://hg.mozilla.org/integration/autoland/rev/e867dcd6be8f Missing Context menu on "Bookmarks Toolbar", "Bookmarks Menu" and "Other Bookmarks" r=robwu

https://hg.mozilla.org/mozilla-central/rev/e867dcd6be8f

Status: NEW → RESOLVED
Closed: 3 years ago
status-firefox102: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → 102 Branch

Verified the fix using the attached extension (Comment 0) on the latest Nightly (102.0a1/20220529190847) under Windows 10 x64 and Ubuntu 16.04 LTS.

With the extensions loaded in the browser, right clicking on the "Bookmarks Toolbar", "Bookmarks Menu" or "Other Bookmarks" folders in the sidebar, the context menu entry is properly displayed, confirming the fix.

For more details, see the attached screenshot.

Status: RESOLVED → VERIFIED
status-firefox102: fixed → verified
Attached image 2022-05-30_10h44_45.png —
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: