Closed Bug 451586 Opened 12 years ago Closed 11 years ago

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

Categories

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

defect

Tracking

()

VERIFIED FIXED
Firefox 3.6a1

People

(Reporter: starcas25, Assigned: dietrich)

References

()

Details

(Keywords: dataloss, late-l10n, verified1.9.1)

Attachments

(1 file, 1 obsolete file)

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
Blocks: 451318
Keywords: dataloss
I intentionally bookmark some web pages in multiple folders for various reasons, so I would like to see this fixed.
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.
Flags: blocking-firefox3.1?
Confirmed and marked blocking+. We should instead only remove the bookmark that is in the folder shown in the dialog.
Severity: normal → major
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: blocking-firefox3.1? → blocking-firefox3.1+
OS: Windows XP → All
Priority: -- → P2
Hardware: PC → All
This looks like intentional behavior (simple to undo, though).
(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.
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.
Target Milestone: --- → Firefox 3.1
We need someone from UX to comment here.
Keywords: uiwanted
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.
Assignee: nobody → dietrich
(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).
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.
Whiteboard: [patch rsn]
Keywords: late-l10n
Attached patch v1 (obsolete) — Splinter Review
Attachment #351981 - Flags: review?(mak77)
Attachment #351981 - Flags: review?(l10n)
Attachment #351981 - Flags: review?(l10n) → review-
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 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
Attachment #351981 - Flags: review?(mak77)
Whiteboard: [patch rsn] → [needs new patch]
Version: unspecified → Trunk
Keywords: uiwanted
Attached patch v2Splinter Review
Attachment #351981 - Attachment is obsolete: true
Attachment #352269 - Flags: review?(mak77)
Attachment #352269 - Flags: review?(l10n)
Whiteboard: [needs new patch] → [has patch][needs reviews]
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.
Attachment #352269 - Flags: review?(mak77) → review+
Whiteboard: [has patch][needs reviews] → [has patch][needs review axel]
Attachment #352269 - Flags: review?(l10n) → review+
Whiteboard: [has patch][needs review axel] → [has patch]
http://hg.mozilla.org/mozilla-central/rev/8e6fb32f0da9
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [has patch]
Target Milestone: Firefox 3.1 → Firefox 3.1b3
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/6a102639c4bc
Keywords: fixed1.9.1
Target Milestone: Firefox 3.1b3 → Firefox 3.2a1
Depends on: 473706
Depends on: 473949
Depends on: 473951
Depends on: 473952
You need to log in before you can comment on or make changes to this bug.