545775 - Text editor in ASUS Forum doesn't work.

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect, P2)

Description

[Gary [:streetwolf52]]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.3a2pre) Gecko/20100211 Minefield/3.7a2pre Firefox/3.6
Build Identifier: 20100211050928

When starting a new reply or topic the editor in the ASUS Forum does not work properly.  It appears it uses the FreeTextBox HTML editor for ASP.NET.

If you use the 'Quote and Reply' option the editor works fine.

Reproducible: Always

Steps to Reproduce:
1. Post a new reply or topic in the ASUS forum.  Registration is required.
2. Try to enter text in the editor.
3.
Actual Results:  
Text does not appear.

Expected Results:  
Text should appear and all functions should work.

Sometimes I can get one character to appear.

Comment 1

[Gary [:streetwolf52]]

BTW.... Works fine in IE8

Comment 2

[Boris Zbarsky [:bzbarsky]]

Does it work in Firefox 3.6?

Comment 3

[Warner Young]

Confirmed in FF 3.7.  This site also works fine in IE6.  I'll try to test it in 3.6 later if I can.

Comment 4

[Gary [:streetwolf52]]

Works fine in 3.6.  Tried Trunk with a new profile and it still doesn't work.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Excellent.  Can you hunt down the trunk nightlies that bracket the regression and post their revision ids (from about:buildconfig) here?

Comment 6

[Gary [:streetwolf52]]

I'll get on it tomorrow first thing.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Awesome.  Thank you!

Comment 8

[Warner Young]

I haven't had time to check all the nightlies, but here's what I got so far (I'm assuming the revision you want is the part that says "Built from"):
OK	20091214 http://hg.mozilla.org/mozilla-central/rev/44c392db6672
bad	20091218 http://hg.mozilla.org/mozilla-central/rev/58359ad70d1b

Gary, I hope this helps narrow it down for you so you have less work to do.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Warner, that's exactly the info I want, yeah.

4-day range based on those changeset ids:

http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=44c392db6672&tochange=58359ad70d1b

Lots of stuff in there, unfortunately...

Comment 10

[Gary [:streetwolf52]]

http://hg.mozilla.org/mozilla-central/rev/dba2abb7db57     OK
http://hg.mozilla.org/mozilla-central/rev/cdb87287a747     BAD

Hope I got it right.

Comment 11

[Boris Zbarsky [:bzbarsky]]

That would be this range:

http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=dba2abb7db57&tochange=cdb87287a747

You said you tested with a new profile, so I assume you don't have the HTML5 parser enabled.

In which case nothing in the range jumps out at me.  :(  I guess I'll see if I can reproduce locally...

Comment 12

[Gary [:streetwolf52]]

It fails to work either way, enabled or disabled.

Comment 13

[Boris Zbarsky [:bzbarsky]]

So this thing actually needs a whole bunch of personal information... If either one of you has a "throwaway" account, want to mail me the login info for it?  If not I guess I'll set one up...

Comment 14

[Gary [:streetwolf52]]

Boris, sent you an email.  Happy hunting.

Comment 15

[Boris Zbarsky [:bzbarsky]]

The first bad revision is:
changeset:   36324:b20cff5e2157
user:        Masayuki Nakano <[e-mail]>
date:        Thu Dec 17 13:46:30 2009 +0900
summary:     Bug 535041 Crash [@ nsContentUtils::IsInSameAnonymousTree] r=Olli

In the failing build, the editor has no mRootContent for a bit and probably fails to init properly...

Comment 16

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

see url for minimal test case of http://vip.asus.com/forum/default.aspx?SLanguage=en-us

seems the root element replacing cannot be handled correctly.

Comment 17

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v1.0

I'll create automated test for this bug ASAP.

Comment 18

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v2.0

Comment 19

[Boris Zbarsky [:bzbarsky]]

Why do you need to check for a null aContainer in ContentAppended?

Also, why is it ok to bail out if a textnode followed by an element is appended?

Comment 20

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #19)
> Why do you need to check for a null aContainer in ContentAppended?

I'm not sure whether aContainer must not be null. Is that guaranteed?

> Also, why is it ok to bail out if a textnode followed by an element is
> appended?

Because mRootElement must be a body element or the document root node or an anonymous div element. Is there a case that a text node can be root node?

Comment 21

[Boris Zbarsky [:bzbarsky]]

> I'm not sure whether aContainer must not be null. Is that guaranteed?

http://hg.mozilla.org/mozilla-central/annotate/ed7d1a491a8e/content/base/public/nsIMutationObserver.h#l180 says:

180    * @param aContainer The container that had new children appended. Is never
181    *                   null.

So yes, it is guaranteed.

> Is there a case that a text node can be root node?

No, but why should a <body> not be a root node just because there's some whitespace before it?

Comment 22

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v2.0 + testcases

Sorry for the delay. I'll check the bz's comment.

The automated tests cannot pass yet. The *first* test failed at IME state checking. And 5th test failed, the editor isn't editable and there is unknown br node. I'll check it next week.

Comment 23

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v2.0 + testcases

sorry for the spam.

Comment 24

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Note for my self: Need to get nsIEditor and should check whether the root element of it is expected or not.

Comment 25

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I need to fix bug 544277 first, that is one of the causes of test failure.

Comment 26

[Gary [:streetwolf52]]

Might I inquire when this will land on Trunk.

Comment 27

[(no longer active)]

Masayuki, with bug 544277 fixed, is this patch ready for review?

Comment 28

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

No, the patch failed some tests, I need more work.

Comment 29

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v3.0

This fixes most part of this bug. However, there are still two problems.

1. Caret isn't visible when set focus to the editor.
2. First character of IME composition is forcedly committed.

Comment 30

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v4.0

The basic concept is that nsHTMLEditor should store the best mRootElement every time. The patch checks whether a better root element is inserted or appended to the DOM tree. If it's inserted or appended, this patch reintialize the selection and caret.

For simplizing the code, this patch moves the mutation event handler from nsEditor to nsHTMLEditor. This should helps the plain text editor's footprint and performance.

Comment 31

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

NOTE: I want to test the patch with latest code, but tryserver aborts the transaction today...

> pushing to ssh://hg.mozilla.org/try/
> searching for changes
> remote: adding changesets
> remote: adding manifests
> remote: adding file changes
> remote: added 1 changesets with 0 changes to 7 files (+1 heads)
> remote: pulling from /repo/hg/mozilla/try
> remote: searching for changes
> remote: adding changesets
> remote: adding manifests
> remote: adding file changes
> remote: transaction abort!
> remote: rollback completed
> remote: abort: Permission denied: /repo/hg/mozilla/try_vcview/.hg/store/data/editor/libeditor/html/tests/test__root__element__replacement.html.i
> remote: warning: changegroup.z_push_to_vcview hook exited with status 255

The patch worked fine on trysever a week ago.

Comment 32

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

okay, tryserver is back now. I posted the patch to it.

Comment 33

[(no longer active)]

(In reply to comment #31)
> NOTE: I want to test the patch with latest code, but tryserver aborts the
> transaction today...
> 
> > pushing to ssh://hg.mozilla.org/try/
> > searching for changes
> > remote: adding changesets
> > remote: adding manifests
> > remote: adding file changes
> > remote: added 1 changesets with 0 changes to 7 files (+1 heads)
> > remote: pulling from /repo/hg/mozilla/try
> > remote: searching for changes
> > remote: adding changesets
> > remote: adding manifests
> > remote: adding file changes
> > remote: transaction abort!
> > remote: rollback completed
> > remote: abort: Permission denied: /repo/hg/mozilla/try_vcview/.hg/store/data/editor/libeditor/html/tests/test__root__element__replacement.html.i
> > remote: warning: changegroup.z_push_to_vcview hook exited with status 255
> 
> The patch worked fine on trysever a week ago.

You might want to file a releng bug about that.

Comment 34

[(no longer active)]

Comment on attachment 450990 [details] [diff] [review]
Patch v4.0

So, I have two questions about this patch:

1. Is it safe to move the mutation observer to the HTML editor?  Does this handle mutations to the textnode under a textarea correctly?

2. This patch contains a bunch of refactoring, which makes seeing the actual approach to fixing the problem extremely hard.  Could you describe what your approach to the fix was, sans the refactorings?  Are the refactorings necessary for the fix?  If not, then could you please split them in a separate patch?  This is needed for branch, and in its current form, this patch really scares me for branch consideration.

Comment 35

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

nsPlaintextEditor doesn't use the mutation event. I'm not sure why many features of nsHTMLEditor has been implemented in nsEditor rather than nsHTMLEditor.

O.K., I'll post separated patches, but I don't want to separate bugs because I tested this patch well.

And this shouldn't be needed on branch because this is a regression of bug 535041.

Comment 36

[(no longer active)]

(In reply to comment #35)
> nsPlaintextEditor doesn't use the mutation event. I'm not sure why many
> features of nsHTMLEditor has been implemented in nsEditor rather than
> nsHTMLEditor.

But the ContentInserted and ContentRemoved methods do things which seem relevant to the plaintext editor as well...

> O.K., I'll post separated patches, but I don't want to separate bugs because I
> tested this patch well.

Great, thanks!

> And this shouldn't be needed on branch because this is a regression of bug
> 535041.

Ah, you're right; sorry.  I took this for bug 543077!  :-)

Comment 37

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #36)
> (In reply to comment #35)
> > nsPlaintextEditor doesn't use the mutation event. I'm not sure why many
> > features of nsHTMLEditor has been implemented in nsEditor rather than
> > nsHTMLEditor.
> 
> But the ContentInserted and ContentRemoved methods do things which seem
> relevant to the plaintext editor as well...

mRootElement is initialized by nsEditor::Init() called by nsTextEditorState::PrepareEditor().
http://mxr.mozilla.org/mozilla-central/source/content/html/content/src/nsTextEditorState.cpp#1157
Then, the root node is anonymous div.
http://mxr.mozilla.org/mozilla-central/source/content/html/content/src/nsTextEditorState.cpp#1439

And this is never modified when the editor is alive. So, nsEditor::ContentInserted() does nothing. And nsEditor::ContentRemoved()'s element == mRootElement always false.

The mutation observer was implemented for bug 335856 which is a HTML editor bug like this.

Comment 38

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v4.0 part.1 Separate GetRootElement()

This patch separates the GetRootElement() to nsEditor::GetRootElement() and nsHTMLEditor::GetRootElement(). This makes clearer.

Comment 39

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v4.0 part.2 Refactor GetPIDOMEventTarget()

This patch guarantees the result of GetPIDOMEventTarget() is current event target which listens the events.

Comment 40

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v4.0 part.3 Move selection initializer into nsEditor

This patch moves selection and caret initializing code from nsEditorEventListener::Focus() to nsEditor. The new method will be called when the editor root element will be replaced.

Comment 41

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v4.0 part.4 Move mutation observer to nsHTMLEditor

This patch moves the mutation observer from nsEditor to nsHTMLEditor. By this patch, the next patch can fix this bug by changing only nsHTMLEditor.

Comment 42

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v4.0 pat.5 fix this bug

Just fix this bug.

Comment 43

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v4.0 pat.5 fix this bug

sorry for the spam.

Comment 44

[(no longer active)]

Comment on attachment 451259 [details] [diff] [review]
Patch v4.0 part.2 Refactor GetPIDOMEventTarget()

This is definitely an improvement over what we have right now, but let's put plaintext editor's GetPIDOMEventTarget logic in nsPlaintextEditor, now that we're putting the rest in nsHTMLEditor.

Comment 45

[(no longer active)]

(In reply to comment #37)
> (In reply to comment #36)
> > (In reply to comment #35)
> > > nsPlaintextEditor doesn't use the mutation event. I'm not sure why many
> > > features of nsHTMLEditor has been implemented in nsEditor rather than
> > > nsHTMLEditor.
> > 
> > But the ContentInserted and ContentRemoved methods do things which seem
> > relevant to the plaintext editor as well...
> 
> mRootElement is initialized by nsEditor::Init() called by
> nsTextEditorState::PrepareEditor().
> http://mxr.mozilla.org/mozilla-central/source/content/html/content/src/nsTextEditorState.cpp#1157
> Then, the root node is anonymous div.
> http://mxr.mozilla.org/mozilla-central/source/content/html/content/src/nsTextEditorState.cpp#1439

So, as of bug 534785, a plaintext editor control now can be initialized multiple times (i.e., sequences of Init, PreDestroy, Init, PreDestroy, ... calls).

Given that, mRootElement can actually change.  How does that affect your assessment above?

Comment 46

[(no longer active)]

Comment on attachment 451263 [details] [diff] [review]
Patch v4.0 pat.5 fix this bug

>+void
>+nsHTMLEditor::ResetRootElementAndEventTarget()
>+{
>+  // Need to remove the event listeners first because BeginningOfDocument
>+  // could set a new root (and event target is set by InstallEventListeners())
>+  // and we won't be able to remove them from the old event target then.
>+  RemoveEventListeners();
>+  mRootElement = nsnull;
>+  nsresult rv = InstallEventListeners();
>+  if (NS_FAILED(rv) || !mRootElement) {
>+    return;
>+  }

Could you add a warning here?

>+  rv = BeginningOfDocument();
>+  if (NS_FAILED(rv)) {
>+    return;
>+  }

Please use:

NS_ENSURE_SUCCESS(rv, )

>+nsINode*
>+nsHTMLEditor::GetFocusedNode()
>+{
>+  if (!HasFocus()) {
>+    return nsnull;
>+  }
>+
>+  nsIFocusManager* fm = nsFocusManager::GetFocusManager();
>+  NS_ASSERTION(fm, "Focus manager is null");
>+  nsCOMPtr<nsIDOMElement> focusedElement;
>+  fm->GetFocusedElement(getter_AddRefs(focusedElement));
>+  if (focusedElement) {
>+    nsCOMPtr<nsINode> node = do_QueryInterface(focusedElement);
>+    return node;
>+  }
>+
>+  nsCOMPtr<nsIDocument> doc = do_QueryReferent(mDocWeak);
>+  return doc;
>+}

The way that this function returns the pointer after releasing it is not safe.  The returned pointers can be dangling.

>+SimpleTest.waitForFocus(runTest, window);

You can drop window here.

>+    is(content.nodeType, 3,
>+       "the content of body isn't text node -- " + kTest.description);

Can you use the named constant instead of 3 here?

Comment 47

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #45)
> So, as of bug 534785, a plaintext editor control now can be initialized
> multiple times (i.e., sequences of Init, PreDestroy, Init, PreDestroy, ...
> calls).
> 
> Given that, mRootElement can actually change.  How does that affect your
> assessment above?

PreDestroy() uninstalls the mutation observer. So, my patch shouldn't change any behavior of the case. If the root element is replaced druing Init() and PreDestroy(), my patch is wrong. However, mRootElement shouldn't be replaced at that time because it's only reset in Init() and GetRoot(). But GetRoot() doesn't have code for plaintext editor. The logic is for HTML editor.

Comment 48

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v4.1 part.2 Refactor GetPIDOMEventTarget()

I'm not sure this is better than the previous one because the boundary of nsEditor and nsPlaintextEditor isn't clear.  This patch needs to change nsEditor::InstallEventListener() too.

Ideally, we should unify nsEditor and nsPlaintextEditor in future.

Comment 49

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v4.0 part.3 Move selection initializer into nsEditor (for v4.1 part.2)

Comment 50

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v4.1 pat.5 fix this bug

Comment 51

[(no longer active)]

Comment on attachment 451546 [details] [diff] [review]
Patch v4.1 pat.5 fix this bug

Looks good, but it would be great if you can address my first two nits in comment 46 before landing.

Comment 52

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #51)
> (From update of attachment 451546 [details] [diff] [review])
> Looks good, but it would be great if you can address my first two nits in
> comment 46 before landing.

Oops, sorry. I'll post the patch soon.

Comment 53

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v4.2 part.5 fix this bug

Sorry, using NS_ENSURE_* for them.

Comment 54

[(no longer active)]

Comment on attachment 451640 [details] [diff] [review]
Patch v4.2 part.5 fix this bug

Thanks!

The reasons that I think we should use NS_ENSURE macros here is that they are easier to read, and they print a warning to the console if their condition is not satisfied in debug builds.

Comment 55

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

http://hg.mozilla.org/mozilla-central/rev/c84bf31888d6
http://hg.mozilla.org/mozilla-central/rev/55bc63d72504
http://hg.mozilla.org/mozilla-central/rev/224af2d73b56
http://hg.mozilla.org/mozilla-central/rev/a7548c52f067
http://hg.mozilla.org/mozilla-central/rev/4ee79cff0024

Yeah, of course, I agreed!

Comment 56

[Gary [:streetwolf52]]

There still is a problem with editing on the Asus site.

1.  There is no insert cursor in the text area until you start typing

2.  More importantly when you submit what you typed you get a response that indicates nothing was typed in.

Comment 57

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Followup patch v1.0

Sorry, there is a mistake. mRootElement is initialized by BeginningOfDocument() (it calls GetRoot()). For future changes, ResetRootElementAndEventTarget() should call GetRoot() directly.

And I'm not sure the second issue. It should be out of scope of this bug if this patch doesn't fix it.

Comment 58

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Additional patch for the followup patch v1.0

Ugh, the patch creates new assertions on crashtests.

> ###!!! ASSERTION: No first node!: 'mFirst', file m:/mozilla-o/src/content/base/src/nsContentIterator.cpp, line 920
> xul!nsFilteredContentIterator::First+0x0000000000000082 (m:\mozilla-o\src\editor\txtsvc\src\nsfilteredcontentiterator.cpp, line 180)
> xul!nsTextServicesDocument::FirstTextNode+0x0000000000000022 (m:\mozilla-o\src\editor\txtsvc\src\nstextservicesdocument.cpp, line 3445)
> xul!nsTextServicesDocument::FirstBlock+0x0000000000000056 (m:\mozilla-o\src\editor\txtsvc\src\nstextservicesdocument.cpp, line 509)
> xul!nsTextServicesDocument::InitWithEditor+0x0000000000000245 (m:\mozilla-o\src\editor\txtsvc\src\nstextservicesdocument.cpp, line 228)
> xul!mozInlineSpellChecker::SetEnableRealTimeSpell+0x0000000000000338 (m:\mozilla-o\src\extensions\spellcheck\src\mozinlinespellchecker.cpp, line 742)
> xul!nsEditor::SyncRealTimeSpell+0x000000000000007B (m:\mozilla-o\src\editor\libeditor\base\nseditor.cpp, line 1289)
> xul!nsHTMLEditor::ResetRootElementAndEventTarget+0x0000000000000251 (m:\mozilla-o\src\editor\libeditor\html\nshtmleditor.cpp, line 5901)

When we create a content iterator with empty body document, the iterator outputs the assertion at First(). I think that the iterator user cannot check whether the iterator has contents or not. Therefore, I think that it isn't good behavior.

http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsContentIterator.cpp#920

Comment 59

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

http://hg.mozilla.org/mozilla-central/rev/5f837430e132
http://hg.mozilla.org/mozilla-central/rev/18644dcb0677

fixes the caret problem.

Comment 60

[Gary [:streetwolf52]]

(In reply to comment #56)
> There still is a problem with editing on the Asus site.
> 
> 1.  There is no insert cursor in the text area until you start typing
> 
> 2.  More importantly when you submit what you typed you get a response that
> indicates nothing was typed in.

FYI

BOTH problems were fixed with this update.  Good job.