41143 - Mail compose only allows 4 addresses

Status: VERIFIED FIXED

(MailNews Core :: Composition, defect, P3)

Description

[selmer (gone)]

Edit a new message.  Add several names (allowing autocomplete).  After entering
the 4th address the next address box does not appear.  No keys bring up the
address box.  It is apparently impossible to add more than 4 addresses.

Comment 1

[selmer (gone)]

This is really pretty bad.  nominating for beta2.

Comment 2

[lchiang]

I think this is dogfood.  I'm fairly certain this used to work.

Comment 3

[Jean-Francois Ducarroz]

This is a new regression!

Comment 4

[laurel]

Still broken using jun01 commercial build on linux rh6.0 and mac OS 9.0. 
Marking All platforms.

Comment 5

[laurel]

By the way, I know this used to work.  A few weeks ago I verified a couple bugs
involving the address block and scrolling and used 6-8 addressee lines.

Comment 6

[Alec Flett]

adding hyatt... this could be a wierd XBL thing.

Comment 7

[(not reading, please use [e-mail] instead)]

when I get to the fourth address and try to enter a fifth address, I see this
error on the console:

************************************************************
** NOTE: This report will only be printed in DEBUG builds.**
* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "[JS Error: "TypeError: popupset.firstChild.openPopup is not a
function"] [nsIAutoCompleteListener::onAutoComplete]"  nsresult: "0x80570021
(<unknown>)"  location: "JS frame :: <unknown filename> :: anonymous :: line
56"  data: yes]
************************************************************

I'll investigate now.

Comment 8

[(not reading, please use [e-mail] instead)]

ignore that, I get that when ever I type an address, and hit return.

still investigating...

Comment 9

[leger]

Putting on [dogfood+] radar.

Comment 10

[(not reading, please use [e-mail] instead)]

ok, still dumpster diving.

in awReturnHit() in addressingWidgetOverlay.js, we aren't adding the row because
inputElement.value is ""

still trying to figure stuff out...

Comment 11

[(not reading, please use [e-mail] instead)]

work around in hand.  not sure what's going on.  I'll post a patch.

Comment 12

[(not reading, please use [e-mail] instead)]

here's a work around.  I'm not sure why this used to work, and not it doesn't.

can someone review?  then, we can worry about why this broke.

Index: addressingWidgetOverlay.js
===================================================================
RCS file:
/cvsroot/mozilla/mailnews/compose/resources/content/addressingWidgetOverlay.js,v
retrieving revision 1.23
diff -p -r1.23 addressingWidgetOverlay.js
*** addressingWidgetOverlay.js	2000/05/11 00:30:18	1.23
--- addressingWidgetOverlay.js	2000/06/01 23:40:53
*************** function awReturnHit(inputElement)
*** 218,224 ****
  {
  	var row = awGetRowByInputElement(inputElement);
  	
! 	if ( inputElement.value )
  	{
  		var nextInput = awGetInputElement(row+1);
  		if ( !nextInput )
--- 218,225 ----
  {
  	var row = awGetRowByInputElement(inputElement);
  	
! 	//commented out to work around bug #41143
! 	//if ( inputElement.value )
  	{
  		var nextInput = awGetInputElement(row+1);
  		if ( !nextInput )

Comment 13

[Alec Flett]

ok, seth and I debugged further. This is an editor bug. Here's what's going on:
- the <textfield> is getting created offscreen (i.e. not visible in the tree, so
lazily created)
- the tree scrolls it into view, which creates the frames for the <textfield>
- from this point forward, the ".value" attribute of the <textfield> returns an
empty string ("") no matter what the user actually types in.

this means that even with the above workaround, when you actually send the
message, any recipients after the first three actually recieve the mail... so we
are NOT checking in the above workaround.

This is an editor bug, something having to do with creating ender widgets
offscreen and then bringing them into view. reassigning to mjudge because he's
been poking around alot with ender widget stuff..
(Mike - this is one of the M16 stoppers!)

Comment 14

[laurel]

By the way,  at some times this bug will stop you from going past ONE addressee
line.  I hit this earlier on all my NT profiles, but since nbaca could go to
four lines I thought my machine was in a funk.  Esther has since hit the one
address line impasse on some of her profiles. We did a little more testing and
found that it hits randomly -- sometimes you can get to 4 lines, sometimes only
1. 

Soo.... reminder to QA to verify over several profiles, several tries to make
sure this random one line impasse is also fixed.

Comment 15

[leger]

Putting on [M16Blocker]radar

Comment 16

[esther]

No, it's not fixed with today's builds 6-2-00

Comment 17

[fenella]

Linux (2000-06-02-08 M16)
Win32 (2000-06-02-09 M16)
Mac (2000-06-02-08 M16)
Jan. I see the bug in all 3 platforms in today's builds.

Comment 18

[(not reading, please use [e-mail] instead)]

QA:  no one has claimed this bug has been fixed.

Comment 19

[fenella]

Seth, fyi....
Jan, our boss, has asked me to check this bug on today's builds.

Comment 20

[fenella]

Linux (2000-06-05-08 M16) Bug is still there.

Comment 21

[fenella]

Win32 (2000-06-05-09 M16)
Mac (2000-06-05-08 M16)
Bug is still there.

Comment 22

[rubydoo123]

this should really be managed by mail/news team

Comment 23

[mjudge]

reassigning back to me. i see the editor reference now.

Comment 24

[Scott MacGregor]

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

Comment 25

[leger]

Setting to Blocker/M16...per mjudge:
"This is some kind of really bad thing that trees is doing. I can look at it but 
i dont think it will be fixed until the new tree stuff and ender-lite lands."

Comment 26

[mjudge]

i can fix this. ender-lite makes this work now.  I dont know if i want to do 
that on a branch.  you can get around this by simply using a comma (,) in 1 
field to address more than 1 person per line also.

Comment 27

[fenella]

Please see bug 41596, only the first 3 addresses receive the mail. So what good
would it do to add more addresses by using the comma (,)?

Comment 28

[Alec Flett]

I know it's scary to do such a landing on the branch, but this is serious data
loss, especially, as fenella pointed out, because the 4th line's address doesn't
even receive the message even though it looks like it does.

There isn't any hacky solution we can do for M16 now that it has branched?

Comment 29

[rubydoo123]

but wait, I entered this in the 1st address block:
[e-mail],[e-mail],[e-mail],[e-mail],pe
[e-mail]

and everyone got the note, that is 5 people. We can release note this that folks 
should enter multiple recipients in one address line as a comma separated list.

Comment 30

[lchiang]

Since this is a Mozilla build/milestone (M16), cc Asa for his input.

New message scenario:
Basically if you send to more than 4 people, you need to use commas to separate
the email addresses on one line instead of spanning across 4 addressing lines.

Reply message scenario:
I also tested this out on a reply-all.  If I select a message which has more
than 4 recipients and do a reply-all, I get more than 4 addressing fields and am
actually able to press enter after the last addressing field to add a new field
and enter an address.  But, after sending, the last recipient I added does not
get the message.

This last scenario is not so obvious to the user since he may not realize that
there is something wrong right away and urgent messages may not be sent to the
intended party.  With the first scenario, we can get away with release noting
since it's obvious to the user that there is a problem (ie. can't get a new
address line which will prompt him/her to look at the release notes).

Comment 31

[Asa Dotzler [:asa]]

the reply all part of this bug scares me. Mail has come a long way in the last
couple of months, but a bug like this could sour users to the product in a big
way. mjudge, what concerns you about landing this fix on a branch?

Comment 32

[Alec Flett]

I agree. This workaround is not sufficient because it requires the user have 
prior knowledge of this bug. This bug makes M16 unusable for mail.

Comment 33

[scottputterman]

I think the usage pattern that this bug breaks is too common to have the relaese
notes be effective.  I don't think we should hold M16 forever for this, but if
we don't get it in, it will make M16 mail pretty unreliable.

Comment 34

[mjudge]

ok. i am turning on ender-lite on the trunk today. I will then pull a branch 
tree to try to hack the old nsGfxTextControlFrame.cpp to get it to work. I dont 
have any high hopes here but i will do whatever i can.

Comment 35

[Alec Flett]

thanks for the effort, I know it's a pain.

Comment 36

[mjudge]

turns out the editor code is working fine. I will keep helping out to find out 
whats going on, but it looks like its in the tree widget.  the creation of the 
Nth element (where N > number of visible rows) is failing.  getprop JS operator 
hits this new element it calls into some kind of xul callback code wheras the 
previous nodes call into js_GetProperty. i will have better details tomorrow. I 
need hyatt and/or brendan's help on this i believe.

Comment 37

[mjudge]

adding brendan to the cc list.

Comment 38

[Brendan Eich [:brendan]]

Hyatt, what's different about the offscreen row, from an XBL or JS point of 
view?  I thought this might be some JS property cache coherence bug, or the 
JS_SetPrototype bug, but now I'm not so sure.

/be

Comment 39

[Brendan Eich [:brendan]]

Mjudge: what was the exact name of that "XUL callback function" (a property 
getter) that got called after a property cache hit?  And what's the *GetProperty 
function that you saw called indirectly via js_GetProperty in the on-screen or 
not-clipped cases?

/be

Comment 40

[(not reading, please use [e-mail] instead)]

the js patch doesn't fix the problem.

Comment 41

[(not reading, please use [e-mail] instead)]

this bug and 41686 are related.  I'm sure of it.

Comment 42

[mjudge]

appendNewRow in addressingnWidgetOverlay.js is calling 
inputElement.value="" and causing an access of a js property on an element that 
has no frame yet. Brendan Hyatt and I looked at this for about 5 hours today and 
finally saw what happened.  This cannot be fixed on the branch since some of the 
CSSProperty methods are not implemented yet.

One possible fix is to remove the .value = ""; i believe this was put in there 
force a NON-ender-lite editor to create itself immediately.  Since we cannot 
turn ender-lite on in the branch (it is on as of today on the trunk) and since 
the CSSProperty methods are not implemented yet (and wont be on the branch) I 
dont believe we can solve this satisfactorily(sp).

still, try removing the value = "" from awAppendNewRow and see if the resulting 
behaviour is better than what is happening now.

Comment 43

[(not reading, please use [e-mail] instead)]

adding myself to the cc list.

I'll try that work around.

Comment 44

[(not reading, please use [e-mail] instead)]

do you mean:

input[0].value = "";

on line 61?

ducarroz comes back tomorrow.  before we check in any work arounds, we should 
run it by him.

(I tried the work around, at it seems to fix this problem, but related bug 
#41686 still remained.)

Comment 45

[rubydoo123]

assigning this bug to spitzer, it has been narrowed down and mail team needs to 
manage the workaround

Comment 46

[(not reading, please use [e-mail] instead)]

ok.  I got a workaround for both this problem and #41686

(they are basically the same workaround.)

Index: addressingWidgetOverlay.js
===================================================================
RCS file: 
/cvsroot/mozilla/mailnews/compose/resources/content/addressingWidgetOverlay.js,v
retrieving revision 1.24
diff -p -r1.24 addressingWidgetOverlay.js
*** addressingWidgetOverlay.js	2000/06/07 01:05:50	1.24
--- addressingWidgetOverlay.js	2000/06/12 13:58:17
*************** function _awSetInputAndPopup(inputValue,
*** 157,163 ****
  		//We need to set the value using both setAttribute and .value 
else we will
  		// loose the content when the field is not visible. See bug 
37435
  	    input[0].setAttribute("value", inputValue);
! 	    input[0].value = inputValue;
  	    input[0].setAttribute("id", "msgRecipient#" + top.MAX_RECIPIENTS);
  	}
      var select = newNode.getElementsByTagName(awSelectElementName());
--- 157,164 ----
  		//We need to set the value using both setAttribute and .value 
else we will
  		// loose the content when the field is not visible. See bug 
37435
  	    input[0].setAttribute("value", inputValue);
! 		//comment this out as a workaround for bug #41143
! 	    //input[0].value = inputValue;
  	    input[0].setAttribute("id", "msgRecipient#" + top.MAX_RECIPIENTS);
  	}
      var select = newNode.getElementsByTagName(awSelectElementName());
*************** function awAppendNewRow(setFocus)
*** 258,264 ****
  			//We need to set the value using both setAttribute and 
.value else we will
  			// loose the content when the field is not visible. See 
bug 37435
     			input[0].setAttribute("value", "");
!    			input[0].value = "";
      	    input[0].setAttribute("id", "msgRecipient#" + top.MAX_RECIPIENTS);
      	}
          var select = newNode.getElementsByTagName(awSelectElementName());
--- 259,266 ----
  			//We need to set the value using both setAttribute and 
.value else we will
  			// loose the content when the field is not visible. See 
bug 37435
     			input[0].setAttribute("value", "");
! 			//comment this out as a workaround for bug #41143
! 			//input[0].value = "";
      	    input[0].setAttribute("id", "msgRecipient#" + top.MAX_RECIPIENTS);
      	}
          var select = newNode.getElementsByTagName(awSelectElementName());

Comment 47

[(not reading, please use [e-mail] instead)]

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

Comment 48

[(not reading, please use [e-mail] instead)]

I just backed out my workaround. it causes #37435 to regress.

re-assigning to ducarroz so he can sort it all out.

I'm not sure what to do about this bug.

Comment 49

[(not reading, please use [e-mail] instead)]

adding mjudge back to the cc list.

Comment 50

[Jean-Francois Ducarroz]

I am working on it as soon I have a fresh build up and running...

Comment 51

[Jean-Francois Ducarroz]

Some info: when I replace the textfield by an HTML:input element, I don't see 
the problem anymore. Maybe something to to with XBL (a text field is an XBL 
widget that use an HTML input element"

Comment 52

[Brendan Eich [:brendan]]

Yes, there's a bug (XBL bindings apply at frame construction time, which breaks 
code that gets script objects for content that has no frame yet, but where the 
script needs the bindings to be applied).  Trouble is, we can't fix that bug for 
M16, but we would like a workaround for M16.

/be

Comment 53

[Jean-Francois Ducarroz]

I will try to rewrite the fix for bug 37435 to let us use this patch. In another 
word, I will try to merge both fixes.

Comment 54

[Jean-Francois Ducarroz]

Ok, I have a work around that works for new message and reply as well (you can 
also add recipients to a reply). In fact we need to apply only the second part 
of Seth's path as the first part is used during replies and the second is only 
for when we add new empty rows which will affected bug 37435.

Comment 55

[Jean-Francois Ducarroz]

Seth, can you review this:



Index: addressingWidgetOverlay.js

===================================================================

RCS file: /cvsroot/mozilla/mailnews/compose/resources/content/addressingWidgetOverlay.js,v

retrieving revision 1.24

diff -c -2 -r1.24 addressingWidgetOverlay.js

*** addressingWidgetOverlay.js	2000/06/07 01:05:50	1.24

--- addressingWidgetOverlay.js	2000/06/12 21:40:09

***************

*** 256,265 ****

          if ( input && input.length == 1 )

          {

! 			//We need to set the value using both setAttribute and .value else we will

! 			// loose the content when the field is not visible. See bug 37435

!    			input[0].setAttribute("value", "");

!    			input[0].value = "";

      	    input[0].setAttribute("id", "msgRecipient#" + top.MAX_RECIPIENTS);

!     	}

          var select = newNode.getElementsByTagName(awSelectElementName());

          if ( select && select.length == 1 )

--- 256,262 ----

          if ( input && input.length == 1 )

          {

!    			  input[0].setAttribute("value", "");

      	    input[0].setAttribute("id", "msgRecipient#" + top.MAX_RECIPIENTS);

!     	  }

          var select = newNode.getElementsByTagName(awSelectElementName());

          if ( select && select.length == 1 )

Comment 56

[Jean-Francois Ducarroz]

oops, looks like this path wont fix bug 41686!

Comment 57

[Jean-Francois Ducarroz]

Attachment: Proposed patch

Comment 58

[Jean-Francois Ducarroz]

I have a new patch that seems to fix all 3 bugs. Instead of setting the dot 
value of the text field during its offline creation to avoid bug 37435 (which 
cause indirectly bug 41143 and bug 41686), I check if the content of dot value 
is empty when I query the field's content, if it's the case I use intead the 
getAttribute("value") which always contains the original value set during the 
initialization.

I have tested this patch only on Mac, My Windows build isn't in a working state 
right now!! Seth, can you test it on Unix & window for me and in the same time 
do a code review. Thanks

Comment 59

[(not reading, please use [e-mail] instead)]

that patch seems to work for me on win32.

ducarroz:  I'm get a ton of assertions when replying to messages.
are you seeing that.

(###!!! ASSERTION: |CharAt| out-of-range: 'aIndex<Length()', file 
..\..\dist\include\nsAReadableString.h, line 544)

Comment 60

[Jean-Francois Ducarroz]

Work around checkin in the M16 branch as well in the trunk

Comment 61

[Jean-Francois Ducarroz]

About the assertion, I seem them, but not a tone, only once. It's append way 
after the code I have just modified and therefore without any relation!

Comment 62

[fenella]

Linux (2000-06-13-08 M17)
This bug is fixed in this build.

Comment 63

[laurel]

This also seems ok using 2000-06-13-20 m17 commercial on NT 4.0.

I'm still seeing a problem using 2000-06-14-13 m17 mac build -- hitting Enter
after the third line actually adds another line but doesn't go/scroll there. 
Scrollbar is left at the top and if you use the scrollbar to go down, you'll see
there is another line.

I'm also having problems with the reply all scenario/duplicate of this bug, but
am investigating further to make sure.

I'm not sure I trust the build, though, so will check next build.

Comment 64

[(not reading, please use [e-mail] instead)]

putterman, could that problem (not scrolling to the newly added address field)
be the same as the "Next Message" scrolling problem?

Comment 65

[scottputterman]

it could be if we're using ensureelementisvisible.

Comment 66

[laurel]

The newer linux m17 commercial build 2000-06-14-12  exhibits the same behavior
for New Message window as I described for the mac.  It acts like a 4th header
line is not added upon Enter, but if you scroll you will see the new line. 

If we need to spinoff a separate bug, please let me know

Comment 67

[David Hyatt]

ensureElementIsVisible was broken in the old tree in order to make it work in 
the new tree.

Comment 68

[Jean-Francois Ducarroz]

Yes, the addressing widget use ensureElementIsVisible.

Comment 69

[lchiang]

This bug is so long...

Laurel - let's file a new bug with your findings and past putterman and hyatt's 
comments in them.

Comment 70

[laurel]

New scroll issue moved to bug #42674. 

Comment 71

[esther]

Using builds 2000-06-21 on win98, mac and linux we can add more than 4 names in 
the addressing area.  We scroll and get an empty field for the next name when 
addressing area is filled up.  So this bug as originally stated is fixed and I 
will verify.