82655 - Modify Form Manager dialogs to use new editable menulist widgets

Status: VERIFIED FIXED

(Toolkit :: Form Manager, defect)

Description

[Charles Manske]

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.

Comment 1

[Charles Manske]

Accepting -- patch comming soon.

Comment 2

[Charles Manske]

Attachment: Use autoSelectMenuitem="false" in all dialogs to supress default behavior of editable menulist widget

Comment 3

[Charles Manske]

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

Comment 4

[Charles Manske]

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

Comment 5

[Charles Manske]

Attachment: Updated patch with all debug "dumps" removed

Comment 6

[Stephen P. Morse]

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

Comment 7

[Stephen P. Morse]

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.

Comment 8

[Alec Flett]

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

Comment 9

[Stephen P. Morse]

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.

Comment 10

[Stephen P. Morse]

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

Comment 11

[Stephen P. Morse]

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

Comment 12

[Stephen P. Morse]

Adding smoketest keyword.  See dup bug 82780 for details.

Comment 13

[Charles Manske]

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

Comment 14

[Stephen P. Morse]

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

Comment 15

[Stephen P. Morse]

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.

Comment 16

[Stephen P. Morse]

Attachment: patch for WalletPreview.js only -- rest of cmanske's patch above still stands

Comment 17

[Charles Manske]

Attachment: Patch to fix all wallet UI problems (includes latest from morse)

Comment 18

[Charles Manske]

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.

Comment 19

[Alec Flett]

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

Comment 20

[Charles Manske]

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

Comment 21

[Alec Flett]

ok, looks good, sr=alecf

Comment 22

[Stephen P. Morse]

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:

Comment 23

[Charles Manske]

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

Comment 24

[Charles Manske]

Attachment: Updated patch. (No real changes, just more recent)

Comment 25

[Stephen P. Morse]

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.

Comment 26

[Stephen P. Morse]

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

Comment 27

[Stephen P. Morse]

Attachment: Good subpatch (this is not a patch to be reviewed)

Comment 28

[Stephen P. Morse]

Attachment: Bad subpatch (this is not a patch to be reviewed)

Comment 29

[Stephen P. Morse]

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

Comment 30

[Charles Manske]

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!

Comment 31

[Stephen P. Morse]

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>

Comment 32

[Charles Manske]

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.

Comment 33

[Stephen P. Morse]

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

Comment 34

[Alec Flett]

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

Comment 35

[Charles Manske]

Attachment: New patch for all wallet changes: mjudge tested applying this patch and it seems to work.

Comment 36

[Charles Manske]

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

Comment 37

[Charles Manske]

r=kin

Comment 38

[Charles Manske]

That's SR=kin

Comment 39

[Stephen P. Morse]

r=morse

Comment 40

[Asa Dotzler [:asa]]

a= [e-mail] for checkin to the trunk.
(on behalf of drivers)

Comment 41

[Charles Manske]

checked in.

Comment 42

[Blake Ross]

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

Comment 43

[Blake Ross]

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

Comment 44

[Charles Manske]

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.

Comment 45

[Blake Ross]

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

Comment 46

[Charles Manske]

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

Comment 47

[Charles Manske]

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?

Comment 48

[Charles Manske]

Adding depends on bug 62730

Comment 49

[Charles Manske]

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.

Comment 50

[Stephen P. Morse]

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

Comment 51

[Terri Preston]

Verified fix checked into lxr.mozilla.org

Comment 52

[Stephen P. Morse]

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

Comment 53

[Stephen P. Morse]

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

Comment 54

[Stephen P. Morse]

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.

Comment 55

[Stephen P. Morse]

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.

Comment 56

[Stephen P. Morse]

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.

Comment 57

[Charles Manske]

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.

Comment 58

[Charles Manske]

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.

Comment 59

[Charles Manske]

Attachment: Fixes problems in Classic skin by always calling Append() when <menupopup> is opened.

Comment 60

[Blake Ross]

Milestone reality check.

Comment 61

[Joe Hewitt (gone)]

sr=hewitt

Comment 62

[Stephen P. Morse]

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

Comment 63

[Charles Manske]

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

Comment 64

[Terri Preston]

Verified Fixed Win XP build 2002013003