Closed Bug 556342 Opened 14 years ago Closed 14 years ago

Invalid Treeview in bookmark menu via star pane

Categories

(Firefox :: Bookmarks & History, defect)

x86
macOS
defect
Not set
major

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- final+

People

(Reporter: tchung, Assigned: asaf)

References

Details

(Keywords: regression)

Attachments

(2 files)

Attached image WTF Treeview screenshot
I must have hit some timing sync with treeview work in bookmarks.  This is kinda hard to repro, but it had to do with editing subfolders in Library window, and then going back and bookmarking a site and moving it to a newly created subgroup.

See screenshot. It just gets worse and worse as you collapse/expand treeview.

Repro: (as best as i can)
1) install`Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.3a4pre) Gecko/20100331 Minefield/3.7a4pre
2) create a library bookmark structure with sublevel bookmarks under Bookmarks Menu  (eg. Bookmarks Menu > Mozilla > Labs > Weave
3) open a site, and use star pane to bookmark to the site to your sublevel (eg. > Labs > Weave)
4) Now open Library, and create a new folder under the last top level. (eg. > Labs > Weave2)
5) go to the same url, open star pane, remove bookmark, and add bookmark again, this time; choosing the new folder through the menu item
6) Notice the treeview structure gets into a funky state (see screenshot)

Expected:
- making edit/changes to Library with subfolders and sublevels, should allow user to expand/collapse/edit the same subfolders and sublevels within the star pane

Actual:
- Treeview confusion.
This happens on windows 7 too. 
Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.3a4pre) Gecko/20100331 Minefield/3.7a4pre ID:20100331064936
regression window:

works:
http://hg.mozilla.org/mozilla-central/rev/53a85c489708
Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.3a3pre) Gecko/20100312 Minefield/3.7a3pre ID:20100312025809

fails:
http://hg.mozilla.org/mozilla-central/rev/aae4b1306cfa
Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.3a3pre) Gecko/20100312 Minefield/3.7a3pre ID:20100312043208

pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=53a85c489708&tochange=aae4b1306cfa

candidate bug:
Bug 543444  - Replace single-view API with multiple observers


error in console on latest nightly build:
http://hg.mozilla.org/mozilla-central/rev/b5f44530499b
Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.3a4pre) Gecko/20100331 Minefield/3.7a4pre ID:20100331064936

After Click FolderTree Expander in Step 5:
Error: Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsINavHistoryResult.removeObserver]
Source file: chrome://browser/content/places/tree.xml
Line: 94

Close Panel after Srep 6:
Error: Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsINavHistoryResult.removeObserver]
Source file: chrome://browser/content/places/treeView.js
Line: 1364
Blocks: 543444
Keywords: regression
Assignee: nobody → mano
Status: NEW → ASSIGNED
Blocks: 560198
Now, we used hidden attribute for "editBookmarkPanelContent" element.

But I think hidden attribute should not be used, We should use collapsed attribute instead. 

And I applied the following patch in local build, the issue seems to be fixed.
If this helps it, I am glad.

--- src1/browser/base/content/browser-places.js	2010-04-19 20:18:16 +0900
+++ src2/browser/base/content/browser-places.js	2010-04-19 21:46:29 +0900
@@ -95,17 +95,17 @@
     }
   },
 
   // nsIDOMEventListener
   handleEvent: function SU_handleEvent(aEvent) {
     switch (aEvent.type) {
       case "popuphidden":
         if (aEvent.originalTarget == this.panel) {
-          if (!this._element("editBookmarkPanelContent").hidden)
+          if (!this._element("editBookmarkPanelContent").collapsed)
             this.quitEditMode();
           this._restoreCommandsState();
           this._itemId = -1;
           this._uri = null;
           if (this._batching) {
             PlacesUIUtils.ptm.endBatch();
             this._batching = false;
           }
@@ -113,17 +113,17 @@
         break;
       case "keypress":
         if (aEvent.getPreventDefault()) {
           // The event has already been consumed inside of the panel.
           break;
         }
         switch (aEvent.keyCode) {
           case KeyEvent.DOM_VK_ESCAPE:
-            if (!this._element("editBookmarkPanelContent").hidden)
+            if (!this._element("editBookmarkPanelContent").collapsed)
               this.cancelButtonOnCommand();
             break;
           case KeyEvent.DOM_VK_RETURN:
             if (aEvent.target.className == "expander-up" ||
                 aEvent.target.className == "expander-down" ||
                 aEvent.target.id == "editBMPanel_newFolderButton") {
               //XXX Why is this necessary? The getPreventDefault() check should
               //    be enough.
@@ -189,17 +189,17 @@
       this._batching ?
         gNavigatorBundle.getString("editBookmarkPanel.pageBookmarkedTitle") :
         gNavigatorBundle.getString("editBookmarkPanel.editBookmarkTitle");
 
     // No description; show the Done, Cancel;
     // hide the Edit, Undo buttons
     this._element("editBookmarkPanelDescription").textContent = "";
     this._element("editBookmarkPanelBottomButtons").hidden = false;
-    this._element("editBookmarkPanelContent").hidden = false;
+    this._element("editBookmarkPanelContent").collapsed = false;
     this._element("editBookmarkPanelEditButton").hidden = true;
     this._element("editBookmarkPanelUndoRemoveButton").hidden = true;
 
     // 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
@@ -223,17 +223,17 @@
     gEditItemOverlay.initPanel(this._itemId,
                                { hiddenRows: ["description", "location",
                                               "loadInSidebar", "keyword"] });
   },
 
   panelShown:
   function SU_panelShown(aEvent) {
     if (aEvent.target == this.panel) {
-      if (!this._element("editBookmarkPanelContent").hidden) {
+      if (!this._element("editBookmarkPanelContent").collapsed) {
         let fieldToFocus = "editBMPanel_" +
           gPrefService.getCharPref("browser.bookmarks.editDialog.firstEditField");
         var elt = this._element(fieldToFocus);
         elt.focus();
         elt.select();
       }
       else {
         // Note this isn't actually used anymore, we should remove this
@@ -275,17 +275,17 @@
           .setConsumeRollupEvent(Ci.nsIPopupBoxObject.ROLLUP_CONSUME);
       this.panel.openPopup(aAnchorElement, aPosition, -1, -1);
     }
     else
       this.panel.focus();
   },
 
   quitEditMode: function SU_quitEditMode() {
-    this._element("editBookmarkPanelContent").hidden = true;
+    this._element("editBookmarkPanelContent").collapsed = true;
     this._element("editBookmarkPanelBottomButtons").hidden = true;
     gEditItemOverlay.uninitPanel(true);
   },
 
   editButtonCommand: function SU_editButtonCommand() {
     this.showEditBookmarkPopup();
   },
 


--- src1/browser/base/content/browser.xul 2010-04-19 20:18:16 +0900
+++ src2/browser/base/content/browser.xul 2010-04-19 21:53:45 +0900
@@ -187,17 +187,17 @@
             <button id="editBookmarkPanelEditButton"
                     class="editBookmarkPanelHeaderButton"
                     oncommand="StarUI.editButtonCommand();"
                     label="&editBookmark.edit.label;"
                     accesskey="&editBookmark.edit.accessKey;"/>
           </hbox>
         </vbox>
       </row>
-      <vbox id="editBookmarkPanelContent" flex="1" hidden="true"/>
+      <vbox id="editBookmarkPanelContent" flex="1" collapsed="true"/>
       <hbox id="editBookmarkPanelBottomButtons" pack="end">
 #ifndef XP_UNIX
         <button id="editBookmarkPanelDoneButton"
                 class="editBookmarkPanelBottomButton"
                 label="&editBookmark.done.label;"
                 default="true"
                 oncommand="StarUI.panel.hidePopup();"/>
         <button id="editBookmarkPanelDeleteButton"
I saw a similar, but not the same, bug - see bug 560202.
In my case, the folder node has no subnodes whatsoever, although it should (folder did have subfolders).
Severity: normal → major
I'm seeing these observer errors when importing bookmarks from an html file and then clicking on the expand folder drop-down in the edit stare pane popup dialog.
blocking2.0: --- → ?
The patch in bug 553334 should fix these bugs too.  It would help much if someone that can reproduce any of these bugs check if they're fixed with that patch.
I can not reproduce the bug any more on the following m-c nightly build.
Mozilla/5.0 (Windows; U; Windows NT 6.1; WOW64; en-US; rv:1.9.3a5pre) Gecko/20100605 Minefield/3.7a5pre ID:20100605040240
I'm marking fixed based on Mano's and Alice0775's comments.
If anyone can reproduce, please reopen.
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Depends on: 553334
Resolution: --- → FIXED
blocking2.0: ? → final+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: