nsIHTMLEditor refuses to set paragraph mode, freezes

RESOLVED FIXED

Status

()

--
major
RESOLVED FIXED
10 years ago
9 years ago

People

(Reporter: eyalroz, Assigned: neil)

Tracking

({regression, verified1.9.1.1})

unspecified
regression, verified1.9.1.1
Points:
---
Bug Flags:
blocking1.9.1 -
wanted1.9.1 +
wanted1.9.0.x ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

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

Updated

10 years ago
Flags: blocking-thunderbird3?
Keywords: qawanted
nsIHTMLEditor is actually a core component. Therefore moving to the relevant bugzilla component.
Component: Message Compose Window → Editor
Flags: blocking-thunderbird3?
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.
Flags: blocking1.9.1?
(Assignee)

Comment 3

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

Comment 4

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

Comment 5

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

Comment 7

10 years ago
(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.
FWIW, for those STR in tb I get
Error: [Exception... "'JavaScript component does not have a method named: "handleEvent"' when calling method: [nsIDOMEventListener::handleEvent]"  nsresult: "0x80570030 (NS_ERROR_XPC_JSOBJECT_HAS_NO_FUNCTION_NAMED)"  location: "<unknown>"  data: no]
Keywords: regression
(Reporter)

Comment 9

10 years ago
First regressing nightly is 2007-06-29. Weird.
(Reporter)

Comment 10

10 years ago
Um, I was checking Seamonkey nightlies, of course.
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.
Flags: wanted1.9.1+
Flags: blocking1.9.1?
Flags: blocking1.9.1-
(Reporter)

Comment 13

10 years ago
dbaron: How about 1.9.2 then?
Flags: blocking1.9.2?
>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...
(Assignee)

Comment 15

10 years ago
Created attachment 377879 [details] [diff] [review]
Safe fix
Assignee: nobody → neil
Status: NEW → ASSIGNED
Attachment #377879 - Flags: superreview?(peterv)
Attachment #377879 - Flags: review?(peterv)
(Assignee)

Updated

10 years ago
Blocks: 237964
Flags: wanted1.9.1?
Flags: wanted1.9.0.x?
Keywords: qawanted
Hardware: x86 → All
Flags: wanted1.9.1? → wanted1.9.1+
Attachment #377879 - Flags: superreview?(peterv)
Attachment #377879 - Flags: superreview+
Attachment #377879 - Flags: review?(peterv)
Attachment #377879 - Flags: review+
(Assignee)

Updated

9 years ago
Attachment #377879 - Flags: approval1.9.1?
(Assignee)

Comment 16

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

Comment 17

9 years ago
Where's the bug about not using dummy br nodes in the first place? :-(
(no longer need approval to land on trunk)
(Assignee)

Comment 19

9 years ago
Pushed changeset 7fc7e27ed155 to mozilla-central.
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Whiteboard: [3.5.1?]
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 1.9.1.1
Flags: blocking1.9.1.1?
Whiteboard: [3.5.1?]
Not blocking 1.9.1.1, but I'll approve the patch.
Flags: blocking1.9.1.1?
Attachment #377879 - Flags: approval1.9.1.1+
Comment on attachment 377879 [details] [diff] [review]
Safe fix

Approved for 1.9.1.1. a=ss

Please land as soon as you can if you intend to get this non-blocker into 1.9.1.1.
(Assignee)

Comment 23

9 years ago
Pushed changeset a5b258c15577 to releases/mozilla-1.9.1
Keywords: fixed1.9.1.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:1.9.1.1pre) 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.
Keywords: fixed1.9.1.1 → verified1.9.1.1
You need to log in before you can comment on or make changes to this bug.