User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.5b) Gecko/20030831 Firebird/0.6.1+ Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.5b) Gecko/20030831 Firebird/0.6.1+ In Tools-Options-Advanced, the upper 4 sections won't stay collapsed. And you have to click twice on Certificates and Validation to expand. Reproducible: Always Steps to Reproduce: 1. Go to Tools-Options-Advanced. 2. Collapse all sections. Click OK. 3. Go to Tools-Options-Advanced. Click OK. 4. Go to Tools-Options-Advanced. Actual Results: The upper 4 sections are expanded again. Expected Results: They should still be collapsed. Related problem: You have to click twice on Certificates and Validation to expand. Steps to reproduce: Collapse both of them. Click OK. Go to Options-Advanced and try to expand them.
perhaps 0.7? since it doesn't look like it's too difficult to fix...
*** Bug 218191 has been marked as a duplicate of this bug. ***
Some addtional notes. If you go to reproduce this and expand just the second and fourth one (Browsing and Security). I noticed that the second, third (Multimedia) and fourth ones get expanded but the first one does not. This is not always reproducable. Sometimes the top four all get expanded.
This appears to be one of those "false" == true things. In expander.xml is this code: "this.open = !this.open;" on line 55. Unfortunately the default value of the certificates and validation section is "false", !"false" is equal to false, so the first click changes open from "false" to false. I think the lack of persistence is because of "this.removeAttribute("open");" on line 39, which prevents the persist="open" from working properly. I don't know the best way to fix this but changing line 39 to "this.setAttribute("open", "false");" and line 55 to "if (this.open == "true") this.open = false; else this.open = true;" seems to work OK.
I didn't explain that very clearly: ...so the first click changes open from "false" to false... What I meant is the first click causes boolean false to be sent to the setter which sets the attribute to "", so really I should have said: ...the first click changes open from "false" to ""... Sorry about that.
I see. The changes you suggest in comment 4 work fine. persist="open" works with the change in line 39. The change in line 52 ensures that you don't have to click twice on Certificates and Validation. The same can be accomlished by removing open="false" from prev-advanced.xul. But your change is more fail-proof. Are you going to provide a patch? I can do it as well if you want.
Created attachment 136729 [details] [diff] [review] Patch to add fixes from comment #4 This was done with cvs diff -u toolkit/content/widgets/expander.xml I hope that's right?
I've been curious as to the cause of this bug for a while, in retrospect this seems like an obvious fix :) I'll apply the patch and do a build, but it looks like this should do the trick. You should ask for a review from one of the Firebird peers (http://www.bengoodger.com/software/mb/review.html), other than perhaps making the line spacing and indenting on your if statements match the rest of the file I'd say it looks great (not that my opinion counts on that).
Comment on attachment 136729 [details] [diff] [review] Patch to add fixes from comment #4 >Index: toolkit/content/widgets/expander.xml cvs diff -u is right, but run it from above mozilla/, so that the path is mozilla/toolkit/ >+ if (this.open == "true") this.open = false; >+ else this.open = true; Use separate lines like: if (this.open == "true") this.open = false; else this.open = true; break; This does not only look better, it's also the style of the file, see e.g. lines 36-39.
Created attachment 136737 [details] [diff] [review] Patch with fixed spacing and location Thanks fixed the spacing. p.s. I don't normally use this style because I always forget to add the braces when I add a new line :-)
Attachment #136729 - Attachment is obsolete: true
Fine! Request review by clicking edit next to your patch, select "?" next to review and enter the reviewer of your choice, e.g. firstname.lastname@example.org. -> Pike.
Assignee: blake → pike
blockers at this stage are showstoppers only. This is really trivial and will certainly be fixed for 0.9
Flags: blocking0.8? → blocking0.8-
*** Bug 229167 has been marked as a duplicate of this bug. ***
You might try pinging Blake for the review (email@example.com), he's going on a streak of checking in older patches that have been waiting for a while.
OS: Windows XP → All
Hardware: PC → All
Per Ben's post on MozillaZine, I'm nominating this for attention. It has a patch that looks like it works, which is always a bonus.
ben asked for 1.0 blockers, not 0.9 blockers. Also, this whole section will be gone with the prefpanel rewrite, so the patch is soon to be obsolete anyway, and 0.9 will have said rewrite.
Flags: blocking0.9? → blocking0.9-
Comment on attachment 136737 [details] [diff] [review] Patch with fixed spacing and location looks good, r=me
Attachment #136737 - Flags: review?(bugs) → review+
0.9 won't have the rewrite, but this is a general bug that should be fixed in the toolkit anyway.
Mike, you didn't check this in yet, did you?
checked in branch and trunk 2004-05-06 17:52
Status: NEW → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED
Whiteboard: checkin0.9 → fixed0.9
Thanks Mike. Now I look at this patch again though I can't help thinking the better fix would've been to make the open getter return a boolean instead of a string (the obvious problem with that route is that it changes the expander "API", so maybe this fix was better, I can't decide).
Hmm, I've seen this a lot lately: this.setAttribute("open", "true"); ... this.removeAttribute("open"); ... if (this.hasAttribute("open"))
Created attachment 148013 [details] [diff] [review] Alternative fix This is what I meant in my last comment, one problem with the current expander is doing something like: myExpander.open = myExpander.open; does not preserve the state of the expander, instead it opens it. This is because the open property expects a boolean in its getter but its setter returns a string (which will always evaluate to true as explained above). Is it worth doing something like this as well or am I worrying over nothing?
sorry for bugspam, long-overdue mass reassign of ancient QA contact bugs, filter on "beltznerLovesGoats" to get rid of this mass change
QA Contact: mconnor → preferences
You need to log in before you can comment on or make changes to this bug.