Closed Bug 581253 Opened 14 years ago Closed 14 years ago

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

Categories

(Firefox :: Bookmarks & History, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 4.0b7
Tracking Status
blocking2.0 --- final+

People

(Reporter: abillings, Assigned: asaf)

References

Details

(Keywords: regression)

Attachments

(1 file, 3 obsolete files)

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.
blocking2.0: --- → ?
Whiteboard: [4b2]
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
Juan reproduced it. Juan, were you on OS X or Windows?
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
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-
Attached patch v2 (obsolete) — Splinter Review
Attachment #462360 - Attachment is obsolete: true
Attachment #462512 - Flags: review?(mak77)
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]
Todayish
Attached patch for checkinSplinter Review
Attachment #462512 - Attachment is obsolete: true
Keywords: checkin-needed
Whiteboard: [4b2][needs new patch with addressed comments]
http://hg.mozilla.org/mozilla-central/rev/7e462f8c5fee
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 4.0b8
Target Milestone: Firefox 4.0b8 → Firefox 4.0b7

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.

Flags: needinfo?(amina.kenessova)

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.

Attachment #9133465 - Attachment is obsolete: true
Flags: needinfo?(amina.kenessova)
You need to log in before you can comment on or make changes to this bug.