1219804 - Show last 5 recent bookmarks in the bookmarks menu

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect, P2)

Description

[Verdi [:verdi] Best to slack me]

Attachment: recent-bm-in-menu.png

When someone creates a bookmark, they often follow the jumping star animation and check to see their newly created bookmark in the bookmarks menu. But because the default location of bookmarks is the unsorted bookmarks subfolder they are not immediately visible. User tests show that this is very confusing to people.

We can fix this without reworking the bookmarks IA if we display the recently bookmarked items directly inline in the menu instead of in a flyout panel. We should limit this to 5 items to avoid making the menu any taller than we have to.

Comment 1

[Dão Gottwald [:dao]]

We can't just "move" the "Recently Bookmarked" menu contents to the top level because "Recently Bookmarked" is actually a smart folder in the "Bookmarks Menu" category. I guess we'd have to remove that folder in a migration step when people upgrade. Are we sure we want to go down this route?

Comment 2

[Marco Bonardo [:mak]]

I don't think that's the major problem from a technical point of view. We can either remove it with ui migration or just leave it there for existing profiles and let users remove it manually. This is the easy part.

The problem here is that currently we assume 1 Places view <=> 1 popup. to properly support drag&drop, icons and contextual options we'd need to make popups allow multiple views, and that may be non-trivial, most of menu.xml, BrowserPlacesViews and controller.js assume 1:1 relation.
The current views assume the viewElt is the popup, we'd need to change that to an element inside the popup instead. Then we'd need to fix menu.xml to not assume there's only one view (no _rootView), and fix drag&drop in it and the static part building code. This may require a new view implementation in browserPlacesViews.js. There would be no more a _placesView property on this popup and I'm not sure how many things this would break. The only good thing is that the arrow panel menu already has a separate view from other popups (PlacesPanelMenuView), maybe it could become sort of a multiview forwarding to subviews? no idea.
Otherwise, it may be possible to try creating a subview inside a main view, managed by its own view code in browserPlacesViews. Everything would stay the same for the main view, but we'd have this view inside a view. It may be simpler, but I'm not sure how many details will not work without trying.

The final alternative would be to hardcode bookmarks in the menu, like we did for the view in the history widget (for whatever reason that still escapes me). It would be less work, but they would have a limited functionality compared to the rest of the menu, and we'd have some duped code.

Comment 3

[Dão Gottwald [:dao]]

Jared, you mentioned you had a work-in-progress patch for this. Could you please attach it? It might give me a head start even if it's a rough patch.

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Rough WIP

Comment 5

[Jared Wein [:jaws] (please needinfo? me)]

Dao, any update on this? If you're not able to keep working on this can you post your WIP?

Comment 6

[Dão Gottwald [:dao]]

Attachment: WIP

Does this look sane to you? It only works for the bookmarks button menu, not for the menu bar's bookmarks menu which I'm guessing uses a separate implementation. I'm not sure if this bug was supposed to cover the menu bar in the first place; comment 0 isn't explicit about it. I suppose we'll want to keep both menus somewhat consistent, though.

Comment 7

[Marco Bonardo [:mak]]

Comment on attachment 8717461 [details] [diff] [review]
WIP

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

one thing the users would likely want to do is to drag & drop these items to the menu or the toolbar. That is likely worth a follow-up. In this first implementation they should not be draggable at all, cause otherwise we will end up creating copies of the bookmarks. I don't know off-hand how hard it will be to properly support moving, it will require populating the dataTransfer with fake Places nodes, and convince other views they can move the bookmarks, not just copy them.

::: browser/base/content/browser-places.js
@@ +1336,5 @@
>        },
>        insertionPoint: ".panel-subview-footer"
>      });
> +
> +    this._insertRecentBookmarks();

I assume we need to update these items everytime the menu is opened, while this is updating only when the bookmarks view is created the first time.

Comment 8

[Dão Gottwald [:dao]]

Attachment: patch

Comment 9

[Marco Bonardo [:mak]]

Comment on attachment 8717817 [details] [diff] [review]
patch

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

please file a follow-up bug for D&D.

::: browser/base/content/browser-places.js
@@ +1345,5 @@
> +    const kMaxResults = 5;
> +
> +    let options = PlacesUtils.history.getNewQueryOptions();
> +    options.excludeQueries = true;
> +    options.includeHidden = false;

false is the default value, no need to set it

@@ +1346,5 @@
> +
> +    let options = PlacesUtils.history.getNewQueryOptions();
> +    options.excludeQueries = true;
> +    options.includeHidden = false;
> +    options.resultType = options.RESULTS_AS_URI;

it's the default as well (feel free to update the idl if it doesn't state that)

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/2ba1faa2dcf9

Comment 11

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/2ba1faa2dcf9

Comment 12

[Sören Hentzschel]

Attachment: useless-bookmarks.png

It's not working. It shows five bookmarks but not the last bookmarks but old bookmarks and not the same bookmarks as in "Recently Bookmarked"… and there is no preference to disable the five random (?) bookmarks…

Comment 13

[Sören Hentzschel]

Reopened because the patch does something different than the bug description.

Comment 14

[Dão Gottwald [:dao]]

As usual, please file a new bug and mark it blocking this one.

Comment 15

[Hussam Al-Tayeb]

You are showing the last 5 opened bookmarks and *not* the last 5 bookmarked links.
Maybe this should be reopened.

Comment 17

[Marco Bonardo [:mak]]

that's already fixed in bug 1248258.

Comment 19

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Worcester12345 from comment #18)
> (In reply to Marco Bonardo [::mak] from comment #17)
> > that's already fixed in bug 1248258.
> 
> Perhaps the bug owner should be able to chime in first that it is working.
> Verdi?

This is not representative of how we work. Marco is a fine representative, and as the reviewer on bug 1248258 he is more than sufficient in answering that this has been fixed. Please move comment specific to bug 1248258 to that bug, and make sure that you are testing with a build that includes the changes that landed in that bug.

Comment 20

[Andrei Vaida [:avaida]]

Adding :mboldan to the CC list, since he manually tested this area in the past.

Comment 21

[Hussam Al-Tayeb]

I found an new issue related to the new "recently bookmarked" section. bug 1250203

Comment 22

[YF (Yang)]

[Tracking Requested - why for this release]:
It has some obvious blocks.

Comment 23

[:Gijs (he/him)]

(In reply to YF (Yang) from comment #22)
> [Tracking Requested - why for this release]:
> It has some obvious blocks.

It was backed out from 47 in bug 1254960.

Comment 25

[Mihai Boldan, Desktop QA [:mboldan]]

I managed to verify this issue on Firefox 49.0a1 (2016-05-05) and I confirm that the last 5 recent bookmarks are correctly displayed in the bookmarks menu.
The tests were performed on Windows 10 x64, Mac OS X 10.11.1 and on Ubuntu 14.04 x86.
I am marking this issue Verified-Fixed.