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)
Core
DOM: Editor
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)
4.47 KB,
patch
|
peterv
:
review+
peterv
:
superreview+
dveditz
:
approval1.9.0.7+
|
Details | Diff | Splinter Review |
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•15 years ago
|
||
SeaMonkey is broken too; neither Editor nor Message Compose work.
Flags: blocking1.9.1?
Updated•15 years ago
|
Keywords: regression
Updated•15 years ago
|
Flags: blocking-seamonkey2.0a3+
Comment 3•15 years ago
|
||
Send button does nothing - at least in Seamonkey.
Comment 5•15 years ago
|
||
TB trunk stopped working today.
Comment 6•15 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•15 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•15 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•15 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•15 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.
Assignee | ||
Comment 12•15 years ago
|
||
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•15 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•15 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
Comment 14•15 years ago
|
||
(In reply to comment #12) > Created an attachment (id=357822) [details] > Proposed fix WFM.
Assignee | ||
Comment 16•15 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.
Comment 18•15 years ago
|
||
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 19•15 years ago
|
||
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•15 years ago
|
||
Dropped back to 2.0a2, which works fine, so it's a3pre.
Assignee | ||
Comment 21•15 years ago
|
||
FirePageHideNotification fires before EndDocumentLoad does.
Updated•15 years ago
|
Attachment #357822 -
Flags: superreview?(peterv)
Attachment #357822 -
Flags: superreview+
Attachment #357822 -
Flags: review?(peterv)
Attachment #357822 -
Flags: review+
Assignee | ||
Comment 22•15 years ago
|
||
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
Assignee | ||
Comment 23•15 years ago
|
||
Also checked in http://hg.mozilla.org/releases/mozilla-1.9.1/rev/fbdc2e740092
Keywords: fixed1.9.1
Assignee | ||
Comment 24•15 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 25•15 years ago
|
||
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+
Comment 28•15 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•15 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•15 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•15 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•15 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.
Comment 33•15 years ago
|
||
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•15 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).
Comment 35•15 years ago
|
||
Where are we with mochitest implementation for this bug?
Assignee | ||
Comment 36•15 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.
Description
•