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

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Al Billings [:abillings - ex-MoCo]]

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.

Comment 1

[Marco Bonardo [:mak]]

I just tried on trunk and my bookmark disappeared and was not moved to unsorted bookmarks...

Comment 2

[Marco Bonardo [:mak]]

is this os x only? since I tried on win7

Comment 3

[Al Billings [:abillings - ex-MoCo]]

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

Comment 4

[Asaf Romano (gone)]

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.

Comment 5

[Asaf Romano (gone)]

Attachment: Patch with test (and some cleanup)

See previous comment for how we got here.

Comment 6

[Asaf Romano (gone)]

Comment on attachment 462360 [details] [diff] [review]
Patch with test (and some cleanup)

He's back!

Comment 7

[Marco Bonardo [:mak]]

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.

Comment 8

[Asaf Romano (gone)]

Attachment: v2

Comment 9

[Marco Bonardo [:mak]]

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

Comment 10

[Asaf Romano (gone)]

> 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).

Comment 11

[Asaf Romano (gone)]

Oh, I thought that was about actionOnHide. Indeed I could avoid cleaning _uriForRemoval.

Comment 12

[Marco Bonardo [:mak]]

what's up here? I thought this already landed

Comment 13

[Johnathan Nightingale [:johnath]]

What, indeed! Can this land?

Comment 14

[Marco Bonardo [:mak]]

needs to address comments

Comment 15

[Asaf Romano (gone)]

Todayish

Comment 16

[Asaf Romano (gone)]

Attachment: for checkin

Comment 17

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/7e462f8c5fee

Comment 20

[Amina Kenessova]

Attachment: Bug 581253 - BrowserTestUtils.waitForCondition in browser_bug581253.js replaced by TestUtils.waitForCondition. r=prathiksha

Comment 21

[Marco Bonardo [:mak]]

Hello Amina, for your patch you should first create a new bug, then change the bug number in the commit message to that bug, and push it again. We can't land more patches for already fixed bugs, so you need a new bug.

Comment 22

[Phabricator Automation]

Comment on attachment 9133465 [details]
Bug 581253 - BrowserTestUtils.waitForCondition in browser_bug581253.js replaced by TestUtils.waitForCondition. r=prathiksha

Revision D66908 was moved to bug 1622244. Setting attachment 9133465 [details] to obsolete.