Closed Bug 217916 Opened 21 years ago Closed 20 years ago

advanced options: expanding/collapsing fun

Categories

(Firefox :: Settings UI, defect)

defect
Not set
minor

Tracking

()

RESOLVED FIXED

People

(Reporter: steffen.wilberg, Assigned: me)

References

Details

(Whiteboard: fixed0.9)

Attachments

(2 files, 1 obsolete file)

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.
>  !"false" is equal to false
I might misunderstand you, but this prints "true":
  <script type="text/javascript">
    var test=false;
    test=!test;
    document.write(test);
  </script>
Sorry I'm bad at explaining, I mean the string "false" evaluates to the boolean
true, so !"false" evaluates to false.

Try this:

<script type="text/javascript">
  var test=!"false";
  document.write(test);
</script>
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.
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.
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. bugs@bengoodger.com.
-> Pike.
Assignee: blake → pike
Attachment #136737 - Flags: review?(bugs)
Flags: blocking0.8?
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 (firefox@blakeross.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.
Flags: blocking0.9?
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.
Whiteboard: checkin0.9
Mike, you didn't check this in yet, did you?
checked in branch and trunk 2004-05-06 17:52
Status: NEW → RESOLVED
Closed: 20 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"))
Attached patch Alternative fixSplinter Review
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.

Attachment

General

Creator:
Created:
Updated:
Size: