451586 - Removing a bookmark using the Star dialog also removes all duplicates (same url)

Status: VERIFIED FIXED

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

Description

[Rick]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.1) Gecko/2008070208 Firefox/3.0.1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.1) Gecko/2008070208 Firefox/3.0.1

I am talking about the "Edit This Bookmark" dialog reached via Star button or Ctrl-D.  When the user removes a bookmark, I think he would expect only one to be removed, but the "Remove Bookmark" button in this dialog will remove all duplicate bookmarks as well, ie any bookmark with the same url though perhaps different other properties: name/title, Tags, keywords, description. This might happen if the user were trying to remove a duplicate he added by mistake/temporarily, but then ends up wiping them all out.

Note: I originally filed one bug (See Bug 451318) which included several issues
regarding duplicate bookmarks, but then broke it up into individual bugs which
block that one.

Reproducible: Always

Comment 1

[Paul J. Weiss]

I intentionally bookmark some web pages in multiple folders for various reasons, so I would like to see this fixed.

Comment 2

[Paul J. Weiss]

I can confirm this bug. This is a regression from Firefox 2. I think the importance/severity should be bumped up to critical or blocker, since this can delete bookmarks without the user being aware of it.

Comment 3

[Dietrich Ayala (:dietrich)]

Confirmed and marked blocking+. We should instead only remove the bookmark that is in the folder shown in the dialog.

Comment 4

[Simon Bünzli]

This looks like intentional behavior (simple to undo, though).

Comment 5

[Marco Bonardo [:mak]]

(In reply to comment #3)
> Confirmed and marked blocking+. We should instead only remove the bookmark that
> is in the folder shown in the dialog.

How can we know what bookmark the user is referring to? he is removing the bookmark from the star dialog (that is related to the locationbar url) or doing CTRL+D (again related to the locationbar url), so it's like he is saying "forget about this address", while he should delete the bookmark from the local position (toolbar, menu, and so on) using the organizer or the context menu if he wants to get rid of only one of them.

We could indeed remove the bookmark based on the folder picker value (that is selected between available bookmarks for that url), but we would be left without an easy way to tell the browser "forget about this address", we would practically invert the behaviour, so to get rid of a bookmarked uri the user should search for it and delete all instances one by one, and if he forgets about one of them we also have a somehow privacy hit.

We provide both a way to remove all bookmarks instances and a way to remove them one by one. Doing the invert would provide an hard way to remove all instances.

Comment 6

[Nochum Sossonko [:Natch]]

That, imho, is very misleading either don't have a folder in the panel and indicate (uiwanted) that this refers to multiple bookmarks, or just remove the specified bookmark.

Comment 7

[Shawn Wilsher :sdwilsh]

We need someone from UX to comment here.

Comment 8

[Dietrich Ayala (:dietrich)]

Talked to Alex today about this:

Closing the dialog immediately with the star still lit up is not good because it gives the impression that nothing happened. However, this is a viable short-term solution until we get the right UI worked out, as data-loss trumps clunky UI.

Alex had two ideas for UI for this:

- change the button text to "Remove All" if there are multiple bookmarks. a con is that the folder selector only shows the one folder.

- after clicking "Remove Bookmark" button, 1) leave the dialog open, 2) show notification text above the button saying "bookmark removed" and 3) update the title and folder-selector fields.

Comment 9

[Marco Bonardo [:mak]]

(In reply to comment #8)
> - after clicking "Remove Bookmark" button, 1) leave the dialog open, 2) show
> notification text above the button saying "bookmark removed" and 3) update the
> title and folder-selector fields.

do you mean showing the next one on the folder selector? So to remove all you click, click, click? what if the user creates a new bookmark and wants to remove the old one? this method works for removal in reverse last modified date. Also notice we could present different bookmarks at different times based on last modified.
So, i could agree with this solution, but would be cool if we would have a sort of "show me the next one" button to be able to remove one of them (visible only if we have more than 1 bookmark).

Comment 10

[Alex Faaborg [:faaborg] (Firefox UX)]

A few more thoughts on this:

-the phrase "remove all bookmarks" is obviously problematic, since the scope of "all" can be pretty easily misunderstood.

-This UI was designed for a simplistic model of bookmarking, so I feel like the best solution is also the most simplistic one.  Having buttons for next bookmark, or tabs or something gets way to complicated.

Since the original problem was that we were removing multiple bookmarks while only stating that we would remove one, let's just change the text of the button to "Remove Bookmarks (n)" where n is the number of bookmarks that exist.

We could later have a follow up bug to add an Edit All right night to the multiple remove button that launches the library window with a search for that URL against All Bookmarks, but we would also want to add a Folder column, and show folder as one of the property elements in the properties pane, so the fix is kind of involved.  Overall I'm not all that interested in us going to a lot of effort to clean up some of these use cases.

Comment 11

[Dietrich Ayala (:dietrich)]

Attachment: v1

Comment 12

[Axel Hecht [:Pike]]

Comment on attachment 351981 [details] [diff] [review]
v1

r-, as soon as we actually name the number, we'll need to handle plurals right, should work with https://developer.mozilla.org/en/Localization_and_Plurals

Comment 13

[Marco Bonardo [:mak]]

Comment on attachment 351981 [details] [diff] [review]
v1

>diff --git a/browser/base/content/browser-places.js b/browser/base/content/browser-places.js
>--- a/browser/base/content/browser-places.js
>+++ b/browser/base/content/browser-places.js
>@@ -190,6 +190,19 @@
>     // The remove button is shown only if we're not already batching, i.e.
>     // if the cancel button/ESC does not remove the bookmark.
>     this._element("editBookmarkPanelRemoveButton").hidden = this._batching;
>+
>+    // The label of the remove button differs if the URI is bookmarked
>+    // multiple times.
>+    var numBookmarks = PlacesUtils.getBookmarksForURI(gBrowser.currentURI);

numBookmarks is a bit confusing name since the num is instead given by numBookmarks.length

Comment 14

[Dietrich Ayala (:dietrich)]

Attachment: v2

Comment 15

[Marco Bonardo [:mak]]

Comment on attachment 352269 [details] [diff] [review]
v2

>diff --git a/browser/base/content/browser-places.js b/browser/base/content/browser-places.js
>--- a/browser/base/content/browser-places.js
>+++ b/browser/base/content/browser-places.js
>@@ -190,6 +190,14 @@
>     // The remove button is shown only if we're not already batching, i.e.
>     // if the cancel button/ESC does not remove the bookmark.
>     this._element("editBookmarkPanelRemoveButton").hidden = this._batching;
>+
>+    // The label of the remove button differs if the URI is bookmarked
>+    // multiple times.
>+    var bookmarks = PlacesUtils.getBookmarksForURI(gBrowser.currentURI);
>+    var forms = bundle.getString("editBookmark.removeBookmarks.label");
>+    Components.utils.import("resource://gre/modules/PluralForm.jsm");

you should be able to use Cu, since browser.js defines it.

Comment 16

[Dietrich Ayala (:dietrich)]

http://hg.mozilla.org/mozilla-central/rev/8e6fb32f0da9

Comment 17

[Shawn Wilsher :sdwilsh]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/6a102639c4bc

Comment 18

[Tracy Walker [:tracy]]

https://bugzilla.mozilla.org/show_bug.cgi?id=412132