advanced options: expanding/collapsing fun

RESOLVED FIXED

Status

()

Firefox
Preferences
--
minor
RESOLVED FIXED
15 years ago
12 years ago

People

(Reporter: Steffen Wilberg, Assigned: PikeUK)

Tracking

unspecified
Points:
---
Bug Flags:
blocking0.8 -
blocking0.9 -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed0.9)

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

15 years ago
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

15 years ago
perhaps 0.7? since it doesn't look like it's too difficult to fix...
(Reporter)

Comment 2

15 years ago
*** 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.
(Assignee)

Comment 4

14 years ago
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.
(Assignee)

Comment 5

14 years ago
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.
(Reporter)

Comment 6

14 years ago
>  !"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>
(Assignee)

Comment 7

14 years ago
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>
(Reporter)

Comment 8

14 years ago
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.
(Assignee)

Comment 9

14 years ago
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?

Comment 10

14 years ago
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).
(Reporter)

Comment 11

14 years ago
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.
(Assignee)

Comment 12

14 years ago
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
(Reporter)

Comment 13

14 years ago
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
(Assignee)

Updated

14 years ago
Attachment #136737 - Flags: review?(bugs)

Updated

14 years ago
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. ***

Comment 16

14 years ago
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

Comment 17

14 years ago
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
(Reporter)

Comment 21

14 years ago
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: 14 years ago
Resolution: --- → FIXED
Whiteboard: checkin0.9 → fixed0.9
(Assignee)

Comment 23

14 years ago
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).
(Reporter)

Comment 24

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

Comment 25

14 years ago
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.