282645 - address autocomplete doesn't work for replies (but works for new compose)

Status: RESOLVED FIXED

(Thunderbird :: Message Compose Window, defect)

Description

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

address autocomplete doesn't work for replies (but works for new compose)

I'm using a trunk tbird build from ftp.mozilla.org, "version 0.6+ (20050217)"

on win xp.

I'm pretty sure this is a recent regression.

Comment 1

[Scott MacGregor]

i see this too... neil tells me seamonkey works fine. 

Comment 2

[Scott MacGregor]

the first autocomplete row for a reply works fine. It's all the subsequent rows
that have a dead widget

Comment 3

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

fwiw, I see this on my mac mozilla tbird build, too.

Comment 4

[Michael Baffoni]

I am seeing this in Seamonkey on XP( : Mozilla 1.8b2 Mozilla/5.0 (Windows; U;
Windows NT 5.1; en-US; rv:1.8b2) Gecko/20050220), and I concur - the first
address line works fine, but all subsequent addresses fail.  I wonder if this is
related to the bug where you lose auto-complete after scrolling the address list?

If possible, please up the severity as this is a pretty high-profile bug.

Comment 5

[Michael Baffoni]

FWIW, autocomplete works fine on both new email compositions and forwards.

Comment 6

[Scott MacGregor]

Keeping Neil in the loop on this issue too as seamonkey's autocomplete widget is
also experiencing the problem.

Comment 7

[Scott MacGregor]

it's interesting..only rows 2, 3 and 4 are dead for autocomplete on a reply.
The first row autocompletes and rows 5 and above also autocomplete correctly. 

Comment 8

[Scott MacGregor]

It looks like autocomplete rows created by awAppendNewRow are still working.
But autocomplete rows created by awCreateOrRemoveDummyRows which trys to add a
widget for every visible row in the envelope aren't working anymore.

Comment 9

[Scott MacGregor]

ignore my last comment. 

Comment 10

[Scott MacGregor]

The problem is happening here:

http://lxr.mozilla.org/mozilla/source/mailnews/compose/resources/content/addressingWidgetOverlay.js#542

If we have an existing dummy row and we are calling listbox.replaceChild then we
end up with a dead auto complete instance. If we call listbox.appendChild, then
we end up with an autocomplete instance which behaves correctly.

Comment 11

[Scott MacGregor]

Backing out Bug #272002 makes the mail autocomplete widget start working again.

Comment 12

[[e-mail]]

Attachment: Improve on bug 272002

If that patch really caused the bug, maybe this will fix it.

Comment 13

[Scott MacGregor]

Comment on attachment 175332 [details] [diff] [review]
Improve on bug 272002

Hey Brian, this patch by Neil fixes mail autocomplete which was broken by the
changes to nsTextControlFrame in Bug #272002

Comment 14

[Brian Ryner (not reading)]

(In reply to comment #13)
> (From update of attachment 175332 [details] [diff] [review] [edit])
> Hey Brian, this patch by Neil fixes mail autocomplete which was broken by the
> changes to nsTextControlFrame in Bug #272002
> 

I don't really understand how this helps. How is it that the text frame never
gets an initial reflow?  Or is the NS_FRAME_FIRST_REFLOW check bogus, or is it
that the box code does not propagate this state through to non-box frames?

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Could there be notifications that need to get through before the first reflow?

Comment 16

[Boris Zbarsky [:bzbarsky]]

Hmm...  So what could be happening here is the following:

1)  We get into the nsTextControlFrame::SetValue _before_ we saw a first reflow
2)  We store our old mNotifyOnInput value (false).
3)  We set mNotifyOnInput to false.
4)  We set the value on the editor.  Editor synchronously flushes out reflows
    and paints (see bug 259636).  This does our initial reflow and sets
    mNotifyOnInput to true.
5)  We unwind and restore the old mNotifyOnInput state (false).
6)  We never have an initial reflow again, so the state stays false.

I'd forgotten about step 4, since I'm used to the way the code looks in my tree.  :(

Does the patch in bug 272002 keep us from firing oninput for the initial setting
of the value?  If so, I suspect that Neil's patch is the right way to go.

Comment 17

[Brian Ryner (not reading)]

Yeah, the steps Boris gave seem to be what's happening, in the case where an
input is inserted into the document dynamically.  One thing that concerns me
about the patch is that it basically gets rid of whatever the original intent of
mNotifyOnInput was.  What do you think about this instead; when restoring
mNotifyOnInput from oldNotify, do:

mNotifyOnInput |= oldNotify;

That will handle the case where the editor insertion enables notifications.

Comment 18

[Boris Zbarsky [:bzbarsky]]

That could work, as long as we never reenter SetValue.  But again, I suspect the
initial purpose of mNotifyOnInput was to not fire oninput handlers when setting
the default value of the input (at frame init time).  Or something like that. 
The checkin comment says it was for bug 42045 and bug 42675, neither of which
has useful information on why the check was put in place...

Comment 19

[Brian Ryner (not reading)]

How would reentering SetValue cause that not to work?

Comment 20

[Brian Ryner (not reading)]

I guess you're thinking of the case where a nested call to SetValue causes
oninput notiifcation to be unsuppressed for the outer call?

Comment 21

[Boris Zbarsky [:bzbarsky]]

Right.

Comment 22

[Brian Ryner (not reading)]

Yet another possibility here would be to put the editor into async update mode
from SetValue().  (InitEditor() does this).  This would prevent the synchronous
reflow condition that's causing us to clobber mNotifyOnInput.  I can't think of
any real drawbacks to this.

Comment 23

[Boris Zbarsky [:bzbarsky]]

See bug 259636 for exactly that proposal, along with kin's listing of the
drawbacks...

Comment 24

[Scott MacGregor]

guys, I'm hearing lots of great debate here on how you want to fix it which is
great. But I'd really like to get this regression fixed ASAP. It really makes
mail composition extremely hard to use right now. Can we back out the bug that
caused this until everyone agrees on the right way to fix it or have we reached
consensus and can fix it ASAP? Thanks!

Comment 25

[Brian Ryner (not reading)]

Attachment: proposed solution

This patch makes it so that we don't unsuppress oninput notifications until
we've fully unwound from processing transactions.  I tested the mail compose
autocomplete case and the firefox username autocomplete that was the original
problem, and both seem to work.

Comment 26

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 175770 [details] [diff] [review]
proposed solution

This is ok, but I do think that Neil's proposal is cleaner, and should work as
well....

Comment 27

[[e-mail]]

FYI my patch has the added advantage of fixing bug 276882.

Comment 28

[Brian Ryner (not reading)]

Comment on attachment 175770 [details] [diff] [review]
proposed solution

Yeah... let's use Neil's patch instead.  As long as we supress oninput for all
scripted editor transactions, it shouldn't be possible to fire oninput until
we've had a reflow, because the user won't be able to interact with the editor.

Comment 29

[Brian Ryner (not reading)]

(In reply to comment #26)
> (From update of attachment 175770 [details] [diff] [review] [edit])
> This is ok, but I do think that Neil's proposal is cleaner, and should work as
> well....
> 

Though doesn't this patch also have the problem where a reentrant call to
SetValue could set mNotifyOnInput = PR_TRUE before the outer call completes?

Comment 30

[Boris Zbarsky [:bzbarsky]]

Yes, it does (should be simpler to fix the issue there, though).

Comment 31

[Brian Ryner (not reading)]

Attachment: neil's patch w/ recursion protection

Comment 32

[[e-mail]]

Attachment: Even simpler recursion protection?

Personally I can't actually figure out what we're protecting against; surely
suppressing the oninput should prevent a recursive notification?

Is there any mileage in using PRUint8 mSuppressInput?

Comment 33

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 175796 [details] [diff] [review]
neil's patch w/ recursion protection

sr=bzbarsky.  I think I prefer this to adding several extra bytes to every
textframe...

As for how this could happen, editor listeners (attached by someone with
sufficient privileges) could catch the change even in the absence of an input
event....

Comment 34

[Brian Ryner (not reading)]

(In reply to comment #32)
> Created an attachment (id=175805) [edit]
> Even simpler recursion protection?
> 
> Personally I can't actually figure out what we're protecting against; surely
> suppressing the oninput should prevent a recursive notification?
> 
> Is there any mileage in using PRUint8 mSuppressInput?

This patch will add 4 bytes per TextControlFrame, which I'd like to avoid.

Comment 35

[Brian Ryner (not reading)]

Comment on attachment 175805 [details] [diff] [review]
Even simpler recursion protection?

minusing per above comment

Comment 36

[Scott MacGregor]

I see the latest patch for this regression fix got minused. Brian, maybe we
should be looking at backing out the change that broke autocomplete if we think
fixing the regression is going to take more time to figure out the right thing.
This is a pretty serious mail regression that's been broken since the 15th of
February. I think we've been pretty patient but we really need to get this
figured out one way or the other. 

Comment 37

[[e-mail]]

Comment on attachment 175796 [details] [diff] [review]
neil's patch w/ recursion protection

It looks as if mInTransaction == !mNotifyOnInput; if you agree then please make
the necessary optimization.

Comment 38

[Brian Ryner (not reading)]

checked in attachment 175796 [details] [diff] [review] with the optimization neil suggested

Comment 39

[R.K.Aa.]

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

Comment 40

[R.K.Aa.]

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

Comment 41

[R.K.Aa.]

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

Comment 42

[R.K.Aa.]

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

Comment 43

[R.K.Aa.]

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

Comment 44

[Mike Cowperthwaite]

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

Comment 45

[R.K.Aa.]

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

Comment 46

[Frank Sauer]

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

Comment 47

[Mark Banner (:standard8)]

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