Cannot remove a bookmark with a tag via 'Edit this bookmark' menu

RESOLVED FIXED in Firefox 4.0b7

Status

()

Firefox
Bookmarks & History
RESOLVED FIXED
8 years ago
7 years ago

People

(Reporter: abillings, Assigned: mano)

Tracking

({regression})

Trunk
Firefox 4.0b7
x86
Mac OS X
regression
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking2.0 final+)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

8 years ago
This was found in the Firefox 4.0 Beta 2 build, Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b2) Gecko/20100720 Firefox/4.0b2, on OS X 10.6.

If a user creates or has a bookmark with a tag in it, when the user attempts to remove the bookmark through the Edit Bookmark menu/dialog (which you get through cmd-d or clicking on the star), the bookmark is not deleted. Instead, it is moved to the "Unsorted Bookmarks" folder from wherever it was. The only way to delete it is to right click on it in the Bookmarks Sidebar or the Bookmarks Library and select "delete" from the context menu.

If the tag is deleted from the bookmark and it is saved, the bookmark can then be deleted normally.

Steps to Reproduce
1. Go to http://www.mozilla.org
2. Hit Cmd/Ctrl-d to add the bookmark
3. In the add bookmark menu that pops up, add a tag, such as "mozilla" to it and click on "done"
4. Click on the star next to the url of the page on the location bar.
5. In the bookmark editing menu, select "Remove Bookmark"

Result: Bookmark is moved to "Unsorted Bookmarks" folder.
Expected Result: Bookmark should be deleted.
(Reporter)

Updated

8 years ago
blocking2.0: --- → ?

Updated

8 years ago
Whiteboard: [4b2]

Updated

8 years ago
Keywords: regression, regressionwindow-wanted
I just tried on trunk and my bookmark disappeared and was not moved to unsorted bookmarks...
is this os x only? since I tried on win7
(Reporter)

Comment 3

8 years ago
Juan reproduced it. Juan, were you on OS X or Windows?

Updated

8 years ago
blocking2.0: ? → final+
Patch coming.

This is a regression from bug 418521. Here's what happens:
1. The tags field is focus with some contents
2. The remove button is clicked, but doesn't get focus (the new behavior on mac).
3. We remove the bookmarks and then hide the panel.
4. A blur event is dispatched on the tags field (on other platforms it's dispatched on the remove button at this point)
5. Instant-apply mechanism receives this event, and re-applies the tags for the given uri.
Assignee: nobody → mano
Status: NEW → ASSIGNED
Blocks: 418521
Keywords: regressionwindow-wanted
Created attachment 462360 [details] [diff] [review]
Patch with test (and some cleanup)

See previous comment for how we got here.
Attachment #462360 - Flags: review?
Attachment #462360 - Flags: review? → review?(dietrich)
Comment on attachment 462360 [details] [diff] [review]
Patch with test (and some cleanup)

He's back!
Attachment #462360 - Flags: review?(dietrich) → review?(mak77)
Comment on attachment 462360 [details] [diff] [review]
Patch with test (and some cleanup)

>diff -r a4d86c3a3494 browser/base/content/browser-places.js

>+
>+          if (this._doOnHide) {
>+            this._doOnHide();
>+            this._doOnHide = null;
>+          }

I don't like much the idea of a disappearing method bcause code is scattered around. Can we instead track action in a property like _actionOnHide = "cancel", "remove", and use a simple switch here so all related code is in one place?

>-#ifdef ADVANCED_STARRING_UI

nice cleanup

>diff -r a4d86c3a3494 browser/base/content/browser.xul

>-            <button id="editBookmarkPanelUndoRemoveButton"
>-                    class="editBookmarkPanelHeaderButton"
>-                    hidden="true"
>-                    oncommand="StarUI.undoRemoveBookmarkCommand();"
>-                    label="&editBookmark.undo.label;"
>-                    accesskey="&editBookmark.undo.accessKey;"/>
>             <button id="editBookmarkPanelRemoveButton"
>                     class="editBookmarkPanelHeaderButton"
>                     oncommand="StarUI.removeBookmarkButtonCommand();"
>                     accesskey="&editBookmark.removeBookmark.accessKey;"/>
>-            <button id="editBookmarkPanelEditButton"
>-                    class="editBookmarkPanelHeaderButton"
>-                    oncommand="StarUI.editButtonCommand();"
>-                    label="&editBookmark.edit.label;"
>-                    accesskey="&editBookmark.edit.accessKey;"/>

as we said on IRC, you can remove the strings right now.

>diff -r a4d86c3a3494 browser/base/content/test/browser_bug581253.js

>+  let tab = gBrowser.selectedTab = gBrowser.addTab();
>+  tab.linkedBrowser.addEventListener("load", (function(event) {
>+    event.currentTarget.removeEventListener("load", arguments.callee, true);
>+

nit: the common behavior I've seen in tests is to repeat tab.linkedBrowser.removeEvent...
    
>+    StarUI.panel.addEventListener("popupshown", onPanelShowm, false);

typo: onPanelShowM... and fix all other instances of it below.

>+function onPanelHidden(aEvent) {
>+  if (aEvent.target == StarUI.panel) {
>+    StarUI.panel.removeEventListener("popuphidden", onPanelHidden, false);

it's possible that our handler is called before the star handler? Just in case I'd vote for an executeSoon here

>+    ok(!starButton.hasAttribute("starred"),
>+       "star button indicates that the bookmark has been removed");

Can you also check PlacesUtils.bookmarks.isBookmarked(uri) just to ensure the attribute is synced?

>+    finish();

copy from another test waitForClearHistory method and use waitForClearHistory(finish); here since you added a page to history

globally looks like the right fix and is fine.
Attachment #462360 - Flags: review?(mak77) → review-
Comment on attachment 462512 [details] [diff] [review]
v2

>diff -r a4d86c3a3494 browser/base/content/browser-places.js

>   handleEvent: function SU_handleEvent(aEvent) {

>+            case "remove": {
>+              // Remove all bookmarks for the bookmark's url, this also removes
>+              // the tags for the url.
>+              PlacesUIUtils.ptm.beginBatch();
>+              let itemIds = PlacesUtils.getBookmarksForURI(this._uriForRemoval);
>+              for (let i=0; i < itemIds.length; i++) {

spaces in the middle of "i=0"

>+                let txn = PlacesUIUtils.ptm.removeItem(itemIds[i]);
>+                PlacesUIUtils.ptm.doTransaction(txn);
>+              }
>+              PlacesUIUtils.ptm.endBatch();
>+              this._uriForRemoval = null;

You always overwrite the value before setting "remove" action, so i'm not sure it's really useful to nullify it, if not for code completion

>diff -r a4d86c3a3494 browser/base/content/test/browser_bug581253.js

>+  let tab = gBrowser.selectedTab = gBrowser.addTab();
>+  tab.linkedBrowser.addEventListener("load", (function(event) {
>+    tab.removeEventListener("load", arguments.callee, true);

should be tab.linkedBrowser.remove, you missed the linkedBrowser part
Attachment #462512 - Flags: review?(mak77) → review+
> You always overwrite the value before setting "remove" action, so i'm not sure it's really useful to nullify it, if not for code completion

I don't override it on normal close (and I cannot).
Oh, I thought that was about actionOnHide. Indeed I could avoid cleaning _uriForRemoval.
what's up here? I thought this already landed
What, indeed! Can this land?
Whiteboard: [4b2] → [4b2][has patch][can land?]
needs to address comments
Whiteboard: [4b2][has patch][can land?] → [4b2][needs new patch with addressed comments]
Keywords: checkin-needed
Whiteboard: [4b2][needs new patch with addressed comments]
http://hg.mozilla.org/mozilla-central/rev/7e462f8c5fee
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 4.0b8

Updated

7 years ago
Target Milestone: Firefox 4.0b8 → Firefox 4.0b7

Updated

7 years ago
Duplicate of this bug: 602701

Updated

7 years ago
Duplicate of this bug: 594471
You need to log in before you can comment on or make changes to this bug.