Show last 5 recent bookmarks in the bookmarks menu

VERIFIED FIXED in Firefox 48

Status

()

defect
P2
normal
VERIFIED FIXED
4 years ago
2 years ago

People

(Reporter: verdi, Assigned: dao)

Tracking

(Depends on 1 bug)

unspecified
Firefox 48
All
Unspecified
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox47 disabled, firefox48 fixed, firefox49 verified)

Details

(Whiteboard: [Onboarding])

Attachments

(2 attachments, 3 obsolete attachments)

Reporter

Description

4 years ago
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.
Reporter

Updated

4 years ago
Blocks: 1219810
Reporter

Updated

4 years ago
Priority: -- → P1
Reporter

Updated

4 years ago
Whiteboard: [Onboarding]
Assignee

Comment 1

3 years ago
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?
Flags: needinfo?(mverdi)
Flags: needinfo?(mak77)
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.
Flags: needinfo?(mak77)
Assignee

Comment 3

3 years ago
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.
Flags: needinfo?(mverdi) → needinfo?(jaws)
Posted patch Rough WIP (obsolete) — Splinter Review
Flags: needinfo?(jaws)
Dao, any update on this? If you're not able to keep working on this can you post your WIP?
Flags: needinfo?(dao)
Assignee

Comment 6

3 years ago
Posted patch WIP (obsolete) — Splinter Review
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.
Assignee: nobody → dao
Attachment #8709560 - Attachment is obsolete: true
Flags: needinfo?(dao)
Attachment #8717461 - Flags: feedback?(mak77)
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.
Attachment #8717461 - Flags: feedback?(mak77) → feedback+
Assignee

Comment 8

3 years ago
Posted patch patchSplinter Review
Attachment #8717461 - Attachment is obsolete: true
Attachment #8717817 - Flags: review?(mak77)
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)
Attachment #8717817 - Flags: review?(mak77) → review+
Assignee

Updated

3 years ago
Flags: qe-verify+

Comment 11

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/2ba1faa2dcf9
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
Posted image useless-bookmarks.png (obsolete) —
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…
Reopened because the patch does something different than the bug description.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee

Comment 14

3 years ago
As usual, please file a new bug and mark it blocking this one.
Status: REOPENED → RESOLVED
Closed: 3 years ago3 years ago
Resolution: --- → FIXED

Updated

3 years ago
Depends on: 1248258

Updated

3 years ago
Depends on: 1248259
Assignee

Updated

3 years ago
Depends on: 1248266

Updated

3 years ago
Depends on: 1248267
Assignee

Updated

3 years ago
Attachment #8719280 - Attachment is obsolete: true

Comment 15

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

Updated

3 years ago
Depends on: 1248268
Assignee

Updated

3 years ago
Depends on: 1248616
Assignee

Updated

3 years ago
Depends on: 1248617
Comment hidden (spam)
that's already fixed in bug 1248258.
Comment hidden (spam)
(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.
Assignee

Updated

3 years ago
Depends on: 1249473
Adding :mboldan to the CC list, since he manually tested this area in the past.

Comment 21

3 years ago
I found an new issue related to the new "recently bookmarked" section. bug 1250203
Depends on: 1250203
Depends on: 1250493

Updated

3 years ago
Depends on: 1250828

Updated

3 years ago
Depends on: 1253930

Updated

3 years ago
No longer depends on: 1253930
Assignee

Updated

3 years ago
Depends on: 1253996
Assignee

Updated

3 years ago
Depends on: 1254960

Comment 22

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

Comment 23

3 years ago
(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.
Target Milestone: Firefox 47 → Firefox 48

Updated

3 years ago
Duplicate of this bug: 972282
Assignee

Updated

3 years ago
Depends on: 1267939
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.
Status: RESOLVED → VERIFIED
Flags: qe-verify+

Updated

3 years ago
Depends on: 1326707
You need to log in before you can comment on or make changes to this bug.