40617 - OK button doesn't function in prefs after certain steps

Status: VERIFIED FIXED

(SeaMonkey :: Preferences, defect, P1)

Description

[Blake Ross]

Build ID: 2000052508

might be dup.

The OK button in the preferences ceases to function after these steps:

(1) Open Preferences (from Edit | Preferences...)
(2) Expand Advanced (if not already done)
(3) Click Desktop Integration
(4) Click Mouse Wheel
(5) Change the "When" dropdown to anything
(6) Attempt to press OK

Button depresses and appears to function visually, but seemingly nothing 
happens. 

The following appears in the console:

JavaScript error:
chrome://communicator/content/pref/pref-winhooks.js line 53: settings is not def
ined

bryner, i know you handle the mousewheel prefs, though this prob has nothin to 
do w/ you so feel free to remove from cc if you want.

cc'ing rogerl

Comment 1

[Blake Ross]

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

Comment 2

[Peter 'Rufus' Nelson]

WFM on win98 2000072608.  I followed the steps exactly, and didn't get any JS
errors, and could press OK.

Comment 3

[sairuh (rarely reading bugmail)]

feel free to verify these (or reopen, if still a problem) if i don't get around
to it beforehand. if you do so, pls do remove the verifyme keyword after
verifying a given bug. thx!

Comment 4

[Alan S. Jones]

Reopening as i see the problem on win95, mozilla build 2000081108 following
Blake's steps

Comment 5

[Andrei Zmievski]

This also happens on Mozilla build 2000080923 on Linux (RedHat 6.2).

Comment 6

[Blake Ross]

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

Comment 7

[Blake Ross]

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

Comment 8

[Blake Ross]

fixing up like bug 55371

Comment 9

[Blake Ross]

OK, I think this happens because settings isn't defined within the scope of onOK
(), but only onOK() is hooked into the prefs "OK" button.  If I'm not mistaken, 
the fix would be to put something like var dataObject = 
parent.hPrefWindow.wsm.dataManager.pageData
["chrome://communicator/content/pref/pref-winhooks.xul"]; in onOK(), and then 
change instances of settings to dataObject.settings.  But when I did this (and 
a couple other things), I get a crash in APPCOMPS.DLL.  So...

Comment 10

[Bill Law]

Blake was on the right track.  onOK is called after the window is gone; thus
*all* the global variables in pref-winhooks.js (which are  window properties)
become undefined.  I suspect your patch for settings just got to tne next
reference to either winhooks or prefs.

I'll attach a more comprehensive patch that stores the globals in the parent
window (per Ben's recommendation).  If anybody qualified can review (Blake,
matt, mcafee), I'll then ask Don to promote this to rtm+.

Comment 11

[Blake Ross]

Cool.  Actually, I changed all the references to settings, winhooks, and prefs 
in the function.  It was crashing with the line winhooks.settings = 
dataObject.settings (had already redeclared winhooks).  Go figure.

Anyways, sure, send your patch my way when it's done and I'll review.  Thanks.

Comment 12

[Bill Law]

Attachment: Proposed fix

Comment 13

[Bill Law]

I've appealed to Blake (review) and Ben (super-review) for this patch.  When 
they've had a chance to respond, we'll update to [rtm+] (presuming there's no 
problem).

Note that the consequence of this bug is that any panels visited after the 
"Desktop Integration" one will not be properly saved when the user finally says 
Ok.

Comment 14

[sairuh (rarely reading bugmail)]

hrm...if changes to other panels visted after going to Desktop Integration
cannot be saved, that would be a big regression. one of the few dogfood tasks
w/prefs  (imho :) is being able to do changes in more than one panel, and having
those multiple changes saved upon hitting OK. alternatives (other than not
fixing this)? thx!

Comment 15

[Blake Ross]

Hmm.  If I change prefs in the Desktop Integration pane and then switch to 
another pane, although the OK button now functions (without any console 
errors), the Desktop Integration prefs don't seem to save -- they're back to 
their old values when I reopen prefs.  Looking into that now...

Comment 16

[Blake Ross]

Just got a mid-air collision with Sarah.  Bill, when you said `Note that the 
consequence of this bug is that any panels visited after the 
"Desktop Integration" one will not be properly saved when the user finally says 
Ok.', did you mean that that happens after your patch is applied?  I thought 
you were just restating the effects of this bug to PDT.

Comment 17

[Bill Law]

Yes, I was describing more specifically what was going wrong, before the fix is 
applied.  Previous descriptions talked about the JS errors on the console but 
didn't really say what the impact was on the user.

Have you verified that your Desktop Integration prefs stick, Blake?  It worked 
for me, but I might have other Desktop Integration patches in my tree that 
otherwise affect things.

Logic-wise, the patch should not affect what happens with respect to the Windows 
registry.  I'll bang on this more myself later this morning.

Comment 18

[Bill Law]

Attachment: New and improved patch

Comment 19

[Bill Law]

I've attached a proper patch that handles redisplaying the panel without
forgetting what had been entered previously.  This should work much better.
Looking to Blake and Ben for reviews.

Comment 20

[Blake Ross]

Hmm.

Now when I make changes in the Desktop Integration pane and then press OK 
without changing panes, my Desktop Integration changes aren't saved.  They are, 
however, now saved if I switch panes after making the changes.

Comment 21

[Bill Law]

Argh. I've fixed that bug; too many patches to the same set of files...

The fix here was to insert a call to GetFields in onOK to ensure that those
fields are gotten (GetFields is automagically called if the user goes to a
different panel). It is in this latest attachment (presuming I've attached the
right one this time).

Comment 22

[Bill Law]

Attachment: The patch that really is new and improved

Comment 23

[Blake Ross]

Ah, okay.  I was wondering why GetFields was added but not called.  retesting 
now...

Comment 24

[Blake Ross]

patch looks good, works great. r=blake (thanks for removing the alert()'s, also)

Comment 25

[Alec Flett]

ok, and sr=alecf

Comment 26

[don]

PDT, i think we need this.

Comment 27

[Blake Ross]

PDT, just to clarify: it's easier to reproduce this bug than the original steps 
suggest. All you have to do is open the "Desktop Integration" panel in 
preferences, and then go to any other panel, at which point the OK button won't 
work anymore (and thus the changes the user made are lost).

Comment 28

[Blake Ross]

OK, Bill checked this into the trunk.  Sarah, please be on the lookout for any 
problems/regressions which come as a result of this patch.  If there are no 
problems, and the patch works for you, PDT can ++.

Comment 29

[sairuh (rarely reading bugmail)]

if someone could check this with a trunk build (dunno if it's in a recent opt
build yet --i've only grabbed this am's branch bits so far...), pls add your
comments here. thx!

Comment 30

[Phil Peterson]

I'd like to know what the trunk testing results are before marking rtm++, since
there's a fair amount of code here. Marking [rtm need info] until test results
are available.

Comment 31

[Blake Ross]

Works like a charm for me (the reporter) in a new trunk build on win98, in all 
cases.

Comment 32

[Phil Peterson]

Talked to Blake and he did a whole bunch of testing; marking [rtm++]

Comment 33

[Blake Ross]

Bill, I checked this in to the branch because I fear PDT's vengeance on Monday 
(I expect them to minus a lot of bugs).  Hope you don't mind.

Comment 34

[Bill Law]

No problem; help is always appreciated.

I am working on landing another patch to the same file, though.  I'm not sure
whether that's going to be easier or harder now :-).  Easier, let's hope; I
think I can just join the latest from the tip (before, I was going to have to
get there in two steps and the steps were in opposite order that they were taken
on the tip).  See bug 27187.

Comment 35

[Bill Law]

Hey, either you're one step ahead of me, Blake, or you messed up :-).  I went to
check in my patch to pref-winhooks.js for bug 27187 and it turns out you had
already done it.  I believe you should have only applied the changes between
revs 1.3 and 1.4 (not 1.2 to 1.4).  What you checked in would not have worked
without the corresponding change to pref-winhooks.xul.

But no harm, no foul.  I've now checked in the .xul so all will be well.

Comment 36

[Bill Law]

Check that; I should have said "you should have only applied the changes between
revs 1.2 and 1.3 (not 1.2 to 1.4)."

Comment 37

[Blake Ross]

Yeah, I just wrote you about this.  Apparently my CVS tree tag was messed up 
and when I updated the file before checking in, it updated against the trunk 
version of the file, thereby giving me merge conflicts.  So I manually added 
the "showDialog" that was part of your change to pref-winhooks.js, thinking you 
had already checked that into the branch (and so I didn't realize I was 
actually adding it).  So when I did my cvs -z3 diff -u before checking in, it 
didn't show that I was adding the "showDialog" line, because it was once again 
comparing to the trunk version.  I don't know why that happened, sorry.

We'll just pretend I'm one step ahead of you and did it on purpose ;)

Comment 38

[sairuh (rarely reading bugmail)]

works now --vrfy'd using commercial branch bits on winnt, 2000.10.25.09-n6. but
do lemme know if this needs an additional check on win98 (i can do that from
home, but would be using mozilla bits).

Comment 39

[sairuh (rarely reading bugmail)]

vrfy fixed using 2000.11.13.08 opt comm *trunk* bits on winnt.