Open Bug 1643886 Opened 5 years ago Updated 1 year ago

Support "history" contexts in browser.contextMenus API

Categories

(WebExtensions :: General, enhancement, P5)

Product:
WebExtensions
Component:
General
Version:
76 Branch
Type:
enhancement

Tracking

(Not tracked)

Status:
ASSIGNED

People

(Reporter: 86Legacy, Assigned: kernp25)

References

Details

Attachments

(2 files)

history-views.ogv
3.47 MB, video/ogg
Details
Bug 1643886 - Support history contexts in browser.menus. r?robwu
48 bytes, text/x-phabricator-request
Details | Review

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:76.0) Gecko/20100101 Firefox/76.0

Expected results:

There should be a context for the history view, but there is not - at least according to https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/menus/ContextType

That article does not list "history" as a context type. "History" on that page only appears in the sidebar, which is a list of APIs, of which history is one.

Do you have a use case for this feature request? Despite the incorrect premise in the first comment, we are willing to accept this feature request if there are use cases for it.

Hmm, what is the incorrect premise you are writing about? From my understanding your own post is the first comment, are you referencing to my description as being the first comment, or was the comment you are referring to rmoved in the meantime?

The use case is to send user-selected urls to an addon for processing. It is easier for the developer and has a better performance compared to reading all urls from history, create some view for it and make the user select from that view.

(In reply to 86Legacy from comment #2)

Hmm, what is the incorrect premise you are writing about? From my understanding your own post is the first comment, are you referencing to my description as being the first comment, or was the comment you are referring to rmoved in the meantime?

I was referring to your comment of the original report. But I might have misread your last sentence, as "There should be a context for the history view according to <documentation link>", while you probably meant "there is not [a context for the history view] according to <documentation link>".

The use case is to send user-selected urls to an addon for processing. It is easier for the developer and has a better performance compared to reading all urls from history, create some view for it and make the user select from that view.

This does sound reasonable, but as it's not high on the priority list, it's probably not going to be implemented anytime soon, unless someone provides a patch. I'm willing to provide pointers if you're interested.

There are three locations in desktop Firefox where a history item can appear (and the implementation in ext-menus.js already has similar logic to support context menus for bookmarks, added in bug 1370499 and bug 1419195):

  • History sidebar (Ctrl-H)
  • History menu (toolbar > History)
  • History library (Ctrl-Shift-H)

Yes, the second one is what I intended to say.

From my understanding
History menu (toolbar > History)
and
History library (Ctrl-Shift-H)
are just two ways to the same location, aren't they?

You're very welcome to provide pointers, knowing close to nothing about the FF codebase I am not sure how helpful this would be. On the other hand your pointers might be helpful for other readers.

Nevermind, I was thinking about the menu bar - until now I did not know there was a toolbar icon for history.
I notice that for the toolbar-history it is actually possible already to "Send to webextension" for items, however you can only select a single item, not multiple ones.
For the sidebar it seems that is not possible to select multiple Elements either - so the really useful one would be the history library.

However, the two extensions that are shown in my toolbar-history seem to be disfunction in that place - maybe the developers do not even know that the context menu entries are shown there.

Actually, make that five, since the library may appear in multiple locations. Despite them being similar, the implementation may differ.

  • History sidebar (Ctrl-H)
  • History menu (alt to reveal the toolbar menu > menu bar > History)
  • History library window (Ctrl-Shift-H)
  • History library via toolbar (the fallen books icon in the toolbar)
  • History library (the fallen books icon in the hamburger menu).

The implementation of bookmarks and history items is very similar in Firefox - they are called "places".

However, the two extensions that are shown in my toolbar-history seem to be disfunction in that place - maybe the developers do not even know that the context menu entries are shown there.

Since the menu is already shown there, a good first step to understanding the code base is:

  1. Open the Browser Toolbox (https://developer.mozilla.org/en-US/docs/Tools/Browser_Toolbox).
  2. Select the ext-menus.js file (Ctrl-P, "ext-menus.js")
  3. Put a breakpoint on the code that is responsible for rendering: https://searchfox.org/mozilla-central/rev/3d39d3b7dd1b2be30692d4541ea681614e34c786/browser/components/extensions/parent/ext-menus.js#64
  4. Optional, but useful since we're debugging menus: Disable popup auto-hide (https://developer.mozilla.org/en-US/docs/Tools/Browser_Toolbox#Debugging_popups )
  5. Open the library menu to reveal the history view.
  6. Right-click on a history item to trigger the context menu.
  7. The Browser Toolbox will now pause on a breakpoint. Click on the call stack to inspect the caller and the values of relevant variables.

If you understand what's happening, then you may be ready for the next step - fixing the bug.
Here is a guide to get started with contributing code: https://wiki.mozilla.org/WebExtensions/Contribution_Onramp

I'm not sure what the "library menu" is?
However I did the steps with the toolbar icon (which is not fallen books but an analog clock).

Furthermore, the breakpoint does hold on right click there or on a website, but not in the History menu. This makes me assume that ext-menus.js#64 is not involved in opening the context menu of the the History menu, which is - if I'm not mistaken - the only place where a user can select multiple history items for a context menu.

Is there a way to continue the execution until UI is responsive again without loosing track of the execution in the debugger? If I do a bunch of "skip over"s quickly after each other it seems to crash the debugger and thereby the entire browser instance. If I just let loose I can not determine where in the code the browser (execution) is ...

From my understanding in like 3 hours of trying to make sense of what I see:

I think the important keyword is "placesContext" defined probably in placesContextMenu.inc.xhtml .

content/places/controller.js:597 (function buildContextMenu) builds the context menu of places items (at least bookmarks and history) (e.g. hides menu items, if needed)

If a single (history) item is selected for history context menu, controller.js:216 (function doCommand) executes the command. doCommand itself is called from PlacesUIUtils.jsm:521 (in function doCommand)

I did not yet figure out how multiple (history) items are handled or where doCommand() is called

I think the webEx-interface would require three new branches in the code, one in buildContextMenu, one in doCommand and "domultipleCommand" - whatever the real name and location of this function is.

instead of buildContextMenu it might be sufficient to just hack into document.getElementById("placesContext").appendChild(), however I did not yet figure out what this child has to look like.

In ex-menus.js 1150 and following maybe relevant, however it registers a listener called onBookmarksContextMenu - and we are looking for history, not bookmarks.

the listener OnBookmarksContextMenu should be named onPlacesContextMenu from my understand, as it is used at least for bookmakrs and history and possibly for all places types.

However I still have no clue about the mechanics of the webextension API, at least not on the browser end. Saying even if I knew how to implement the functionaility in FF I still don't know how to provide this as a webext-API

Attached video history-views.ogv —

This video (captured with Firefox 77) shows the five different places in the UI where history views appear. For comparison, I have also installed an add-on that adds a menu item to bookmark contexts (relevant source code), because the bookmark menu item unexpectedly appears in the history view (which has been fixed on Nightly in bug 1638360).

  1. 0:08 enable History sidebar.
    0:10 contextmenu on history item.
  2. 0:17 show History menu.
    0:18 contextmenu on history item.
  3. 0:25 open History library (Ctrl-Shift-H).
    0:31 contextmenu on history item.
  4. 0:36 open History view via toolbar.
    0:40 contextmenu on history item. Note that an context menu from an extension meant for bookmarks is shown.
  5. 0:45 open History view via hamburger menu.
    0:49 contextmenu on history item. Note that an context menu from an extension meant for bookmarks is shown again.

The last two views unexpectedly contain a bookmark menu item, but the fact that they appear does show that ext-menus.js is aware of those contexts. It allows you to get started with implementing "history" support without having to spend too much time on exactly figuring out how the menu is created: ext-menus.js does already recognize the menu item, except it incorrectly attributes the menu to the "bookmarks" context instead of the (proposed) "history" context. If you step through with a debugger, then you will be able to inspect the relevant variables, and try to find how this maps to history.

Relevant files:

(In reply to 86Legacy from comment #8)

I'm not sure what the "library menu" is?

Never mind the name, see comment 10 for a video with exact details.

Furthermore, the breakpoint does hold on right click there or on a website, but not in the History menu. This makes me assume that ext-menus.js#64 is not involved in opening the context menu of the the History menu,

Since the history context is not supported yet, the breakpoint should not be triggered. However, as I showed in the video (comment 10), there are two cases where the menu is unexpectedly shown (until it got fixed in bug 1638360 - from this small patch you can easily learn which part of the code was responsible for the difference).

I suggest to check if popupshowing event handler is already triggered for other menus (but lacking extension menus due to an early return).

which is - if I'm not mistaken - the only place where a user can select multiple history items for a context menu.

I haven't though of multiple history items. This only happens in the Library window. Do you really need this functionality? We don't even support this for the bookmarks contextmenu.

Is there a way to continue the execution until UI is responsive again without loosing track of the execution in the debugger?

What I usually do is to set a breakpoint later in the code, and then continue execution. Then the breakpoint will be hit again. If you are merely interested in the values of variables, you can also use logpoints instead of breakpoints - https://developer.mozilla.org/en-US/docs/Tools/Debugger/Set_a_logpoint

If I do a bunch of "skip over"s quickly after each other it seems to crash the debugger and thereby the entire browser instance.

The debugger shouldn't crash. Is this reliably reproducible? If yes, please file a bug report with the reproduction steps.

From my understanding in like 3 hours of trying to make sense of what I see:

I think the important keyword is "placesContext" defined probably in placesContextMenu.inc.xhtml .

context="placesContext" means that when a context menu is opened, that the "menupopup" with id="placesContext" will be used.

content/places/controller.js:597 (function buildContextMenu) builds the context menu of places items (at least bookmarks and history) (e.g. hides menu items, if needed)

If a single (history) item is selected for history context menu, controller.js:216 (function doCommand) executes the command. doCommand itself is called from PlacesUIUtils.jsm:521 (in function doCommand)

I did not yet figure out how multiple (history) items are handled or where doCommand() is called

If you cannot find the function call itself, then it sometimes helps to search for a substring/parameter that the function expects. For example, doCommand accepts placesCmd_deleteDataHost, and a search for placesCmd_deleteDataHost yields two results. But before getting sidetracked too soon, I suggest to pick an easy case to get familiar with the code, and defer the other cases to a future stage.

I think the webEx-interface would require three new branches in the code, one in buildContextMenu, one in doCommand and "domultipleCommand" - whatever the real name and location of this function is.

instead of buildContextMenu it might be sufficient to just hack into document.getElementById("placesContext").appendChild(), however I did not yet figure out what this child has to look like.

ext-menus.js already has all logic to create menu items and insert them. The only things that you need to implement are:

  • Add listener / code to call gMenuBuilder.build when needed, with the right contextual information, i.e.:
  • an identifier of the history menu item (that extensions can use in the history API).

In ex-menus.js 1150 and following maybe relevant, however it registers a listener called onBookmarksContextMenu - and we are looking for history, not bookmarks.

the listener OnBookmarksContextMenu should be named onPlacesContextMenu from my understand, as it is used at least for bookmakrs and history and possibly for all places types.

This function name does not have a meaning outside of the file. It is called here: https://searchfox.org/mozilla-central/rev/46e3b1ce2cc120a188f6940b5c6eab6b24530e4f/browser/components/extensions/parent/ext-menus.js#1095,1103,1128-1146
Note: this specific code that you are looking at is solely responsible for menu items in the Library window.

Thanks for all the information

I haven't though of multiple history items. This only happens in the Library window. Do you really need this functionality? We don't even support this for the bookmarks contextmenu.(In reply to Rob Wu [:robwu] from comment #11)
So by design the context menu of more than one bookmark is broken, as it is shown for multiple items as well. "really need [...]?" No. I can make the user mark history items, copy them to clipboard and paste them to the config page. But it would be way more convenient the other way.

What I usually do is to set a breakpoint later in the code, and then continue execution.
well, to find out where to set this later breakpoint you have to step through until this point before - my intention is more like "I dont care about the code until the next user action"

The debugger shouldn't crash. Is this reliably reproducible? If yes, please file a bug report with the reproduction steps.
Well, if "keep clicking on the button 15 or 20 times until the window becomes inresponsive" is reproducible - yes it is. But I only tested on one PC with one browser profile, of course.

Severity: -- → N/A
Status: UNCONFIRMED → NEW
Component: Untriaged → General
Ever confirmed: true
Priority: -- → P5

I hope, this issue is a better place for response than original Bug #1597142.

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

(In reply to max from Bug #1597142 comment #5)

I could agree that, strictly speaking, history items are not bookmarks, but there is no equivalent in history API

This functionality is not part of the bookmarks nor history API, but of the menus API. The "bookmarks" (or "history") permission requirement exists because extensions without such permissions cannot make use of the exposed IDs (e.g. bookmarkId).

Either I have missed something or history API does not have "get by ID" method. For "bookmark" context, namely bookmark ID is provided to onClicked listener. Personally I do not mind to have history.get(id) method but I am in doubt if such firefox-specific function is appropriate. Maybe it is better to allow bookmarks.get to resolve history ID if an extension has enough permissions.

and in firefox they are presented in the same window. In my opinion, the context menu item could be shared as well if the extension has enough permissions.

Even if the UI is currently overlapping, they are conceptually different.

Due to Bug #1597142 I believed that they are not only used interchangeably in UI but tightly coupled in the code as well.

Thank you for pointing me to this bug, originally I found Bug #1597142 (actually at bugzilla-dev.allizom.org, I have no idea who runs that server) and did not expected that there is another issue with so detailed description.

I am impressed how many UI elements have history items, I never realized it before. By the way, the list from "History" menu does not have specific context menu at all (e.g. "Bookmark page" item existing in other lists). I suppose that since menu bar is hidden by default, the assumption is that only rare people use it. I did not remenber if earlier mozilla versions had proper context menu for the history item of menu bar, but there was a period when hover action (show url in "status bar") was broken.

So likely more code has to be touched to make history context menu aware of extensions.

Flags: needinfo?(kernp25)
Assignee: nobody → kernp25
Status: NEW → ASSIGNED
Flags: needinfo?(kernp25)

The bug assignee is inactive on Bugzilla, so the assignee is being reset.

Assignee: kernp25 → nobody
Status: ASSIGNED → NEW
Assignee: nobody → kernp25
Status: NEW → ASSIGNED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: