Closed Bug 41143 Opened 24 years ago Closed 24 years ago

Mail compose only allows 4 addresses

Categories

(MailNews Core :: Composition, defect, P3)

Product:
MailNews Core
Component:
Composition
Type:
defect

Tracking

(Not tracked)

Status:
VERIFIED FIXED

People

(Reporter: selmer, Assigned: bugzilla)

References

Details

(Whiteboard: [dogfood+][nsbeta2+][M16Blocker] Fix in hand)

Attachments

(1 file)

Proposed patch
4.09 KB, patch
Details | Diff | Splinter Review 
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.
This is really pretty bad.  nominating for beta2.
Keywords: nsbeta2
Severity: normal → major
QA Contact: lchiang → esther
I think this is dogfood.  I'm fairly certain this used to work.
Severity: major → critical
Keywords: dogfood
This is a new regression!
Status: NEW → ASSIGNED
Target Milestone: --- → M17
Still broken using jun01 commercial build on linux rh6.0 and mac OS 9.0. 
Marking All platforms.
OS: Windows NT → All
Hardware: PC → All
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.
adding hyatt... this could be a wierd XBL thing.
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.
ignore that, I get that when ever I type an address, and hit return.

still investigating...
Putting on [dogfood+] radar.
Whiteboard: [dogfood+][nsbeta2+]
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...
work around in hand.  not sure what's going on.  I'll post a patch.
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 )
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!)
Assignee: ducarroz → mjudge
Status: ASSIGNED → NEW
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.
Putting on [M16Blocker]radar
Whiteboard: [dogfood+][nsbeta2+] → [dogfood+][nsbeta2+][M16Blocker]
No, it's not fixed with today's builds 6-2-00
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.

QA:  no one has claimed this bug has been fixed.
Seth, fyi....
Jan, our boss, has asked me to check this bug on today's builds.

Linux (2000-06-05-08 M16) Bug is still there.
Win32 (2000-06-05-09 M16)
Mac (2000-06-05-08 M16)
Bug is still there.
this should really be managed by mail/news team
Assignee: mjudge → mscott
reassigning back to me. i see the editor reference now.
Assignee: mscott → mjudge
*** Bug 41596 has been marked as a duplicate of this bug. ***
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."
Severity: critical → blocker
Target Milestone: M17 → M16
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.
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 (,)?

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?
but wait, I entered this in the 1st address block:
akkana@netscape.com,janc@netscape.com,hamerly@netscape.com,dawnr@netscape.com,pe
tersen@netscape.com

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


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

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.

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.
Status: NEW → ASSIGNED
thanks for the effort, I know it's a pain.
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.
adding brendan to the cc list.
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
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
the js patch doesn't fix the problem.
this bug and 41686 are related.  I'm sure of it.
Blocks: 41686
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.
adding myself to the cc list.

I'll try that work around.
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.)

assigning this bug to spitzer, it has been narrowed down and mail team needs to 
manage the workaround
Assignee: mjudge → sspitzer
Status: ASSIGNED → NEW
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());
*** Bug 41686 has been marked as a duplicate of this bug. ***
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.
Assignee: sspitzer → ducarroz
adding mjudge back to the cc list.
I am working on it as soon I have a fresh build up and running...
Status: NEW → ASSIGNED
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"
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
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.
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.

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 )


oops, looks like this path wont fix bug 41686!
Attached patch Proposed patch — — Splinter Review 

    
    

    
  
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
Whiteboard: [dogfood+][nsbeta2+][M16Blocker] → [dogfood+][nsbeta2+][M16Blocker] Fix in hand

    
    

    
  
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)

    
    

    
  
Work around checkin in the M16 branch as well in the trunk
Status: ASSIGNED → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED

    
    

    
  
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!

    
    

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

    
    

    
  
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.

    
    

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



    
    

    
  
it could be if we're using ensureelementisvisible.

    
    

    
  
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

    
    

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


    
    

    
  
Yes, the addressing widget use ensureElementIsVisible.

    
    

    
  
This bug is so long...

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

    
    

    
  
New scroll issue moved to bug #42674. 


    
    

    
  
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.
Status: RESOLVED → VERIFIED
Product: MailNews → Core
Product: Core → MailNews Core

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: