Charles Manske Reporter
	Description • 23 years ago

The xul widget <menulist editable="true"> combines a textbox (input field) with
a popup menulist. The default behavior for this has been modified according
to expected standards. (See bug 82652).
The Form Manager's use of these widgets assumes that the selected item doesn't
change when the the menu is opened, so it needs to use set the attribute:
autoSelectMenuitem="false"
to retain it's previous behavior.

	Charles Manske Reporter
	Comment 1 • 23 years ago

Accepting -- patch comming soon.

	Charles Manske Reporter
	Comment 3 • 23 years ago

Steve: ready for your review. You must also apply the patch to fix 82652 before
testing this. Who is a good SR= candidate?

	Charles Manske Reporter
	Comment 4 • 23 years ago

Oops! I removed the debug dump line:
+dump("*** UpdateMenuListValue: menuItem.value = "+menuItem.value+"\n");
from my file.

	Stephen P. Morse
	Comment 6 • 23 years ago

cc'ing alecf for super-review.  I'll test this out right now, and if it works 
I'll add my r=.

Charlie, thanks for taking care of this

	Stephen P. Morse
	Comment 7 • 23 years ago

I applied the patch presented here, but was not able to apply the one in 
bug 82652 (see comments in that bug report).  With only this one patch applied, 
I'm still getting the wrong behavior.

	Alec Flett
	Comment 8 • 23 years ago

well, assuming the other bug makes this bug work, sr=alecf

	Stephen P. Morse
	Comment 9 • 23 years ago

Things are looking much better.  With the revised patch posted in bug 82652, the 
form-manager use of editable menulists is working fine again.

However the prefill dialog's use of editable menulists is broken.  To 
demonstrate that, do the following:

1. From menu, go to tasks->privacy->form-manager->demo
2. Click on sample 1
3. Fill in name field with "Steve"
4. From menu, click on edit->save-form-data
5. Change the name field to "Charlie"
6. Repeat step 4
7. Erase the value in the name field
8. From menu, click on edit->prefill-form

At this point you will see editable menulists for each field that can be 
prefilled -- in this case the name field.  That menulist will contain both 
"Steve" and "Charlie" as choices.

9. Select "Charlie" in the menulist and click OK.  "Charlie" gets prefilled as 
expected.

10. Repeat steps 8 & 9 but this time select "Steve".  It gets prefilled as 
expected.

11. Now repeats steps 8 & 9 but this time type an entirely new name into the 
editable menulist.  When you click OK, you'll see something get prefilled, but 
it won't be the new name that you typed.

	Stephen P. Morse
	Comment 10 • 23 years ago

The files in involved for the prefill dialog are those in 
extensions/wallet/walletpreview, specifically walletpreview.js and 
walletpreview.xul

	Stephen P. Morse
	Comment 11 • 23 years ago

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

	Stephen P. Morse
	Comment 12 • 23 years ago

Adding smoketest keyword.  See dup bug 82780 for details.

	Charles Manske Reporter
	Comment 13 • 23 years ago

This is a smoketest blocker? There's a lot of Composer funtionality that gets
busted routinely that isn't considered smoketest blockers!
All these problems will be fixed for 0.9.2

	Stephen P. Morse
	Comment 14 • 23 years ago

It's a smoketest blocker because it blocks B.25 section of the smoketest.  
That's what dup bug 82780 reported.

	Stephen P. Morse
	Comment 15 • 23 years ago

The changes to the last file (WalletPreview.js) in cmanske's posted patch is 
wrong.  Attaching a revised patch just for that last file.

r=morse for the other files in cmanske's patch.  Charlie, please do a review on 
my patch for WalletPreview.js.  Thanks.

	Charles Manske Reporter
	Comment 18 • 23 years ago

The latest patch (5/29) contains all earlier (already reviewed) changes plus
recent changes to WalletPreview only. r=cmanske on that.
One additional change is that I removed all onmousedown="Append(this)" handlers
for wallet dialogs. I don't think they're needed -- the "onchange" handler is
enough. Steve: My tests confirm this, but please test yourself to be sure.
When everthing is confirmed to work, please give your "r=" on the whole thing.

	Alec Flett
	Comment 19 • 23 years ago

so what's left for me to sr=? which patches are valid and have reviews?

	Charles Manske Reporter
	Comment 20 • 23 years ago

Alec: Use just the last patch. The only changes you really need to SR are
those in WalledPreview.js, and you assessment on removing all the "onmousedown"
handers from all the previously-reviewed dialogs (just about all the wallet
UI dialogs.)

	Alec Flett
	Comment 21 • 23 years ago

ok, looks good, sr=alecf

	Stephen P. Morse
	Comment 22 • 23 years ago

Looks bad to me.  I get a failure when I try to apply the latest composite 
patch.  Namely:

|Index: editor/WalletAddress.xul
|===================================================================
|RCS file: /cvsroot/mozilla/extensions/wallet/editor/WalletAddress.xul,v
|retrieving revision 1.4
|diff -u -r1.4 WalletAddress.xul
|--- WalletAddress.xul   2001/03/22 00:57:47     1.4
|+++ WalletAddress.xul   2001/05/29 16:01:44
--------------------------
Patching file WalletAddress.xul using Plan A...
Hunk #1 succeeded at 60.
Hunk #2 succeeded at 70.
Hunk #3 succeeded at 80.
Hunk #4 succeeded at 90.
Hunk #5 succeeded at 100.
patch: **** malformed patch at line 66:

	Charles Manske Reporter
	Comment 23 • 23 years ago

Did you back out any existing changes before applying new patch? I'll update and
supply a new one ASAP.

	Stephen P. Morse
	Comment 25 • 23 years ago

Of course I backed out the previous changes before applying the patch.  In fact, 
I backed out several times so that I could alternately apply your 5/25 patch and 
your 5/29 patch.  Each time I did so, the 5/25 patch worked (for all but the 
wallet preview of course) and the 5/29 patch did not work.

I'll fetch your latest patch and try again.

	Stephen P. Morse
	Comment 26 • 23 years ago

Same failure with newest patch.  **** malformed patch at line 66:

	Stephen P. Morse
	Comment 29 • 23 years ago

Problem is that the bad patch has a blank line where it shouldn't.  Namely in 
the @111 section, following <html>&addressZipcode.label;</html>

	Charles Manske Reporter
	Comment 30 • 23 years ago

Very weird! I don't see that extra line in the "Updated patch" (5/29/01 15:46).
Is it some weird lf/cr problem?
Can you generate a complete patch using your "Good subpatch" and the rest of
my updated patch?
Sorry, I'm just attaching what "cvs diff -u" gives me!

	Stephen P. Morse
	Comment 31 • 23 years ago

Do you not see the following lines when you view the 05/29/01 15:46 patch, with 
a blank line before <box>?


@@ -111,15 +111,15 @@
         <row>
           <html>&addressZipcode.label;</html>

           <box>

	Charles Manske Reporter
	Comment 32 • 23 years ago

Nope. I just re-saved that attachment to a new file and the blank line isn't
there.
I see:
@@ -111,15 +111,15 @@
         <row>
           <html>&addressZipcode.label;</html>
           <box>
-          <menulist id="postalcode.prefix" editable="true" flex="55%" width="0"
-                    onchange="Append(this)" onmousedown="Append(this)">
+          <menulist id="postalcode.prefix" editable="true"
autoSelectMenuitem="false" flex="55%" width="0"
+                    onchange="Append(this)">

So please just delete the blank line in yours and see if complete patch works.

	Stephen P. Morse
	Comment 33 • 23 years ago

Sorry, Charlie, but with the blank line removed I can get passed the patch-time 
error message but then the form-manager dialog doesn't work properly.  Original 
problem -- I can't add new entries to the editable menulist.

Can we stick with your 5/25 patch and my 5/27 ammendment?  That works fine.

r=morse for cmanske's 5/25 patch excluding the change to WalletPreview.js

	Alec Flett
	Comment 34 • 23 years ago

my sr= still stands, however you apply the patch.. 

	Charles Manske Reporter
	Comment 36 • 23 years ago

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

	Charles Manske Reporter
	Comment 37 • 23 years ago

r=kin

	Charles Manske Reporter
	Comment 38 • 23 years ago

That's SR=kin

	Stephen P. Morse
	Comment 39 • 23 years ago

r=morse

	Asa Dotzler [:asa]
	Comment 40 • 23 years ago

a= asa@mozilla.org for checkin to the trunk.
(on behalf of drivers)

	Charles Manske Reporter
	Comment 41 • 23 years ago

checked in.

	Blake Ross
	Comment 42 • 23 years ago

Now all of the panels in the dialog are empty for me (6/8 build, winXP).

	Blake Ross
	Comment 43 • 23 years ago

Also,

http://bonsai.mozilla.org/cvsview2.cgi?
diff_mode=context&whitespace_mode=show&subdir=mozilla/extensions/wallet/walletpr
eview&command=DIFF_FRAMESET&file=WalletPreview.js&rev1=1.16&rev2=1.17&root=/cvsr
oot

references 'data' twice.  Those should be 'value', although they probably 
aren't causing this problem (and existed before the change).

	Charles Manske Reporter
	Comment 44 • 23 years ago

Blake: Can you please be more specific? Which dialogs? (This work affected all
the "Wallet" dialogs.)
The "data" usage existed before, and it's ok to leave that.

	Blake Ross
	Comment 45 • 23 years ago

Oops, sorry -- the Form Manager (Edit | View Saved Data).

Data was an old menulist property that has since been changed to value.  That 
must have been a missed case; it really should be changed...

	Charles Manske Reporter
	Comment 46 • 23 years ago

Ok, I see the missing dialog widgets. I'm getting the JS error:
chrome://communicator/content/wallet/WalletViewer.js line 124: elementIDs has no
 properties
when dialog loads.
I don't see how my changes could have caused this; it worked during last testing
about a week ago.
Investigating...

	Charles Manske Reporter
	Comment 47 • 23 years ago

This problem is not caused by my changes, but by the fix to bug 62730.
This relocated the Wallet UI from "content" to "locale" directory.
Maybe this is a build problem?

	Charles Manske Reporter
	Comment 48 • 23 years ago

Adding depends on bug 62730

	Charles Manske Reporter
	Comment 49 • 23 years ago

Since there's a new bug to cover the problems with Form Manager dialogs,
bug 85300, I'm marking this fixed again.
Verification is dependent on getting 85300 fixed, but this bug is fixed.

	Stephen P. Morse
	Comment 50 • 23 years ago

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

	Terri Preston
	Comment 51 • 23 years ago

Verified fix checked into lxr.mozilla.org

	Stephen P. Morse
	Comment 52 • 23 years ago

Form manager's use of editable menulists is once again broken.  You can't enter 
several items into a menulist.

To reproduce:

go to edit->view-saved-data
point curson on any of the editable menulists on the page
type in some text
open the menulist and select the blank entry
type in some more text
open the menulist again

expected result:

the previously entered text should appear as one of the selections in the list

actual result:

the previously entered text is not in the list

	Stephen P. Morse
	Comment 53 • 23 years ago

Problem appears to be that the onchange handler for the editable menulist never 
gets called.

	Stephen P. Morse
	Comment 54 • 23 years ago

However if you do the following, the onchange handler is getting called and the 
menulist behaves as it is supposed to (you are able to enter several values into 
the menulist):

go to edit->view-saved-data
point curson on any of the editable menulists on the page
type in some text
select another menulist
select the original menulist again and open it

At this point, the original value entered is seen as one of the values inside 
the menulist.

	Stephen P. Morse
	Comment 55 • 23 years ago

The patch that cmanske checked in (5/30/01 10:31 New patch for all wallet 
changes) makes the following two changes to all the form-manager's editable 
menulists:

1. adds the attribute autoSelectMenuitem="false"
2. removes the onmousedown="Append(this)" attribute

If I back off the second part of the patch (restore the onmousedown attribute), 
everything works fine again.

	Stephen P. Morse
	Comment 56 • 23 years ago

It's quicker to describe my proposed patch than to generate it.  It's simply to 
back out 1/2 of cmanke's patch, namely the half that deals with removing the 
onmousedown attributes.  In other words, restoring the attribute 
onmousedown="Append(this)" in each place that charlie's patch removed it.

cmanske, blake: please r and sr this proposed patch.  thanks.

	Charles Manske Reporter
	Comment 57 • 23 years ago

I don't support the proposed change (adding back the "onmousedown" handlers).
Since someone broke the "onchange" handlers, we should fix that instead.
They worked before and still work in the 0.9.2 (NS RTM) branch build.

	Charles Manske Reporter
	Comment 58 • 23 years ago

Ok, I see what's going on. It only occurs in Classic. The reason I didn't like 
the "onmousedown" fix is because it might be hiding an important problem in the 
"onchange" handling. So after thinking about it, I'm not sure if onchange should
fire when user opens the menulist (it does in Modern, it doesn't in Classic.)
The "onmousedown" fix isn't adequate, since user can open the menulist by 
pressing the down arrow key as well -- in that case the Append() method wouldn't
fire. The solution to get the behavior you want when menulist is opened, you
must add "Append(this.parentNode)" to each <menupopup> elements under
each <menulist> that uses onchange="Append(this)"
The really bad news is that this bug does happen in the RTM branch, also just
in Classic skin.

	Blake Ross
	Comment 60 • 23 years ago

Milestone reality check.

	Joe Hewitt (gone)
	Comment 61 • 23 years ago

sr=hewitt

	Stephen P. Morse
	Comment 62 • 23 years ago

r=morse if it really works (I haven't had the time to test it out).

	Charles Manske Reporter
	Comment 63 • 23 years ago

New patch from 7/24/01 checked in.
We should probably add a release note for the 0.9.2 branch (NS rtm build)

	Terri Preston
	Comment 64 • 23 years ago

Verified Fixed Win XP build 2002013003