David Hyatt Reporter
	Description • 23 years ago

The persist attribute is being broadcast, and it should never be broadcast. 
This was broken during the whole XULElement attribute cleanup.  

This bug is critical for 0.9.6, since it will cause permanent corruption in the
localstore.rdf file, which will prevent you from ever getting column headers
back in a tree widget.

	David Hyatt Reporter
	Comment 1 • 23 years ago

Taking the liberty of targeting to 0.9.6.  Hopefully that's ok.

	Chris Waterson Assignee
	Comment 3 • 23 years ago

I think that ought to fix it. Can you tell me what to do to test?

	David Hyatt Reporter
	Comment 4 • 23 years ago

I can't see the surrounding code, but it looks like you're in a loop or 
something, so shouldn't it be if (!CanBroadcast) continue?

	Chris Waterson Assignee
	Comment 5 • 23 years ago

Oh good grief. Yes, it should.

	Chris Waterson Assignee
	Comment 6 • 23 years ago

So jag sez, to test this:

1. Bring up bookmarks.
2. Add an additional column (e.g., ``keyword'').
3. Close the window.
4. Open the window again.

Expected: keyword column appears
Actual: a big white hole where the keyword column should be

	Chris Waterson Assignee
	Comment 7 • 23 years ago

(And by the way, this patch doesn't fix that problem.)

	Chris Waterson Assignee
	Comment 9 • 23 years ago

So, the above test case shows that the patch fixes the |persist| pushing
problem, which may be necessary to fix this bug but doesn't appear to be sufficient.

	jag (Peter Annema)
	Comment 10 • 23 years ago

I think the column's supposed to come up white. The bug is that the header (that
thing that says "Keyword" for the keyword column) is coming up white when it
shouldn't.

	Chris Waterson Assignee
	Comment 12 • 23 years ago

You attached a PNG as text/plain, fokker! ;-)

But yeah, I still see white column headers with my patch (modified per hyatt)
that shouldn't be pusing |persist| anymore.

	timeless
	Comment 13 • 23 years ago

Comment on attachment 56861 [details]
screenshot displaying the problem

Thanks to the attachment manager errors like that can be fixed.

	Chris Waterson Assignee
	Comment 14 • 23 years ago

Pretty sneaky, sis! Yes, this patch _does not_ fix the problem.

	jag (Peter Annema)
	Comment 16 • 23 years ago

waterson: and that was on a clean profile, right?

Hmmm... Does localstore have anything for that element?

	Chris Waterson Assignee
	Comment 17 • 23 years ago

Yes, on a clean profile. This is what ends up in my localstore.rdf:

<RDF:Description
   about="chrome://communicator/content/bookmarks/bookmarks.xul#ShortcutURL"
   hidden="" />

	Chris Waterson Assignee
	Comment 18 • 23 years ago

So it looks like what is happening here is that the <treecell> is
|hidden="true"|, but the <treecol> is |hidden=""|. I suspect the problem has to
due with the fact that the persist information is set using a ``quiet'' update,
which doesn't trigger the document notification. Since the broadcaster
maintenance now happens during document notification, the empty string is
propagated from teh <treecol> to the <treecell>.

	Chris Waterson Assignee
	Comment 19 • 23 years ago

Okay, this fixes the bug. The problem was that we weren't doing a noisy notify
when restoring persisted attribute values. Since the broadcaster code now
relies on the AttributeChanged notification to work, we were missing these
updates. The fix is to make |ApplyPersistentAttributesToElements| do a noisy
notify. Unfortunately, to avoid sending RDF off into the weeds (gotta file a
separate bug), we need to make sure not to re-enter when the document is
notified, trying to re-persist the value that we just restored!

	David Hyatt Reporter
	Comment 20 • 23 years ago

sr=hyatt

	Chris Waterson Assignee
	Comment 21 • 23 years ago

cc'ing shaver since jag is awol.

	jag (Peter Annema)
	Comment 22 • 23 years ago

Comment on attachment 57183 [details] [diff] [review]
better fix

>Index: nsXULDocument.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/content/xul/document/src/nsXULDocument.cpp,v
>retrieving revision 1.455
>diff -u -r1.455 nsXULDocument.cpp
>--- nsXULDocument.cpp	2001/11/02 01:53:04	1.455
>+++ nsXULDocument.cpp	2001/11/09 03:06:39

>@@ -2002,9 +2017,7 @@
>     }
> 
>     // Synchronize broadcast listeners
>-    if (mBroadcasterMap &&
>-        (aNameSpaceID == kNameSpaceID_None) &&
>-        (aAttribute != nsXULAtoms::id)) {
>+    if (mBroadcasterMap && CanBroadcast(aNameSpaceID, aAttribute)) {

This change makes it so that if the namespace isn't "none", it will 
broadcast the attribute (which seems like a good change, but not what
the old code was doing).

If this was intended, r=jag.

	Chris Waterson Assignee
	Comment 23 • 23 years ago

This is what the old, old code was doing. (Well, it's what hyatt meant it to be
doing -- that code stifled |id|, |ref|, and |persist| on _any_ namespace, which
is overkill. See nsXULElement.cpp r1.371.)

	jag (Peter Annema)
	Comment 24 • 23 years ago

Comment on attachment 57183 [details] [diff] [review]
better fix

r=jag then :-)

	Christopher Blizzard (:blizzard)
	Comment 25 • 23 years ago

a=blizzard on behalf of drivers for 0.9.6

	Chris Waterson Assignee
	Comment 26 • 23 years ago

Fixed on the mozilla-0.9.6 branch.