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

Status: VERIFIED FIXED

(SeaMonkey :: Sidebar, defect)

Description

[sujay]

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.

Comment 1

[Peter Trudelle]

This is bad, but are any users likely to do it?

Comment 2

[Peter Trudelle]

nsbeta1- per Nav triage team.

Comment 3

[Jacobo Tarrio]

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

Comment 4

[sujay]

*** Bug 145328 has been marked as a duplicate of this bug. ***

Comment 5

[Jay Farrell]

I did this as part the of the front-end tests, see the dup bug I just opened!
Bug 145328

Comment 6

[sujay]

*** Bug 147792 has been marked as a duplicate of this bug. ***

Comment 7

[sujay]

*** Bug 148865 has been marked as a duplicate of this bug. ***

Comment 8

[Steve]

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?

Comment 9

[Philip K. Warren]

Attachment: Small fix

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.

Comment 10

[Philip K. Warren]

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.

Comment 11

[R.K.Aa.]

*** Bug 163430 has been marked as a duplicate of this bug. ***

Comment 12

[Samir Gehani]

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

Comment 13

[Philip K. Warren]

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.

Comment 14

[sujay]

*** Bug 180865 has been marked as a duplicate of this bug. ***

Comment 15

[sujay]

*** Bug 182022 has been marked as a duplicate of this bug. ***

Comment 16

[R.K.Aa.]

*** Bug 182230 has been marked as a duplicate of this bug. ***

Comment 17

[Kyle Yuan]

*** Bug 186101 has been marked as a duplicate of this bug. ***

Comment 18

[Kyle Yuan]

Attachment: simpler fix

always have an empty treechildren in the customize.xul file. this will avoid
tree.getElementsByTagName("treechildren")[1] to be an undefined object.

Comment 19

[Kyle Yuan]

Comment on attachment 109824 [details] [diff] [review]
simpler fix

this patch doesn't work.

Comment 20

[Samir Gehani]

Comment on attachment 94545 [details] [diff] [review]
Small fix

Chnaging review requestee to Shuehan since she now works on sidebar.

Comment 21

[Bryan Wright]

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.

Comment 22

[Dac Chartrand]

This happened to me today! Mozilla 1.5b

Bryan Wright: thanks for the workaround.

Comment 23

[Philip K. Warren]

Adding Neil who may have a better idea for how to fix this bug.

Comment 24

[[e-mail]]

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

Comment 25

[[e-mail]]

Attachment: Proposed patch

Can someone please test this for me?

Comment 26

[Philip K. Warren]

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

Comment 27

[[e-mail]]

Attachment: Fixed patch

Ah, that would be because I patched the wrong tree :-[

I've had time to test this now, it works this time!

Comment 28

[Philip K. Warren]

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.

Comment 29

[[e-mail]]

Strange, all the treeitems appear in the DOM, but view.rowCount is 1 ...
Jan, any ideas?

Comment 30

[rbs]

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.

Comment 31

[Boris Zbarsky [: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?

Comment 32

[Philip K. Warren]

Yes - the problems described in comment 28 need to be resolved before this 
patch can be checked in.

Comment 33

[[e-mail]]

Attachment: Ugly hack

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.

Comment 34

[Andreas Kunz]

Adjusting summary, there are lots of duplicates...

Comment 35

[Andreas Kunz]

*** Bug 203612 has been marked as a duplicate of this bug. ***

Comment 36

[Andreas Kunz]

*** Bug 224015 has been marked as a duplicate of this bug. ***

Comment 37

[Chase Tingley]

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

Comment 38

[Andreas Kunz]

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

Comment 39

[[e-mail]]

Attachment: Based on bug 218640 comment 0

Comment 40

[Jan Varga [:janv]]

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

Comment 41

[jag (Peter Annema)]

Comment on attachment 139201 [details] [diff] [review]
Based on bug 218640 comment 0

sr=jag

Comment 42

[[e-mail]]

OK, checked in the workaround, but leaving open to properly fix the RDF.

Comment 43

[Stefan Borggraefe]

*** Bug 231385 has been marked as a duplicate of this bug. ***

Comment 44

[Simon Paquet [:sipaq]]

Comment on attachment 94545 [details] [diff] [review]
Small fix

Removing obsolete review request

Comment 45

[[e-mail]]

Oh, and apologies to Kyle for not reading his nonworking patch before "copying"
it :-( and to Phillip for not reading comment 9 carefully enough :-/

Comment 46

[Kyle Yuan]

Neil, you are always faster to catch the right thing than I do :) Good to see
this bug gets fixed.

Comment 47

[[e-mail]]

Attachment: Do things properly

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 48

[[e-mail]]

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

Comment 49

[[e-mail]]

Attachment: Fix selection nits

Ok, so sometimes enable_buttons_for_current_panel could get confused...

Comment 50

[Jan Varga [:janv]]

Comment on attachment 139908 [details] [diff] [review]
Fix selection nits

r=varga, but please change |treeBoxObject.selection| to |view.selection|

Comment 51

[[e-mail]]

Attachment: Forgot the xul part of the patch :-[

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

Comment 52

[[e-mail]]

Comment on attachment 139909 [details] [diff] [review]
Forgot the xul part of the patch :-[

Transferring Jan's r=

Comment 53

[[e-mail]]

Attachment: Fixed edge case

OK, so when I failed to add an initial item to the tree I still marked it as
added making it impossible to readd :-(

Comment 54

[Jan Varga [:janv]]

Comment on attachment 140237 [details] [diff] [review]
Fixed edge case

r=varga

Comment 55

[Alec Flett]

Comment on attachment 140237 [details] [diff] [review]
Fixed edge case

makes me wonder if panel.push() should take a true/false argument...
sr=alecf

Comment 56

[[e-mail]]

Fix checked in.

Comment 57

[Andreas Kunz]

*** Bug 223673 has been marked as a duplicate of this bug. ***

Comment 58

[R.K.Aa.]

*** Bug 237280 has been marked as a duplicate of this bug. ***

Comment 59

[R.K.Aa.]

*** Bug 237477 has been marked as a duplicate of this bug. ***

Comment 60

[R.K.Aa.]

*** Bug 237621 has been marked as a duplicate of this bug. ***

Comment 61

[Grace Bush]

verified on build 2004031708

Comment 62

[Bill Mason]

*** Bug 239574 has been marked as a duplicate of this bug. ***

Comment 63

[[e-mail]]

*** Bug 232126 has been marked as a duplicate of this bug. ***