Closed Bug 135717 Opened 22 years ago Closed 20 years ago

deleting/removing all sidebar tabs and adding them back does not work (cannot add when sidebar empty)

Categories

(SeaMonkey :: Sidebar, defect)

defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED
mozilla1.1alpha

People

(Reporter: sujay, Assigned: neil)

References

(Blocks 1 open bug)

Details

Attachments

(4 files, 6 obsolete files)

using 4/4 build of netscape

1) launch netscape (old or new profile)
2) open sidebar
3) Tabs | Customize Sidebar
4) Remove all the tabs in the right window using Remove button, one at a time.

after they are all removed

5) try adding some tabs from one of the 5 folders on the left side.

does not work. can't add tabs..

you are stuck at this point..you have to install an older build.
even re-launch with new profile does not cure the problem.

I tried this on the 3/1 trunk commercial build and this is not
a problem on that build..I can try a couple more intermediate
builds between 3/1 and today 4/5.
Keywords: nsbeta1
This is bad, but are any users likely to do it?
nsbeta1- per Nav triage team.
Keywords: nsbeta1nsbeta1-
Target Milestone: --- → mozilla1.1alpha
Well, I make the Galician translation. One user did it (apparently to remove
English tabs and add Galician tabs instead) and it happened to him (and he
complained to me) :-)
*** Bug 145328 has been marked as a duplicate of this bug. ***
I did this as part the of the front-end tests, see the dup bug I just opened!
Bug 145328
*** Bug 147792 has been marked as a duplicate of this bug. ***
*** Bug 148865 has been marked as a duplicate of this bug. ***
Just loaded Mozilla today and while checking it features, I also discovered this
problem.  Now the sidebar is useless.  How do I add tabs to it now?
Attached patch Small fixSplinter Review
I am not a XUL expert, but this patch fixes the problem for me. I believe this
started occurring with the 1.43->1.44 checkin of  
xpfe/components/sidebar/resources/customize.js.

There are two primary problems:

1) Usage of tree.getElementsByTagName("treechildren")[1]. When all sidebar
panels have been removed, tree.getElementsByTagName("treechildren") returns
only one item. When sidebar panels exist, this call returns two items.

2) tree.view is always null in the save_initial_panels function when all panels
have been removed. This causes the setTimeout(0,...) call to continue until a
panel is added. Once a panel has been added, save_initial_panels initializes
the original_panels array with the newly added panel, so when Save() is called,
no changes are detected and so the RDF file is not updated.
Note: the fix posted above is just a preliminary one. I am particularly unsure
about the following:

tree_root = document.createElement('treechildren');

When I create a treechildren element in this fashion, it seems that all
references to tree.treeBoxObject.selection trigger warnings in the debug build.
*** Bug 163430 has been marked as a duplicate of this bug. ***
> tree_root = document.createElement('treechildren');
> 
> When I create a treechildren element in this fashion, it seems that all
> references to tree.treeBoxObject.selection trigger warnings in the debug build.

Use createElementNS() with the XUL namespace.  See:
<http://www.w3.org/TR/2000/REC-DOM-Level-2-Core-20001113/core.html#ID-DocCrElNS>
<http://www.xulplanet.com/tutorials/xultu/window.html>
I have tried changing the usage of createElement to createElementNS, and it
doesn't seem to have any effect. After doing the following:

const XULNS = "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul";
...
tree_root = document.createElementNS(XULNS,'treechildren');
...
tree.appendChild(tree_root);

references to tree.treeBoxObject.selection are still null. I have also found
that if all sidebar tabs are removed, tree.treeBoxObject.selection is also null
in the enable_buttons_for_current_panels function, and simply accessing the
variable produces the following warning in a debug build:

WARNING: NS_ENSURE_TRUE(aContent) failed, file
/home/pkw/builds/trunk/mozilla/layout/html/base/src/nsFrameManager.cpp, line 602

This problem occurs with or without this patch.
Keywords: patch, review
*** Bug 180865 has been marked as a duplicate of this bug. ***
*** Bug 182022 has been marked as a duplicate of this bug. ***
*** Bug 182230 has been marked as a duplicate of this bug. ***
*** Bug 186101 has been marked as a duplicate of this bug. ***
Attached patch simpler fix (obsolete) — Splinter Review
always have an empty treechildren in the customize.xul file. this will avoid
tree.getElementsByTagName("treechildren")[1] to be an undefined object.
Attachment #109824 - Flags: review?(sgehani)
Comment on attachment 109824 [details] [diff] [review]
simpler fix

this patch doesn't work.
Attachment #109824 - Attachment is obsolete: true
Attachment #109824 - Flags: review?(sgehani)
Attachment #94545 - Flags: review?(sgehani)
Comment on attachment 94545 [details] [diff] [review]
Small fix

Chnaging review requestee to Shuehan since she now works on sidebar.
Attachment #94545 - Flags: review?(sgehani) → review?(shliang)
Here's a simple workaround for end-users (at least it works for Mozilla 1.3a --
I haven't tried others):

Edit -> Preferences -> Advanced -> DOM Inspector -> install sidebar

This will successfully add the DOM inspector tab to the sidebar.  After this,
you can successfully add other tabs (bookmarks, etc.), and delete the DOM
sidebar if you don't really want it.

I presume that this procedure adds the DOM inspector through a 
different mechanism than that used by the usual "configure sidebar" 
interface.  This is good, in that it gives us a workaround for the 
present problem, but bad in the long run -- it seems like there 
should be just one function that adds tabs to the sidebar, and that 
everything should use it.
This happened to me today! Mozilla 1.5b

Bryan Wright: thanks for the workaround.
Adding Neil who may have a better idea for how to fix this bug.
Easy one this. The template only generates the <treechildren/> when you have
panels, so you need to explicitly specify one (the template will reuse it).
Attached patch Proposed patch (obsolete) — Splinter Review
Can someone please test this for me?
That doesn't seem to fix it for me here - I get the following error message in
the Javascript console every time I attempt to add a tab (after removing all of
them):

Error: tree_root has no properties
Source File: chrome://communicator/content/sidebar/customize.js
Line: 377
Attached patch Fixed patch (obsolete) — Splinter Review
Ah, that would be because I patched the wrong tree :-[

I've had time to test this now, it works this time!
Attachment #131917 - Attachment is obsolete: true
Attachment #131951 - Flags: review?(varga)
Attachment #131951 - Flags: superreview?(rbs)
Attachment #131951 - Flags: review?(varga) → review+
In my preliminary testing, this does seem to fix not being able to add sidebar
tabs after removing them all, but it does regress the sidebar somewhat. For me,
it only allows one sidebar tab to show up at a time in:

Tabs->Customize Sidebar under the "Tabs in Sidebar" column.

This only allows you to delete one item from the sidebar at a time. You have to
open and close the "Customize Sidebar" dialog several times to delete them all.
Strange, all the treeitems appear in the DOM, but view.rowCount is 1 ...
Jan, any ideas?
Comment on attachment 131951 [details] [diff] [review]
Fixed patch

Since there is still a pending issue, I am passing the sr request to bz, as I
will be travelling in the coming days.
Attachment #131951 - Flags: superreview?(rbs) → superreview?(bzbarsky)
Comment on attachment 131951 [details] [diff] [review]
Fixed patch

So let me get this straight.  The patch fixes this bug but introduces another
one?
Yes - the problems described in comment 28 need to be resolved before this 
patch can be checked in.
Attached patch Ugly hackSplinter Review
OK, so deferring the creation of the view makes it display the template.

Having said this, Jan says that the right way is either to construct the DOM by
reading the RDF manually, and keep the existing DOM manipulation routines, or
rewrite everything in RDF and not use DOM at all.

So I won't bother asking for reviews on this patch, because I won't get them.
Attachment #131951 - Attachment is obsolete: true
Attachment #131951 - Flags: superreview?(bzbarsky) → superreview-
Adjusting summary, there are lots of duplicates...
Summary: deleting all tabs then adding them back does not work → deleting/removing all sidebar tabs and adding them back does not work (cannot add when sidebar empty)
*** Bug 203612 has been marked as a duplicate of this bug. ***
*** Bug 224015 has been marked as a duplicate of this bug. ***
Neil, are the bugs that could be filed on making DOM stuff work better with
templated rdf/xul here?  (As a third alternative to the two methods Jan
suggested.  Also, do these bugs already exist?)
I searched all the duped bugs (and their dupes). Result:
Deleting "panels.rdf" is a different possibility to get rid of the problem.
And bug 203612 comment #10 and bug 218640 comment #0 propose solutions for the
underlying bug (did not look at them).
Attachment #139201 - Flags: superreview?(jag)
Attachment #139201 - Flags: review?(varga)
Comment on attachment 139201 [details] [diff] [review]
Based on bug 218640 comment 0

r=varga, but it's still a temporary solution
I had a similar problem recently, I created a temp in-memory data source and it
worked like a charm
Attachment #139201 - Flags: review?(varga) → review+
Comment on attachment 139201 [details] [diff] [review]
Based on bug 218640 comment 0

sr=jag
Attachment #139201 - Flags: superreview?(jag) → superreview+
OK, checked in the workaround, but leaving open to properly fix the RDF.
*** Bug 231385 has been marked as a duplicate of this bug. ***
Comment on attachment 94545 [details] [diff] [review]
Small fix

Removing obsolete review request
Attachment #94545 - Flags: review?(shliang)
Oh, and apologies to Kyle for not reading his nonworking patch before "copying"
it :-( and to Phillip for not reading comment 9 carefully enough :-/
Neil, you are always faster to catch the right thing than I do :) Good to see
this bug gets fixed.
Attached patch Do things properly (obsolete) — Splinter Review
OK, so the main thrust of the patch is to enumerate the panels on load and
manually add each one to the panels tree instead of templating it. However to
do that I had to tweak the add panel function not to adjust the selection,
which gave me the chance to remove some redundant code.
Comment on attachment 139822 [details] [diff] [review]
Do things properly

This way is easier than trying to manipulate an in-memory data source (and
simpler too because some of it is just cut and paste).
Attachment #139822 - Flags: review?(varga)
Attached patch Fix selection nits (obsolete) — Splinter Review
Ok, so sometimes enable_buttons_for_current_panel could get confused...
Assignee: sgehani → neil.parkwaycc.co.uk
Status: NEW → ASSIGNED
Attachment #139908 - Flags: review?(varga)
Comment on attachment 139908 [details] [diff] [review]
Fix selection nits

r=varga, but please change |treeBoxObject.selection| to |view.selection|
Attachment #139908 - Flags: review?(varga) → review+
This has the xul part of the patch (from attachment 139822 [details] [diff] [review]).
Jan also asked me to replace treeBoxObject.selection with view.selection
Attachment #139822 - Attachment is obsolete: true
Attachment #139908 - Attachment is obsolete: true
Comment on attachment 139909 [details] [diff] [review]
Forgot the xul part of the patch :-[

Transferring Jan's r=
Attachment #139909 - Flags: superreview?(jag)
Attachment #139909 - Flags: review+
Blocks: 184540
Attachment #139822 - Flags: review?(varga)
Attached patch Fixed edge caseSplinter Review
OK, so when I failed to add an initial item to the tree I still marked it as
added making it impossible to readd :-(
Attachment #139909 - Attachment is obsolete: true
Comment on attachment 140237 [details] [diff] [review]
Fixed edge case

r=varga
Attachment #140237 - Flags: review+
Attachment #140237 - Flags: superreview?(alecf)
Comment on attachment 140237 [details] [diff] [review]
Fixed edge case

makes me wonder if panel.push() should take a true/false argument...
sr=alecf
Attachment #140237 - Flags: superreview?(alecf) → superreview+
Fix checked in.
Status: ASSIGNED → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
Attachment #139909 - Flags: superreview?(jag)
*** Bug 223673 has been marked as a duplicate of this bug. ***
*** Bug 237280 has been marked as a duplicate of this bug. ***
*** Bug 237477 has been marked as a duplicate of this bug. ***
*** Bug 237621 has been marked as a duplicate of this bug. ***
verified on build 2004031708
Status: RESOLVED → VERIFIED
*** Bug 239574 has been marked as a duplicate of this bug. ***
Product: Browser → Seamonkey
*** Bug 232126 has been marked as a duplicate of this bug. ***
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: