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

RESOLVED FIXED in mozilla1.9.2a1

Status

()

--
blocker
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: philor, Assigned: bzbarsky)

Tracking

({fixed1.9.0.7, fixed1.9.1, regression})

Trunk
mozilla1.9.2a1
fixed1.9.0.7, fixed1.9.1, regression
Points:
---
Bug Flags:
blocking1.9.1 +
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

10 years ago
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+

Comment 1

10 years ago
SeaMonkey is broken too; neither Editor nor Message Compose work.
Flags: blocking1.9.1?

Updated

10 years ago
Keywords: regression

Updated

10 years ago
Flags: blocking-seamonkey2.0a3+

Updated

10 years ago
Duplicate of this bug: 474416

Comment 3

10 years ago
Send button does nothing - at least in Seamonkey.
(Reporter)

Updated

10 years ago
Duplicate of this bug: 474428

Comment 5

10 years ago
TB trunk stopped working today.

Comment 6

10 years ago
Confirm: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1b3pre) Gecko/20090120 Shredder/3.0b2pre

Comment 7

10 years ago
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
(Assignee)

Comment 8

10 years ago
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...
(Assignee)

Comment 9

10 years ago
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.
(Assignee)

Comment 10

10 years ago
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.
(Reporter)

Updated

10 years ago
Duplicate of this bug: 474448
(Assignee)

Comment 12

10 years ago
Created attachment 357822 [details] [diff] [review]
Proposed fix

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)
(Assignee)

Updated

10 years ago
Component: Composition → Editor
Flags: blocking-thunderbird3+
Flags: blocking-seamonkey2.0a3+
Product: MailNews Core → Core
QA Contact: composition → editor
Target Milestone: Thunderbird 3.0b2 → ---
(Assignee)

Updated

10 years ago
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+

Comment 14

10 years ago
(In reply to comment #12)

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

WFM.

Updated

10 years ago
Duplicate of this bug: 474522
(Assignee)

Comment 16

10 years ago
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.
Duplicate of this bug: 474580
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+

Comment 20

10 years ago
Dropped back to 2.0a2, which works fine, so it's a3pre.
(Assignee)

Comment 21

10 years ago
FirePageHideNotification fires before EndDocumentLoad does.
Attachment #357822 - Flags: superreview?(peterv)
Attachment #357822 - Flags: superreview+
Attachment #357822 - Flags: review?(peterv)
Attachment #357822 - Flags: review+
(Assignee)

Comment 22

10 years ago
Checked in http://hg.mozilla.org/mozilla-central/rev/0ea4c52d4a8c

Still need help on those tests...
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.2a1
(Assignee)

Comment 24

10 years ago
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+
(Assignee)

Comment 26

10 years ago
Fixed on 1.9.0 branch.
Keywords: fixed1.9.0.7

Updated

10 years ago
Duplicate of this bug: 474542

Comment 28

10 years ago
(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?

Comment 29

10 years ago
(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.
(Assignee)

Comment 30

10 years ago
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.

Comment 31

10 years ago
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.
(Assignee)

Comment 32

10 years ago
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).
(Assignee)

Comment 34

10 years ago
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?
(Assignee)

Comment 36

10 years ago
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.