Closed Bug 488094 Opened 11 years ago Closed 11 years ago
IHTMLEditor refuses to set paragraph mode, freezes
With Thunderbird 3b1 (possibly alpha versions as well), I'm unable to do my editor.setParagraphFormat("p"); (which my extension, BiDi Mail UI does). I get: Error: uncaught exception: [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIHTMLEditor.setParagraphFormat]" nsresult: "0x80004005 (NS_ERROR_FAILURE)" location: "JS frame :: chrome://bidimailpack/content/bidimailpack-composer.js :: InsertParagraph :: line 1084" data: no] why is that? To reproduce: Install BiDi Mail UI 0.9.3 (pre-release) from here: https://www.mozdev.org/bugs/attachment.cgi?id=5791 compose a new HTML message. Switch to paragraph mode (why can't I make the default be paragraph mode anyway?). Type some text. Press Enter. That should do it; otherwise type some more text and press Enter twice.
nsIHTMLEditor is actually a core component. Therefore moving to the relevant bugzilla component.
Component: Message Compose Window → Editor
Product: Thunderbird → Core
QA Contact: message-compose → editor
Eyal, presumably your extension works on TB 2.x? Which platform(s) do you see this on? I'm not sure if this should block or not, but seems to warrant some investigation as to a possible regression or not.
The Paragraph menuitem (Format - Paragraph - Paragraph) calls doStatefulCommand('cmd_paragraphState', 'p') which calls nsParagraphStateCommand::SetState('p') which calls SetParagraphFormat('p'), so any breakage should be reproducible without your extension... always assuming your extension doesn't do anything weird with the editor.
This does indeed only happen with my extension installed. Here's what it does (when I pref this code off, bug does not manifest): http://pastebin.mozilla.org/643507 This is a transition to a new paragraph when the user presses enter. Still, I shouldn't be getting a failure setting the paragraph state.
It's definitely an editor bug; I can now reproduce the exception quite easily, and it seems to be caused by trying to delete the same <br> node twice. As WillMakeBasicBlock hasn't changed in years I don't know why its behaviour might have changed, so I'm hoping Graeme might have an idea ;-)
Do you have a regression range? Are there steps to reproduce that can work in a Gecko-based browser (and which could, e.g., be put in a mochitest)?
(In reply to comment #6) > Are there steps to reproduce that can work in a Gecko-based browser (and which > could, e.g., be put in a mochitest)? These steps work for me in SeaMonkey Composer; presumably there's a way to duplicate them in Midas but I don't know how to program Midas. 1. Open a new Composer window 2. Hit Enter 3. Format - Paragraph - Paragraph Although the Composer code eats the exception, Venkman can log it and you can also view the source to demonstrate that the paragraph was not created. > Do you have a regression range? A fairly old SeaMonkey (1.9a5pre/20070619) does not have the bug.
First regressing nightly is 2007-06-29. Weird.
Um, I was checking Seamonkey nightlies, of course.
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=SeaMonkeyAll&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2007-06-28&maxdate=2007-06-29+04%3A00%3A00&cvsroot=%2Fcvsroot Would have been easy to blame bug 386009 but the times doesn't seem to match for when it was in, out, and in again.
Given that: (1) this seems to have regressed prior to 1.9 (2) we don't yet have an accurate regression range it's pretty hard to call this a 1.9.1 blocker, especially at this late stage, so marking wanted.
dbaron: How about 1.9.2 then?
>It's definitely an editor bug; I can now reproduce the exception quite easily, >and it seems to be caused by trying to delete the same <br> node twice. As >WillMakeBasicBlock hasn't changed in years I don't know why its behaviour might >have changed, so I'm hoping Graeme might have an idea ;-) You're grossly overestimating my knowledge of editor code! But did some digging anyway. Yeah, the problem does seem to be that the placeholder <br> at the start of the line where the block is being gets inserted has two attempts to delete it. The real action seems to be in nsHTMLEditRules::WillMakeBasicBlock. First, we seem to be hitting the code after // consume a br, if needed which deletes the placeholder br at the start of the line. All well and good. Next the code wants tries to delete the nodes in arrayOfNodes. In this case, the only node in that array is the placeholder br we've already deleted, so we fail. We fail because bug 237964 (landed 26th June 07) introduced a check into nsHTMLEditor::DeleteNode that returns NS_ERROR_FAILURE if the node we're trying to delete isn't modifiable. (The isModifiable check ultimately leads to a call of nsINode::IsEditable() - which returns true the first time we try to delete the br - it's contained in an editable doc - and false the second, because it's not contained in any doc). Before bug 237964, no such check took place, and the delete transaction would proceed (...and work out that the node has no parent, and thus do nothing). So, technically a regression from bug 237964, but only in as much as the sensible code landed in that bug exposed the silliness already in the codebase. The hack fix would be to compare the node we're trying to delete to the br node we've already deleted. Not sure what the _correct_ fix is...
Flags: wanted1.9.1? → wanted1.9.1+
Comment on attachment 377879 [details] [diff] [review] Safe fix We need to fix this regression on the 1.9.1 branch at some point. Requesting approval now since we can't even bake it on trunk first otherwise.
Where's the bug about not using dummy br nodes in the first place? :-(
(no longer need approval to land on trunk)
Pushed changeset 7fc7e27ed155 to mozilla-central.
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Attachment #377879 - Flags: approval1.9.1? → approval1.9.1-
Comment on attachment 377879 [details] [diff] [review] Safe fix We're closed for 1.9.1 patches, but can re-evaluate this when it comes time to look at 220.127.116.11
Not blocking 18.104.22.168, but I'll approve the patch.
Attachment #377879 - Flags: approval22.214.171.124+
Comment on attachment 377879 [details] [diff] [review] Safe fix Approved for 126.96.36.199. a=ss Please land as soon as you can if you intend to get this non-blocker into 188.8.131.52.
Pushed changeset a5b258c15577 to releases/mozilla-1.9.1
Eyal/Peter: can you verify that this is fixed in latest-mozilla1.9.1 nightly or better yet the 3.5.1 release candidate: ftp://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/3.5.1-candidates/build1/
Verified on Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:184.108.40.206pre) Gecko/20090716 Shredder/3.0b4pre First I checked in Tb3(beta2) and tried the steps to reproduce the problem. The exception in the console was the same. Then I tried in the nightly, and there was no error in the console.
You need to log in before you can comment on or make changes to this bug.