1420246 - add a bookmarks.show() function to focus a bookmark in the UI

Status: NEW

(WebExtensions :: Frontend, enhancement, P5)

Description

[Nicolas Mandil (:NicolasWeb)]

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:56.0) Gecko/20100101 Firefox/56.0
Build ID: 20171003100843

Steps to reproduce:

Add a bookmarks.show() function to focus a bookmark in the UI, so it is possible to find it in the bookmark tree of the sidebar/library from a bookmark search results list.

Comment 1

[Caitlin Neiman [:caitmuenster]]

Hi Nicolas, this has been added to the agenda for the WebExtensions APIs triage on March 27, 2018. Would you be able to join us? 

Here’s a quick overview of what to expect at the triage: 

* We normally spend 5 minutes per bug
* The more information in the bug, the better
* The goal of the triage is to give a general thumbs up or thumbs down on a proposal; we won't be going deep into implementation details

Relevant Links: 

* Wiki for the meeting: https://wiki.mozilla.org/WebExtensions/Triage#Next_Meeting
* Meeting agenda: https://docs.google.com/document/d/1k7AgEyVdZzeF2AIrQl9pP2QRpLENa1OfHvTKhYvKaSo/edit#
* Vision doc for WebExtensions: https://wiki.mozilla.org/WebExtensions/Vision

Comment 2

[aafn]

If I may, my 2 cents as developer for the Bookmark Search Plus 2 add-on https://addons.mozilla.org/fr/firefox/addon/bookmark-search-plus-2/ and https://github.com/aaFn/Bookmark-search-plus-2
in case I cannot join at the triage as Nicolas was advising me.

From my experience on that subject, I believe that bookmarks.show() can represent some amount of code, but this is probably one of the best ways to provide the functionality to extensions, and worth the investment given the big lack this represents to users (this is why I decided to go through the hassle of creating a new UI, mimicking as much the real one ..).

However:
- it requires the native sidebar to be open
- I am not sure how it would work in Android .. there is no sidebar as I can see it, and this is why I dropped support for Android when publishing my add-on
- It won't work on the library since as I get it no WebExtension is working in the library

Therefore a WebExtension using it would need to be a popup, not a sidebar. Or maybe something in the web page itself, but this would be quite limited I presume.

It the menus.ContextType feature can be made to work from the library and from the bookmark sidebar (https://bugzilla.mozilla.org/show_bug.cgi?id=1419195) , in particular from the search pane, that would enable the above bug to work from inside the native sidebar ... with the additional complexity that then the native sidebar should be able to show the bookmark pane at the same time than the search pane (which is the option I chose in BSP2, in line with the initial BSP extension from alice0775).

Hope all the above makes sense :-)

Comment 3

[aafn]

As an afterthought (sorry for the 2 posts then), I believe providing this feature would also require to provide ways to restore the bookmark tree as it was before bookmarks.show().

Indeed, this action can open up a lot of folders on the path to the bookmark to show, and destroy the tree visual aspect, which would require the user to close things up again one by one (or at least the topmost one) .. not friendly at all.
An alternative is to show the bookmark with keeping folders closed if they are closed, as the legacy BSP and follow on BSP2 are doing, but we still need a way to say that the show() is finished ..

Something like:
- bookmarks.saveVisualTreeState() and bookmarks.restoreVisualTreeState(),
- or simpler maybe, a bookmarks.closeShow() using a handle given by bookmarks.show(), but mind that the shows can be stacked, by showing multiple bookmarks .. so the close should close them all at once at least.


so as to restore the tree as it was after the show ..

Again as visual illustration if needed, this is what the initial BSP legacy extension from alice0775, or the follow-on BSP2 add-on, are doing.

Comment 4

[Luca Greco [:rpl] [:luca] [:lgreco]]

Hi Marco,
This bugzilla issue is related to a WebExtensions API request that have been briefly discussed today in the API triage meeting.

We are not thrilled about implementing this method and we are inclined to deny this API request, especially if the underlying "Places and the Bookmarks UI" implementation doesn't already provide a similar feature (which is basically "open the bookmarks UI and select a bookmark or bookmark tree given its id"),
but we wanted to double-check it with you first, in case in your opinion this could be something that could be useful elsewhere and worth to support from your point of view, only in that case we would be ok with wiring up it into a new WebExtensions API method.

As a side note: the workaround that an extension may currently use to point the users to the bookmarks that the extension is manipulating is implementing its own UI in an extension page (e.g. in the sidebar).

Comment 5

[Nicolas Mandil (:NicolasWeb)]

Hi Marco and the WebExtension team,
I'm trying ta make a summary of the current situation to make things clear while you decide, 

That bug has been opened originally because of this discourse thread https://discourse.mozilla.org/t/new-to-extension-development-bookmark-api-and-search-function/21686

A similar feature has been approved as a Firefox feature, not as an API, in bug 1272891 . It's now a P5.

The feature that bookmarks.show() function should give was provided by the legacy add-on Go-parent-Folder for the basic behaviour this new API target, and Bookmark Search Plus 1 (legacy) and then 2 (WebExtension) for a more advanced behaviour combining search panel and bookmark tree.
This feature exist in Chrome as a core one : Bookmark manager -> right click on a search result -> Context menu item "Show the folder"

Add-on users and the BSP2 dev originally focused more on an API as a first step to a solution, because it allow to support both the simple and advanced behaviour, in both Firefox and add-ons.
You better know the Firefox bookmark module and might better know the legacy bookmarks add-ons that existed before to evaluate if this would be useful for other simple/advanced webExtension add-ons.
aafn said (and could confirm) that he is ready to implement as much as possible the feature as a Firefox feature, as approved in bug 1272891 at least.

It seems to us that such a feature is useful for heavy bookmark users. While implementing BSP2 add-on, we had a lot of discussions on its implementation and feature on its github repo https://github.com/aaFn/Bookmark-search-plus-2/issues/, and spoke about the user stories/use cases :
- Heavy bookmarks users: people with a lot of bookmarks, who have the bookmarks sidebar open on their left all navigation time long, and search for bookmarks, reorganize, drag, drop tabs and links to new bookmarks in the same folder as their brothers and already existing ones.
- To find a bookmark while searching for one that the user remember is not far away... from the one she/he look for.

BTW, the actual workaround (extension is manipulating and is implementing its own UI in an extension page (e.g. in the sidebar)) have very poor performance results. It seems it's because of WebExtension API storage current solutions. This have a huge impact at startup and each time a user switch to the BSP2 sidebar. It too have: window dialog focus issue, window position issue, keyboard sniffing issue, ... Hope that things will become better on Firefox Web Extension implementation with the time going on. Now Add-ons dev have to use a lot of ugly half-workaround to deliver half feature.
And the add-on dev have to reimplement for it's sidebar any old and new Firefox preference (Ex.: left click open the bookmark in own new tab). From a user point of view, it seems Firefox is not respecting its own preferences.

Any decision you take, thanks for the job ! ;)

Comment 6

[Marco Bonardo [:mak]]

I will make a long story short: we will very likely add a similar API in the future, because we think it adds value to the Bookmark views. Ideally WebExtensions could just expose that, once available.

The problem is that we don't yet have an efficient way to get the full path to a bookmark (Bug 408991), that is the same problem blocking Bug 469441 (the original "open enclosing folder" bug, that I just duped the other one to) and Bug 469421. We need an adjacency table (likely mirrored in memory) to be able to quickly get the full path given a bookmark GUID.
Without that prerequisite, we can't do much. At this time we don't have the resources to do that in the team taking care of the bookmarks backend, we're paying technical and performance debts yet.

Comment 7

[Mike Conca [:mconca]]

Thanks, Marco. Sounds like this API could be possible for WebExtensions, but will need to wait for both bug 408991 as well as some TBD bug that implements the API to the bookmark view.

Comment 8

[aafn]

Hello Marco, is the lack of "efficient way" you mention above related in any way with bad performances we observe on the bookmarks.getTree() and bookmarks.getChildren() API ? https://bugzilla.mozilla.org/show_bug.cgi?id=1396364

In Bookmark Search Plus 2 (BSP2) WebExtension add-on, I am using a side hash list of bookmark object (in fact an overall object of objects with bookmark ids as index), which can be seen as an adjacency list (https://en.wikipedia.org/wiki/Adjacency_list) of the graph since each object contains the id of its parent and an array of pointers to its children.
I guess this is what you call adjacency table ?
Things are then very fast to find the path to an object.

However, the piece which is very painful is to get the tree of bookmarks from FF as said above. One of my users with 80000 bookmarks is at an extreme of 25s spent just in bookmarks.getTree() .. before I can start displaying anything.
With the native bookmark sidebar, he sees no delay.

And if I try to use only bookmarks.getChildren() in hopoe for going faster and to start to display top of the tree immediately, I still have the 25s wait before getting the few top children .. and also each time I want to get then the children of children .. etc .. and things are worse.

This is really the first thing to improve for add-ons if we can.

Comment 9

[Nicolas Mandil (:NicolasWeb)]

Hi aafn, when you are waiting an answer from someone, it's better to set a needing flag in bugzilla. (Except if I'm no more up-to-date to date about the good behaviour here...)

Comment 10

[Marco Bonardo [:mak]]

(In reply to aafn from comment #8)
> Hello Marco, is the lack of "efficient way" you mention above related in any
> way with bad performances we observe on the bookmarks.getTree() and
> bookmarks.getChildren() API ?
> https://bugzilla.mozilla.org/show_bug.cgi?id=1396364

No, I don't think so. That must be profiled apart. Most of our users have less than 1000 bookmarks, and the few pathological cases of tens of thousands bookmarks is not well managed yet. But as I said, we need a perf profile and I don't think it's related to this.

> In Bookmark Search Plus 2 (BSP2) WebExtension add-on, I am using a side hash
> list of bookmark object (in fact an overall object of objects with bookmark
> ids as index), which can be seen as an adjacency list
> (https://en.wikipedia.org/wiki/Adjacency_list) of the graph since each
> object contains the id of its parent and an array of pointers to its
> children.
> I guess this is what you call adjacency table ?

Yes, sort of, we'd probably just keep a folders hierarchy, but we don't want to recalculate it at every startup, so it must be stored and kept up-to.date.

> However, the piece which is very painful is to get the tree of bookmarks
> from FF as said above. One of my users with 80000 bookmarks is at an extreme
> of 25s spent just in bookmarks.getTree() .. before I can start displaying
> anything.
> With the native bookmark sidebar, he sees no delay.

They take different code paths, the sidebar is fully synchronous, while this API is fully asynchronous. Though, just that is not enough to explain the difference. That's why we need to perf profile it.