Closed Bug 474389 Opened 15 years ago Closed 15 years ago

[FIX]Can't focus or type in editor in mail compose window

Categories

(Core :: DOM: Editor, defect)

defect
Not set
blocker

Tracking

()

RESOLVED FIXED
mozilla1.9.2a1

People

(Reporter: philor, Assigned: bzbarsky)

References

Details

(Keywords: fixed1.9.0.7, fixed1.9.1, regression)

Attachments

(1 file)

hg bisect says that this started with the landing for bug 459443 - the two visible symptoms are that the address widget is just one line long, instead of the usual four, and when you try to move to the body of the email by clicking, nothing at all happens, or if you tab from the subject you get a focus ring, but no caret and no ability to type anything.

The console is surprisingly quiet about it: if you have the default mail.compose.max_recycled_windows set to 1, there's an assertion from http://mxr.mozilla.org/comm-central/source/mailnews/compose/src/nsMsgCompose.cpp#1336 when you finally give up and close the window, since there's no editor to recycle, but that's about it.
Flags: blocking-thunderbird3+
SeaMonkey is broken too; neither Editor nor Message Compose work.
Flags: blocking1.9.1?
Keywords: regression
Flags: blocking-seamonkey2.0a3+
Send button does nothing - at least in Seamonkey.
TB trunk stopped working today.
Confirm: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1b3pre) Gecko/20090120 Shredder/3.0b2pre
Detail from dupe'd bug #474428 (sorry if this is cruft at this point):

Bad behavior: when replying not only can you not enter text into the body
area, but the existing body text doesn't come over.
Expected: text from the original body should come over and any rules about how
it should be indented or organized should be followed.

Bad behavior: When replying the compose window isn't bringing over the person's name/address that I'm replying to over (i.e. "To:" line is blank). 
Expected: The compose window should bring over (on regular reply) the original
sender's name/address. On "Reply All" it should bring over all recipients of
the conversation.

Bad behavior: When replying the compose window's title just says "Write: (no subject)" even though the Subject field is filled in with "Re:" and the email's original subject.
Expected: The compose window's title should reflect that I'm composing a reply
to the original mail. Ex: "Re: That one thing...".

UA: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1b3pre)
Gecko/20090120 Lightning/1.0pre Shredder/3.0b2pre
OK, looking at the Seamonkey MsgComposeCommands.js the problem is this:

We set up the editor in ComposeStartup() like so (line 1305):

  editorElement.makeEditable(editortype, true);

Then later on in the same method (line 1383) we have:

          // Load empty page to create the editor
          editorElement.webNavigation.loadURI("about:blank", // uri string
                               0,                            // load flags
                               null,                         // referrer
                               null,                         // post-data stream
                               null);

And the load naturally drops the editor data...
OK, so more precisely the sequence is:

1)  Set up the editor data
2)  nsEditingSession::StartDocumentLoad clears it
3)  Pagehide also clears it
4)  editor.js has an anonymous method that gets an obs_documentLocationChanged
    notification and tries to get the current editor, which sets up a _new_ 
    editor data.

nsEditingSession::EndDocumentLoad is NOT called in this case.
Ok, now EndDocumentLoad is in fact being called.  But GetEditable() on the docshell returns false, so we don't do anything to make it editable again.
Attached patch Proposed fixSplinter Review
This patch basically makes the "enable editor after the load happens" thing work again.  It was broken, in general, but happened to work by accident for loads starting with a null mOSHE...

I'd love some help on writing tests for this.  I can get some together eventually, I guess, but I don't really know the editor API so it'll take me some time.  If someone who does wants to help, that would rock.  Basically we want to test both the "after load" and the "before load" mode, with a null and non-null mOSHE.
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Attachment #357822 - Flags: superreview?(peterv)
Attachment #357822 - Flags: review?(peterv)
Component: Composition → Editor
Flags: blocking-thunderbird3+
Flags: blocking-seamonkey2.0a3+
Product: MailNews Core → Core
QA Contact: composition → editor
Target Milestone: Thunderbird 3.0b2 → ---
Summary: Can't focus or type in editor in mail compose window → [FIX]Can't focus or type in editor in mail compose window
blocking+ per bz.
Flags: blocking1.9.1? → blocking1.9.1+
(In reply to comment #12)

> Created an attachment (id=357822) [details]
> Proposed fix

WFM.
Note that I backed out bug 459443 for now to fix this for tomorrow's nightlies, but leaving this open so I can get those reviews and land both patches together.
Comment on attachment 357822 [details] [diff] [review]
Proposed fix

The sequence seems to be nsEditingSession::MakeWindowEditable will end up setting mMakeEditable to true and register as a progress listener. nsEditingSession::StartDocumentLoad will try to nsDocShell::DetachEditorFromWindow, that'll return early. nsEditingSession::EndDocumentLoad will end up calling nsDocShellEditorData::SetEditor which will set mMakeEditable to false.
Now what happens next? nsDocShell::FirePageHideNotification will call nsDocShell::DetachEditorFromWindow which will now just detach the editor? What code ensures we reinitialize the editor then?
Attachment #357822 - Flags: superreview?(peterv)
Attachment #357822 - Flags: superreview+
Attachment #357822 - Flags: review?(peterv)
Attachment #357822 - Flags: review+
Comment on attachment 357822 [details] [diff] [review]
Proposed fix

Ugh, didn't mean to + yet.
Attachment #357822 - Flags: superreview?(peterv)
Attachment #357822 - Flags: superreview+
Attachment #357822 - Flags: review?(peterv)
Attachment #357822 - Flags: review+
Dropped back to 2.0a2, which works fine, so it's a3pre.
FirePageHideNotification fires before EndDocumentLoad does.
Attachment #357822 - Flags: superreview?(peterv)
Attachment #357822 - Flags: superreview+
Attachment #357822 - Flags: review?(peterv)
Attachment #357822 - Flags: review+
Checked in http://hg.mozilla.org/mozilla-central/rev/0ea4c52d4a8c

Still need help on those tests...
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.2a1
Comment on attachment 357822 [details] [diff] [review]
Proposed fix

Need this on branch if we're going to land bug 459443 there.
Attachment #357822 - Flags: approval1.9.0.7?
Comment on attachment 357822 [details] [diff] [review]
Proposed fix

Approved for 1.9.0.7, a=dveditz for release-drivers.
Attachment #357822 - Flags: approval1.9.0.7? → approval1.9.0.7+
Fixed on 1.9.0 branch.
Keywords: fixed1.9.0.7
(In reply to comment #22)
> Checked in http://hg.mozilla.org/mozilla-central/rev/0ea4c52d4a8c
> 
> Still need help on those tests...

Hi Boris, did you ever get a chance to get tests for these?
(In reply to comment #28)
> (In reply to comment #22)
> > Checked in http://hg.mozilla.org/mozilla-central/rev/0ea4c52d4a8c
> > 
> > Still need help on those tests...
> 
> Hi Boris, did you ever get a chance to get tests for these?

Ok, I just talked to ctalbert.  Looks like Mozmill UI automation could be a better tool for creating tests for this.  Cc'ing clint to get onto QA's testcase wanted list.
This shouldn't need mozmill.  It can be tested completely programmatically via editor APIs in a chrome mochitest, and that's the right place to test it.

And no, I haven't gotten any takers to help write the tests yet.
A chrome mochitest, if it works in Firefox, is a good solution because everything visible on Firefox tests triggers more people's attention than anything visible only on some other product.
The downside is that Thunderbird can't run mochitests, but I guess that's OK if Firefox and SeaMonkey are running the tests.
Yeah, this is core editor functionality that doesn't depend on any specific piece of mailnews code, other than the specific editor API calls mailnews makes.
Boris, in my experience, writing Mozmill tests is easier and more pleasant than writing mochitests.  I would personally recommended biasing towards them more than mochitests (though perhaps that should wait until they get hooked up to the build automation, which I believe is in progress).
mochitests, on the other hand, have better coverage across our products and as you note are in fact hooked up to automation.

In this case we probably want both: one test to test core editor API behavior and one test to test mailnews behavior.  Note that the latter could be passing even if the former fails (e.g. if mailnews changes the way it uses editor) and vice versa (e.g. if mailnews screws up something else).
Where are we with mochitest implementation for this bug?
They don't exist yet.  See comment 12, comment 22, comment 30.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: