306076 - Edit menu in Compose window has many grayed out items the first time you use it after app start

Status: VERIFIED FIXED

(MailNews Core :: Composition, defect)

Description

[Ruediger Lahl]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20050826 SeaMonkey/1.1a
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20050826 SeaMonkey/1.1a

If you start SM, the 'Rewrap-Function' in the MailNews Composerwindow is grayed
out. This only happend for the very first start of the composerwindow, after you
started SM. If you close the composerwindow and open it again, 'Rewrap' is
enabeled and stays enabeled until you restart SM again.

Reproducible: Always

Steps to Reproduce:
1.Start SM-MailNews
2.Reply any Mail/Posting
3.Rewrap is not enabled
4.Close Composerwindow and open it again
5.Rewrap now is enabled for the rest of the MailNews Session.
Actual Results:  
Rewrap is not enabled at the first start of the composerwindow

Expected Results:  
Rewrap should be enabled at the first start of the composerwindow too.

This Bug appears between Build 2005082304 and 2005082403.

The Bug also appears in an actual Linux-SM-Nightly.
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a1) Gecko/20050826 SeaMonkey/1.1a

Comment 1

[Frank Wein [:mcsmurf]]

Bonsai Link:
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=MozillaTinderboxAll&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2005-08-23+03%3A00%3A00+&maxdate=2005-08-24+05%3A00%3A00&cvsroot=%2Fcvsroot

Comment 2

[Ruediger Lahl]

(In reply to comment #1)
> Bonsai Link:
[...]

Thanks Frank. My knowlege is to small to find a proper checkin in this timeslot.

Comment 3

[Frank Wein [:mcsmurf]]

It seems not only Rewrap is broken, but some more items (i noticed Paste). This
can be reproduced in Thunderbird, too.
When setting mail.compose.max_recycled_windows to 0 (so disabling cached compose
window), you can reproduce this bug every time you open a compose window.

Comment 4

[Frank Wein [:mcsmurf]]

Ok, after some testing it might be that mail.compose.max_recycled_windows is not
so related after all.
I also found out how to "fix" this (workaround): Open a Compose window, observe
grayed out items, switch away to another window, switch back to the Compose
window and you'll see that the menu items are no longer grayed out.

Comment 5

[Ruediger Lahl]

(In reply to comment #4)

> I also found out how to "fix" this (workaround): Open a Compose window, observe
> grayed out items, switch away to another window, switch back to the Compose
> window and you'll see that the menu items are no longer grayed out.

I can confirm this. When I open the addressbook and switching back to the
Compose window, the menu items are no longer grayed out.

Frank, thanks for the bug-busting.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Sounds focus-related...  Given that this is also broken in Thunderbird, it'd be
good to get to the bottom of this.  Especially if whatever checkin broke it is
aiming to land on branch.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Actually, I just did some testing and this broke between 2005-08-24-07 and
2005-08-25-07 as far as I can tell.  In that range the most likely culprit is
bug 303620.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Though I just tested a local backout of that bug and that didn't seem to help...

Comment 9

[Boris Zbarsky [:bzbarsky]]

OK, this was caused by the last patch for bug 249136; I've verified this via
local backout.

Over to aaron, and requesting blocking 1.8b4, since that patch has already been
approved for the branch.

Comment 10

[Aaron Leventhal]

I won't be around until Tuesday. Maybe Mats can look at this.

If it's urgent we can back out bug 249136 on the branch until this is understood.

Comment 11

[Mats Palmgren (inactive)]

I can't say I understand exactly what is going on yet, but I have some clues...
This fixes it:
   // we need this to update command, including the case where there is no element
   // because nsPresShell::UnsuppressPainting may have just now unsuppressed
   // focus on the currently focused window
-  if (!mSuppressFocus) {
+  if (!mSuppressFocus && mCurrentElement) {
     UpdateCommands();
   }

I suspect this brings back at least some of the problems in bug 249136 though,
so it's no good.

The odd thing is that when it is *working* we never reach the update line:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/dom/src/base/nsFocusController.cpp&rev=1.46&root=/cvsroot&mark=222#194

If we reach it, which we do if we remove the 'mCurrentElement' above, then
the bug occurs. So I suspect this is a matter of timing - we do it to early.
Could it be that we have a valid document but that the message body has not
loaded yet and that there is no notifaction after it's loaded to
update the commands? or that the controller thinks an update isn't needed
because it's the same window/document as before?

Ok, enough with wild guesses, I have found at least a wallpaper for the
problem. If you can test this on Windows would be great and see that bug
249136 does not come back with it (I could not reproduce that on Linux).

Comment 12

[Mats Palmgren (inactive)]

Attachment: wip1

This works for me in a SeaMonkey debug build on Linux.
It delays the command update until we have got the first call to Focus().

Comment 13

[Mats Palmgren (inactive)]

Comment on attachment 194067 [details] [diff] [review]
wip1

He, this actually blocked us from reaching the command update,
since all calls to Focus() are with suppression on...

Comment 14

[Mats Palmgren (inactive)]

Attachment: Patch rev. 1

This bug is a regression from removing the "&& mCurrentElement" bit in
bug 249136.

This patch tries to keep with the spirit of the original code which was to
only allow changes to |FocusedElement| to cause UpdateCommands() here, but
still fix the case we were after in bug 249136, that is |FocusedElement|
becoming NULL while being suppressed. As far as I can tell, doing
UpdateCommand() for |FocusedWindow| changes is far too early to do here.

I have verified that it fixes the bug on SeaMonkey Linux and Windows XP.
I also checked Firefox both on Linux and Windows and didn't see any
obvious regressions.

I am not sure if it regresses bug 249136 though. I don't think
I really understood how to reproduce that bug (without triggering
bug 305939 that is).  Aaron, could you test that bit please?

Comment 15

[Aaron Leventhal]

I can't get mail composer window to open at all in trunk.  I get this error:
Is Seamonkey mail broken on the trunk?

[Exception... "Cannot find interface information for parameter arg 4
[nsIMsgComposeService.OpenComposeWindow]"  nsresult: "0x80570006
(NS_ERROR_XPC_CANT_GET_PARAM_IFACE
NFO)"  location: "JS frame :: chrome://messenger/content/mailCommands.js ::
ComposeMessage :: line 274"  data: no]
WARNING: Declared InterfaceInfo not found, file
c:/moz/sea/mozilla/xpcom/reflect/xptinfo/src/xptiInterfaceInfo.cpp, line 434
JavaScript error: , line 0: uncaught exception: [Exception... "Cannot find
interface information for parameter arg 4
[nsIMsgComposeService.OpenComposeWindow]"  nsresult
"0x80570006 (NS_ERROR_XPC_CANT_GET_PARAM_IFACE_INFO)"  location: "JS frame ::
chrome://messenger/content/mailCommands.js :: ComposeMessage :: line 274"  data: no]

Comment 16

[David :Bienvenu]

Aaron, have you tried make clean in mailnews and then rebuild? That looks like
an error that occurs because we don't handle dependencies in idl files.

Comment 17

[Scott MacGregor]

that's a build dependency error as David pointed out. Make clean will fix that.

Comment 18

[Scott MacGregor]

this is actually a 1.8b4 blocker not a 1.8b5 blocker. adjusting the flags
accordingly per the triage meeting this morning.

Comment 19

[Aaron Leventhal]

I can verify that this patch fixes the bug and that bug 249136 is still fixed.
Now I want to look more closely at why it is necessary.

Comment 20

[Aaron Leventhal]

Just some thoughts in case anyone wants to throw in.

For ordinary browsing, it makes sense to update commands as soon as something is
painted and focus suppression is off, so that the user can start scrolling with
the keyboard.

For editing, it makes sense to update again when the entire document is loaded,
because that's when we finally know we're an editor. The obs_documentCreated
observer topic is fired at that point.

A couple possible solutions:
1. We could manually update commands again once we know we're an editor, but
then what other similar cases might we be missing?
2. We could update commands once when painting is unsuppressed, and then a
second time when the document is fully loaded in case we've learned something
new. I'm not sure how much 2 command updates would impact performance, but I
think it would be a safe fix.

Comment 21

[Aaron Leventhal]

> 1. We could manually update commands again once we know we're an editor, but
> then what other similar cases might we be missing?
> 2. We could update commands once when painting is unsuppressed, and then a
> second time when the document is fully loaded in case we've learned something
> new. I'm not sure how much 2 command updates would impact performance, but I
> think it would be a safe fix.
Really #1 isn't a broad enough fix.
And for some reason doing an UpdateCommands after the doc has loaded isn't
fixing us, so #2 is out for the moment. Next I have to really study why Mats'
patch is causing the UpdateCommands() to occur at the right time.

Comment 22

[Aaron Leventhal]

Comment on attachment 194131 [details] [diff] [review]
Patch rev. 1

One thing that makes me nervous about this patch is that the first time the
compose window is opened, UpdateCommands() does not happen. The subsequent
times it does.

Comment 23

[Aaron Leventhal]

Attachment: Low risk patch -- updateComposeItems() for obs_documentCreated

What changed with bug 249136 is that we do a command update as soon as the
first painting occurs. However, the document doesn't know that it's an editor
yet. This wasn't a problem for Seamonkey composer because it updates its
commands on obs_documentCreated. I believe mail should do the same thing.

On request I do have an alternative patch which mostly touches
nsFocusController so that a second command update is given when a document is
completely loaded. However, this still requires a change in mailnews because it
won't update commands based on focus for the same window twice in a row -- see
CommandUpdate_MsgCompose().

Comment 24

[[e-mail]]

Comment on attachment 194509 [details] [diff] [review]
Low risk patch -- updateComposeItems() for obs_documentCreated

Doesn't work for me, after applying the patch and pressing ^M the menuitems
remain greyed out until I focus another window.

Comment 25

[Aaron Leventhal]

Comment on attachment 194509 [details] [diff] [review]
Low risk patch -- updateComposeItems() for obs_documentCreated

You're supposed to use Ctrl+R so that focus starts out in the content area.

The items are supposed to be grayed out at first when you hit Ctrl+M, because
you're still typing in someone's address :)

Comment 26

[Aaron Leventhal]

Comment on attachment 194509 [details] [diff] [review]
Low risk patch -- updateComposeItems() for obs_documentCreated

You're supposed to use Ctrl+R so that focus starts out in the content area.

The items are supposed to be grayed out at first when you hit Ctrl+M, because
you're still typing in someone's address :)

Comment 27

[[e-mail]]

(In reply to comment #25)
>The items are supposed to be grayed out at first when you hit Ctrl+M, because
>you're still typing in someone's address :)
Sure, but then they're supposed to ungray out when I click in the compose area
:-P

Comment 28

[[e-mail]]

They only ungray out when the message body loses focus to some other window.

Comment 29

[Aaron Leventhal]

Comment on attachment 194509 [details] [diff] [review]
Low risk patch -- updateComposeItems() for obs_documentCreated

Good catch.

Comment 30

[Aaron Leventhal]

Attachment: After editor doc created, allow the next CommandUpdate_MsgCompose() to update commands for the same window. The commands are then updated the next time this document is focused.

Alternatively we could patch CommandUpdate_MsgCompose to remove the same window
check, but I guess that's there to help with performance.

Comment 31

[Aaron Leventhal]

Neil asked me to add a comment. It will read:

      // Now that we know this document is an editor, ensure
      // that CommandUpdate_MsgCompose() will update commands
      // next time the editable document receives focus
      gLastWindowToHaveFocus = null;

Comment 32

[Asa Dotzler [:asa]]

Comment on attachment 194537 [details] [diff] [review]
After editor doc created, allow the next CommandUpdate_MsgCompose() to update commands for the same window. The commands are then updated the next time this document is focused.

aaron, can you land this on the trunk ASAP so we can get it verified

Comment 33

[Bob Clary [:bc] (inactive)]

I'll verify with tomorrow's build.

Comment 34

[Mats Palmgren (inactive)]

I still see this in a debug build of SeaMonkey on Windows XP.

STEPS TO REPRODUCE
1. start "seamonkey -mail" and click on a mail in the thread pane
2. click Reply in the toolbar (Compose window comes up, with quoted message
   and focus is in the message area (caret blinking)).
3. click on the Edit menu: note that "Rewrap" is greyed out
4. click on the Subject text field: "HTML toolbar" below is greyed out (ok)
5. click in the message area: most of the toolbar is enabled again, but not
   the heading and font controls (the two on the left).
6. click on Edit menu again: note that "Rewrap" is still greyed out
7. click on another window: note that the full HTML toolbar got enabled!
8. click in the message area again and then Edit menu: "Rewrap" is enabled
9. click in Subject and then in message area: "Rewrap" is disabled

I am sorry but this bug is not fixed, it's actually worse since earlier it
only occured when the Reply window was opened for the first time.
I tested both with and without the patch for bug 305032 (attachment 194668 [details] [diff] [review])
with the same results.

Comment 35

[Ruediger Lahl]

(In reply to comment #34)

> I am sorry but this bug is not fixed,

I can confirm that. The patch has no effect in
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20050903
SeaMonkey/1.1a (Build 00)

Comment 36

[Scott MacGregor]

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

Comment 37

[Bob Clary [:bc] (inactive)]

Not fixed in thunderbird 1.5 20050903 on winxp.

Comment 38

[Scott MacGregor]

(In reply to comment #37)
> Not fixed in thunderbird 1.5 20050903 on winxp.

Bob this change hasn't been approved for the branch yet, so we wouldn't expect
any changes in the thunderbird 1.5 branch build. You'd only see it in 1.6a1 but
it sounds like it may not be fixed there anyway.

Comment 39

[Bob Clary [:bc] (inactive)]

(In reply to comment #38)

doh! I knew that. /me needs more coffee.

Comment 40

[Mats Palmgren (inactive)]

Attachment: Combined patch, rev. 1

This patch fixes this bug, bug 305032 and bug 306924.
I think we need to do something like this to address them all at once.

This patch is a combination of "Patch rev. 1" in this bug + my suggested change

for bug 305032 + reverting one of the ESM changes for bug 258285 + backing out
the last patch for this bug.

Everything seems to work with this, but I'm not sure if it regresses
bug 249136 or not. I think we should take this patch either way because
the current problems are much worse than that bug.

Regarding the nsFocusController changes: as I said before I think the
intent of the original code was to only propagate element changes
when becoming unsuppressed in SetSuppressFocus().

nsWebShellWindow.cpp: I think it makes sense to unconditionally suppress
focus here, until we reach NS_ACTIVATE in ESM. (I wonder if this could be
the cause of the race condition Kin talks about in bug 103197?)

nsEventStateManager.cpp: if we do the unsuppress loop before the call
to SetFocusedElement() then we could cause two UpdateCommands() in the
case we are suppressed when running this code, the first one could be with
an obsolete element. If we do the SetFocusedElement() first, then we will
only cause UpdateCommands() with the correct (ESM) element.

Comment 41

[Boris Zbarsky [:bzbarsky]]

What about cases when there is no nsWebShellWindow around (eg embedding apps)? 
Or does this situation never arise there?

Comment 42

[Mats Palmgren (inactive)]

Attachment: Combined patch, rev. 2

This also fixes regression bug 307153. The difference to the last patch is
that I have resurrected the "Window Switch" focus suppressions that we removed
in bug 258285, BUT I added a matching unsuppress call for them.

To sum up, bug 258285 did four things:
1. it swapped the order between SetFocusedElement() and the unsuppress loop
   in the NS_ACTIVATE code in ESM.
2. it added EnsureFocusSynchronization() calls
3. it removed the "Window Switch" focus suppressions
4. it avoided suppressing focus in nsWebShellWindow.cpp if the focused
   window was the same as the already focues one.

Bug 306235 backed out 4.
This patch backs out 1 and 3, but fixes the problem with 3 by adding
matching unsuppress calls. And 2 was only needed for the "tabbing from
combobox" case and should not disturb other cases AFAICT.

As far as I can see, this patch fixes all the regressions while still
not regressing the cases bug 258285 was fixed for... I hope.

Aaron, Bryner do you have any comments or suggestions?
(see also comment 40)

Comment 43

[Aaron Leventhal]

Comment on attachment 195006 [details] [diff] [review]
Combined patch, rev. 2

These changes are too big.

I prefer the webshell window fix from bug 305032, because it is smaller and
most like what we had orignally. You're going into new territory with that.
Also, I liked having it separate in bug 305032 so that if there is a problem
with it we can tell which focus change caused the problem.

I also don't like to focus controller changes because the only reason they work
is luck. If anything, we should just go to pre-249136 focus controller changes
as you suggested.

Comment 44

[Aaron Leventhal]

(In reply to comment #41)
> What about cases when there is no nsWebShellWindow around (eg embedding apps)? 
> Or does this situation never arise there?

The bug was caused by webshellwindow changes.

Comment 45

[Aaron Leventhal]

Neil had also tested my original fix. If we're going to patch nsFocusController
to solve this I have a less risky patch which I understand much better.

Comment 46

[Aaron Leventhal]

Attachment: Safe and sure

Comment 47

[[e-mail]]

Comment on attachment 195011 [details] [diff] [review]
Safe and sure

I don't understand why you are removing the focused window change check, it
will cause a pref hit when tabbing through the addressing area. Also, as I'm on
holiday for the next week I'm not in a position to review this anyway.

Comment 48

[Boris Zbarsky [:bzbarsky]]

Doesn't the "safe and sure" patch not address any other editors that may have
been broken by the original core changes?

Comment 49

[Boris Zbarsky [:bzbarsky]]

> The bug was caused by webshellwindow changes.

Er?  I thought it was caused by focus controller changes.  Certainly the patches
in bug 249136 only touch focus controller that I can see.

Comment 50

[Aaron Leventhal]

Attachment: Make sure commands get updated when switching to window and doc is not busy

Boris might be right about other editors. Here is a more general fix.

Boris, sorry for my confusing comment about the nsWebShellWindow stuff. You're
right, that's another bug.

Comment 51

[Aaron Leventhal]

(In reply to comment #47)
> (From update of attachment 195011 [details] [diff] [review] [edit])
> I don't understand why you are removing the focused window change check, it
> will cause a pref hit when tabbing through the addressing area.
We need the focused window change check because in order to fix bug 249136 we
need to update commands as soon as we set focus to a doc, which may be before it
is fully loaded. We also need to update commands once we know if it's an edit
document, which we only know after it's fully loaded. The mail compose tries to
optimize command updates by not allowing 2 in a row on the same window. I don't
think that perf improvement for tabbing is necessary -- I don't ever see any
perf issues with tab navigation. If it happens in 1/100 of a second or 1/50 no
one notices the difference.

(In reply to comment #48)
> Doesn't the "safe and sure" patch not address any other editors that may have
> been broken by the original core changes?
I doubt most editors have implemented this tab navigation perf thing. If they
have they'll need to remove it because the fix for bug 249136 requires that we
update commands both as soon as a doc gets focus and after it loads and we now
know that it's editable.

Comment 52

[Aaron Leventhal]

If we care about the tab nav optimization in mail compose the other option was
to set:
gLastWindowToHaveFocus = null in obs_documentCreated handling.
That's what Neil and I had tested and I checked in.
I think that didn't work because the timing in the release builds was different
then on my test machine.

To fix that, I think we should updateComposeItems() if the doc that's being
created has focus, and we need some changes to nsFocusController() to make sure
the 2nd command update happens (the doc load one).

Comment 53

[Aaron Leventhal]

I have posted a new patch in bug 305032 that deals with this. It does require a
mail/mailnews change to remove the inability for command updates to happen a 2nd
time once obs_documentCreated has occured.

Comment 54

[Aaron Leventhal]

Fixed by checkin to bug 305032.

Comment 55

[Ruediger Lahl]

(In reply to comment #54)
> Fixed by checkin to bug 305032.

Thanks Aaron, that works for me!

Comment 56

[Stephen Donner [:stephend] Not actively reading bugmail]

Verified FIXED using SeaMonkey 1.1a--Mozilla/5.0 (Windows; U; Windows NT 5.1;
en-US; rv:1.9a1) Gecko/20050915 Mozilla/1.0

Comment 57

[Ruediger Lahl]

(In reply to comment #56)
> Verified FIXED using SeaMonkey 1.1a--Mozilla/5.0 (Windows; U; Windows NT 5.1;
> en-US; rv:1.9a1) Gecko/20050915 Mozilla/1.0

It looks like this bug is still alive in the actual Thunderbird 1.5beta 1
(20050908). Anybody their, who can confirm that?