217916 - advanced options: expanding/collapsing fun

Status: RESOLVED FIXED

(Firefox :: Settings UI, defect)

Description

[Steffen Wilberg]

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.

Comment 1

[Oleg Sidletskiy]

perhaps 0.7? since it doesn't look like it's too difficult to fix...

Comment 2

[Steffen Wilberg]

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

Comment 3

[Sean Umphlet (nosebleed)]

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.

Comment 4

[PikeUK]

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.

Comment 5

[PikeUK]

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.

Comment 6

[Steffen Wilberg]

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

Comment 7

[PikeUK]

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>

Comment 8

[Steffen Wilberg]

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.

Comment 9

[PikeUK]

Attachment: Patch to add fixes from comment #4

This was done with cvs diff -u toolkit/content/widgets/expander.xml I hope
that's right?

Comment 10

[Jon Henry]

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 11

[Steffen Wilberg]

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.

Comment 12

[PikeUK]

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

Comment 13

[Steffen Wilberg]

Fine!
Request review by clicking edit next to your patch, select "?" next to review
and enter the reviewer of your choice, e.g. [e-mail].
-> Pike.

Comment 14

[Mike Connor [:mconnor]]

blockers at this stage are showstoppers only.  This is really trivial and will
certainly be fixed for 0.9

Comment 15

[Mike Connor [:mconnor]]

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

Comment 16

[Jon Henry]

You might try pinging Blake for the review ([e-mail]), he's going
on a streak of checking in older patches that have been waiting for a while.

Comment 17

[Jon Henry]

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.

Comment 18

[Mike Connor [:mconnor]]

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.

Comment 19

[Mike Connor [:mconnor]]

Comment on attachment 136737 [details] [diff] [review]
Patch with fixed spacing and location

looks good, r=me

Comment 20

[Mike Connor [:mconnor]]

0.9 won't have the rewrite, but this is a general bug that should be fixed in
the toolkit anyway.

Comment 21

[Steffen Wilberg]

Mike, you didn't check this in yet, did you?

Comment 22

[Mike Connor [:mconnor]]

checked in branch and trunk 2004-05-06 17:52

Comment 23

[PikeUK]

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

Comment 24

[Steffen Wilberg]

Hmm, I've seen this a lot lately:
this.setAttribute("open", "true");
...
this.removeAttribute("open");
...
if (this.hasAttribute("open"))

Comment 25

[PikeUK]

Attachment: 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?

Comment 26

[Mike Connor [:mconnor]]

sorry for bugspam, long-overdue mass reassign of ancient QA contact bugs,
filter on "beltznerLovesGoats" to get rid of this mass change