296265 - Caret disappears in cached compose window body

Status: VERIFIED FIXED

(Core :: Spelling checker, defect)

Description

[Frank Wein [:mcsmurf]]

To reproduce:
1. Open MailNews Composer
2. Paste some text into subject
3. Press tab key
4. Observe that caret is invisible

Comment 1

[Frank Wein [:mcsmurf]]

This regressed between 2005-05-31-06 and 2005-06-01-05. 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-05-31+05%3A00%3A00&maxdate=2005-06-01+06%3A00%3A00&cvsroot=%2Fcvsroot

Comment 2

[Sander]

I am seeing this same bug with 2005052706, seamonkey win2k. Are you sure the
regression window is correct?

Comment 3

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

Is this related?

Error: Unknown namespace prefix 'html'.  Ruleset ignored due to bad selector.
Source File: chrome://messenger/skin/messageBody.css
Line: 54

Comment 4

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

(In reply to comment #3)
> Is this related?
> 
> Error: Unknown namespace prefix 'html'.  Ruleset ignored due to bad selector.
> Source File: chrome://messenger/skin/messageBody.css
> Line: 54

Yeah, ignore this comment.

Instead, I've found a particular case that fails between 2005-05-19-05 and
2005-05-20-05:

1. Launch Mail.
2. Select a folder.
3. Select a message in the folder.
4. Click Compose.
5. Click to focus the compose message body.

Notice there's no caret at this point.

I could be way off base, but please do humor me:

Boris, could this be from bug 293914?

In particular, the following changes to nsEventStateManager.cpp:

-  if (aContent == mActiveContent) {
-    mActiveContent = nsnull;
+  if (mActiveContent &&
+      nsContentUtils::ContentIsDescendantOf(mActiveContent, aContent)) {
+    // Active is hierarchical, so set the current active to the
+    // content's parent node.
+    mActiveContent = aContent->GetParent();

I apologize in advance if I'm way off base here.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Those are very unlikely to have caused this.

Further, I can't reproduce this with either the steps in comment 0 or the steps
in comment 4, with either Seamonkey mailnews or tbird.  Both Linux debug builds
pulled at MOZ_CO_DATE="Thu Jun 2 00:44:32 CDT 2005".

Martijn, or someone else who builds on Windows, can you reproduce this?  If so,
could you possibly hunt down which checkin is the problem?

Comment 6

[Boris Zbarsky [:bzbarsky]]

OK, so to reproduce this you have to set the recycled compose window pref to 1
(the Windows default).  Then the second and later compose windows show the problem.

The regression range is in fact 2005-05-19-05 to 2005-05-20-05.   Not sure what
in there is breaking this yet.

Comment 7

[Boris Zbarsky [:bzbarsky]]

OK, looks like this is a regression from bug 294251.  If I comment out the
|StopInlineSpellChecker();| call that added to MsgComposeCommands.js this bug
goes away.

No idea what's going on here, really, but I do get some nice content iterator
asserts when the spellchecker restarts when we reshow the recycled compose
window.  Maybe those are relevant.

Comment 8

[[e-mail]]

(In reply to comment #7)
>OK, looks like this is a regression from bug 294251.  If I comment out the
>|StopInlineSpellChecker();| call that added to MsgComposeCommands.js this bug
>goes away.
Phew, so all we did was expose an underlying bug, that is to say, if the spell
checker is stopped either manually or (on the suite) automatically then when the
window is recycled then the cursor does not appear.

>No idea what's going on here, really, but I do get some nice content iterator
>asserts when the spellchecker restarts when we reshow the recycled compose
>window.  Maybe those are relevant.
Interestingly I run my linux build with the recycled compose window pref and it
does not assert or have any caret weirdness. It does trigger a couple of
NS_ENSURE_SUCCESS warnings at nsFilteredContentIterator.cpp:110 and
mozInlineSpellChecker.cpp:902 though. Here's a stack trace of an assert:

nsDebug::Assertion(const char * 0x01bdc7ec `string', const char * 0x01bdc7fc
`string', const char * 0x01bdc738 `string', int 965) line 109
nsContentIterator::First(nsContentIterator * const 0x00fbafd8) line 965 + 35 bytes
nsFilteredContentIterator::First(nsFilteredContentIterator * const 0x00fbafd8)
line 158
nsTextServicesDocument::FirstTextNode(nsIContentIterator * 0x03067f20,
nsTextServicesDocument::TSDIteratorStatus * 0x03067e64) line 4123
nsTextServicesDocument::FirstBlock(nsTextServicesDocument * const 0x03067e50)
line 608 + 10 bytes
nsEditorSpellCheck::InitSpellChecker(nsEditorSpellCheck * const 0x03067d90,
nsIEditor * 0x03b63530, int 0) line 93
mozInlineSpellChecker::SetEnableRealTimeSpell(mozInlineSpellChecker * const
0x03c56800, int 50757008) line 174
XPTC_InvokeByIndex(nsISupports * 0x03c56800, unsigned int 7, unsigned int 1,
nsXPTCVariant * 0x0012d684) line 102
... the rest is the usual stack from opening a window and running JS stuff.

Comment 9

[[e-mail]]

I've done a bit of debugging and I can see that when a new compose window is
created the body element has at least one child that the content iterator range
is selecting but when it is recycled the range is collapsed.

Comment 10

[[e-mail]]

OK, so it turns out that a new editor body contains a <br> ...

Comment 11

[Boris Zbarsky [:bzbarsky]]

> Interestingly I run my linux build with the recycled compose window pref

I never managed to reproduce this bug in a debug build.  I tested backing out
bug 294251 by editing my chrome in a nightly...

I bet the reason we're not blinking is that nothing is on the line when we
recycle.  This is a known caret issue; that's why editor sticks <br> all over.

Comment 12

[[e-mail]]

OK, I have some minimal steps to reproduce that also work on Thunderbird.
1. Start Suite/TB
2. Turn off spell as you type in preferences
3. Write a new message
4. Using the options menu, turn spell as you type on and off again
5. Close the window
6. Write a new message
The recycled body will not have a caret.

Comment 13

[[e-mail]]

(In reply to comment #11)
>>Interestingly I run my linux build with the recycled compose window pref
>I never managed to reproduce this bug in a debug build.  I tested backing out
>bug 294251 by editing my chrome in a nightly...
Oh, it shows up fine in my MSVC debug build.

>I bet the reason we're not blinking is that nothing is on the line when we
>recycle.  This is a known caret issue; that's why editor sticks <br> all over.
The document body appears to contain a <br> either way; what I can't figure is
why it doesn't work the second time.

Comment 14

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

Changing summary FROM: Caret disappears after tabbing to Composer Body

TO: Caret disappears in cached compose window body

Comment 15

[[e-mail]]

OK, I can now reproduce this without step 4...

Comment 16

[[e-mail]]

... and I can reproduce in 2005051905 ...

Comment 17

[[e-mail]]

In fact my regression range is 2005-05-12-06 to 2005-05-13-06.
But nothing in that range strike me as being plausible :-(

Comment 18

[[e-mail]]

I double-checked and my regression range is actually 2005-05-13-06 to
2005-05-14-06 - just when Spell As You Type originally landed...

Comment 19

[[e-mail]]

(In reply to comment #11)
>I bet the reason we're not blinking is that nothing is on the line when we
>recycle.  This is a known caret issue; that's why editor sticks <br> all over.
You're absolutely right as usual.
When the inline spell checker is disabled, it returns error codes from its post
edit hook. This prevents various post-edit actions from occuring. In particular
the very next call after the error check is to CreateBogusNodeIfNeeded...

Comment 20

[[e-mail]]

Attachment: Proposed patch

I'm surprised that turning off inline spellcheck doesn't make more stuff fail.

Comment 21

[Frank Wein [:mcsmurf]]

Comment on attachment 185414 [details] [diff] [review]
Proposed patch

regression

Comment 22

[[e-mail]]

Fix checked in.

Comment 23

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

I've been using daily builds and haven't seen this crop up since the fix landed.

Verified FIXED using build 2005-06-11-05 on Windows XP Seamonkey trunk.  (I
double-checked the steps in comment 0, comment 4, and comment 12.)