The default bug view has changed. See this FAQ.

nsHTMLEditor should override IsRootNode

RESOLVED FIXED in mozilla15

Status

()

Core
Editor
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: bz, Assigned: ayg)

Tracking

9 Branch
mozilla15
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 2 obsolete attachments)

And in particular, it should make use of GetActiveEditingHost in there.  Then we can remove most of the 5th patch from bug 635618.

Updated

5 years ago
Whiteboard: [mentor=ehsan][lang=c++]
For any document which is in design mode, or has one or more contenteditable elements, there is one nsHTMLEditor object created.  Therefore, in the contenteditable case, an nsHTMLEditor might be responsible for managing multiple editable regions in the document.  nsHTMLEditor has a notion of "active editing host", which corresponds to the editable region of the document which has the focus and is being edited.  nsHTMLEditor::GetActiveEditingHost() returns the root node of the active editing host region.

nsEditor::IsRootNode is not aware of all this, therefore its implementation for nsHTMLEditor is incorrect.  This causes us to have to check for GetActiveEditingHost all around the code base, for example in this patch: <https://hg.mozilla.org/mozilla-central/rev/68ed3842525d>.  We should override both versions of nsEditor::IsRootNode in nsHTMLEditor, and the new implementation needs compare the node passed in to the result of GetActiveEditingHost().  Once we do that, we can remove most of the checks in <https://hg.mozilla.org/mozilla-central/rev/68ed3842525d> (and maybe other similar checks), because IsRootNode would be doing the check internally.
Created attachment 590453 [details] [diff] [review]
Patch
Attachment #590453 - Flags: feedback?(ehsan)
Comment on attachment 590453 [details] [diff] [review]
Patch

Review of attachment 590453 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks a lot for your patch, Jignesh!

The patch looks good, except for the comment below.  Have you run tests with this patch?  It should probably be a good choice if you submit it to the try server, and see if you get any test failures.  Do you have try server access?

::: editor/libeditor/html/nsHTMLEditor.h
@@ +408,2 @@
>  
> +  bool IsRootNode(nsINode *inNode); 

You need to make this method in nsEditor virtual.  Please also add the virtual keyword to the nsHTMLEditor versions of these methods.
Attachment #590453 - Flags: feedback?(ehsan)
Created attachment 591052 [details] [diff] [review]
Patch

Ehsan,
Our campus firewall blocks ssh push.So I will not be able to push into try for few more days.Can you do it for me? 

Thank you!
Attachment #590453 - Attachment is obsolete: true
Attachment #591052 - Flags: review?(ehsan)
https://tbpl.mozilla.org/?tree=Try&rev=b906b6ba874d
Looks like you're hitting

###!!! ASSERTION: We started with a proper descendant of root, and should stop if we ever hit the root, so we better have a descendant of root now!: 'IsDescendantOfBody(parent)', file /builds/slave/try-lnx64-dbg/build/editor/libeditor/base/nsEditor.cpp, line 3309
(In reply to Ms2ger from comment #6)
> Looks like you're hitting
> 
> ###!!! ASSERTION: We started with a proper descendant of root, and should
> stop if we ever hit the root, so we better have a descendant of root now!:
> 'IsDescendantOfBody(parent)', file
> /builds/slave/try-lnx64-dbg/build/editor/libeditor/base/nsEditor.cpp, line
> 3309

Hmm, the results of the try server job don't seem to be available for some reason.  Do you know which test was triggering this assertion?
https://tbpl.mozilla.org/?tree=Try&rev=1950052411f1
REFTEST TEST-UNEXPECTED-FAIL | file:///home/cltbld/talos-slave/test/build/reftest/tests/editor/libeditor/html/crashtests/582138-1.xhtml | assertion count 2 is more than expected 0 assertions
REFTEST TEST-UNEXPECTED-FAIL | file:///home/cltbld/talos-slave/test/build/reftest/tests/editor/libeditor/html/crashtests/682650-1.html | assertion count 11 is more than expected 0 assertions
REFTEST TEST-UNEXPECTED-FAIL | file:///home/cltbld/talos-slave/test/build/reftest/tests/editor/libeditor/base/crashtests/636074-1.html | assertion count 13 is more than expected 6 assertions
Jignesh, please see comment 9 for the test failures introduced by your patch.

In order to debug these, first run this command to make sure that you can reproduce the test failures locally:

make -C /path/to/objdir crashtest TEST_PATH=editor/crashtests.list

Once you get the same failures locally, try debugging them.  In order to debug them, you can run your patched version of Firefox under a debugger, set a conditional breakpoint on NS_DebugBreak_P with aSeverity==1 as the breakpoint condition.  This breakpoint will only trigger in case there is an assertion.  Once you've done that, open the first HTML file in Firefox and try to investigate why the assertion is happening.

Please let me know if you need help.
I tried to find out. There is completely different set of tests failed locally.

1 )REFTEST TEST-UNEXPECTED-FAIL | file:///home/jiggy/moxilla/asrc/src/editor/libeditor/html/crashtests/382778-1.html | assertion count 2 is more than expected 0 assertions
2 )REFTEST TEST-UNEXPECTED-FAIL | file:///home/jiggy/moxilla/asrc/src/editor/libeditor/html/crashtests/456727-2.html | assertion count 1 is more than expected 0 assertions

4 )REFTEST TEST-UNEXPECTED-FAIL | file:///home/jiggy/moxilla/asrc/src/editor/libeditor/base/crashtests/382527-1.html | assertion count 2 is more than expected 0 assertions


#1 and #4 failed at
http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp#3578 and 
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsFrame.cpp#4160

#2 failed at http://mxr.mozilla.org/mozilla-central/source/accessible/src/base/nsAccTreeWalker.cpp#72 

Let me know if it does help.
(In reply to Jignesh kakadiya from comment #11)
> I tried to find out. There is completely different set of tests failed
> locally.
> 
> 1 )REFTEST TEST-UNEXPECTED-FAIL |
> file:///home/jiggy/moxilla/asrc/src/editor/libeditor/html/crashtests/382778-
> 1.html | assertion count 2 is more than expected 0 assertions
> 2 )REFTEST TEST-UNEXPECTED-FAIL |
> file:///home/jiggy/moxilla/asrc/src/editor/libeditor/html/crashtests/456727-
> 2.html | assertion count 1 is more than expected 0 assertions
>
> 4 )REFTEST TEST-UNEXPECTED-FAIL |
> file:///home/jiggy/moxilla/asrc/src/editor/libeditor/base/crashtests/382527-
> 1.html | assertion count 2 is more than expected 0 assertions
> 
> 
> #1 and #4 failed at
> http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.
> cpp#3578 and 
> http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsFrame.cpp#4160

Hmm, these two assertions are really weird.  Do you get them even without your patch?

> #2 failed at
> http://mxr.mozilla.org/mozilla-central/source/accessible/src/base/
> nsAccTreeWalker.cpp#72 

What is the call stack leading to this assertion?

You should really get assertions similar to the ones in this log: https://tbpl.mozilla.org/php/getParsedLog.php?id=8892094&tree=Try&full=1  Is the crashtest window focused when you ran the tests locally?
Comment on attachment 591052 [details] [diff] [review]
Patch

Clearing the review request for now.
Attachment #591052 - Flags: review?(ehsan)
Jignesh, do you plan to continue to work on this?
I tried it but result coming is not expected. If someone wants to do it go ahead:).
OK, thanks!  Aryeh, can you take a look at this, please?
Assignee: nobody → ayg
Status: NEW → ASSIGNED
Okay, so I looked at this and I don't think we actually want to do it.  There's a bunch of code that assumes IsRootNode() deals with the same root as IsDescendantOfBody() and GetRoot() and various other things, all of which refer to the document root.  I think if we're going to do this, we want a new set of methods, like IsEditorRootNode() and IsDescendantOfEditorRoot() and things like that, which are the same as the existing ones in nsEditor but are overridden in nsHTMLEditor.  I'll write something to this effect and see how it goes.
OK, that sounds good!
Created attachment 619889 [details] [diff] [review]
Patch part 1, v1 -- Name some nsEditor methods more consistently

We had IsDescendantOfBody, IsRootNode, and GetRoot.  I renamed the first two to IsDescendantOfRoot and IsRoot.  A later patch will add EditorRoot variants.
Attachment #591052 - Attachment is obsolete: true
Attachment #619889 - Flags: review?(ehsan)
Created attachment 619890 [details] [diff] [review]
Patch part 2, v1 -- Make methods that return editing hosts return dom::Element* instead of nsIContent*

This is so that the eventual GetEditorRoot can return a dom::Element like GetRoot.  I'm not sure if the static_cast in GetEditingHost is the right way to do things.  The IDL changes were cargo-culted from dom/interfaces/canvas/nsIDOMWebGLRenderingContext.idl.  I didn't rev the UUID because it should be compatible, since we're still returning an nsIContent*, right?  (I'm still not sure when I need to do that, exactly.)
Attachment #619890 - Flags: review?(ehsan)
Created attachment 619892 [details] [diff] [review]
Patch part 3, v1 -- Clean up some nsEditor methods
Attachment #619892 - Flags: review?(ehsan)
Created attachment 619894 [details] [diff] [review]
Patch part 4, v1 -- Add and use new EditorRoot methods in nsEditor
Attachment #619894 - Flags: review?(ehsan)
Created attachment 619895 [details] [diff] [review]
Patch part 5, v1 -- Remove now-redundant nsHTMLEditor::IsNodeInActiveEditor

I realized after the fact that this is the same as the new nsEditor::IsDescendantOfEditorRoot, which I prefer due to more consistent naming.
Attachment #619895 - Flags: review?(ehsan)
Flags: in-testsuite-
Whiteboard: [mentor=ehsan][lang=c++] → [autoland-try:-u all]

Updated

5 years ago
Whiteboard: [autoland-try:-u all] → [autoland-in-queue]

Updated

5 years ago
Attachment #619889 - Flags: review?(ehsan) → review+
Comment on attachment 619890 [details] [diff] [review]
Patch part 2, v1 -- Make methods that return editing hosts return dom::Element* instead of nsIContent*

Review of attachment 619890 [details] [diff] [review]:
-----------------------------------------------------------------

FWIW, not revving the IID is fine here since Element's vtable just adds to nsIContent's, so that pointer will be binary compatible with the current nsIContent pointer.

::: content/base/src/nsGenericElement.cpp
@@ +1572,3 @@
>      content = parent;
>    }
> +  return static_cast<dom::Element*>(content);

Please use AsElement().
Attachment #619890 - Flags: review?(ehsan) → review+

Updated

5 years ago
Attachment #619892 - Flags: review?(ehsan) → review+
Comment on attachment 619894 [details] [diff] [review]
Patch part 4, v1 -- Add and use new EditorRoot methods in nsEditor

Review of attachment 619894 [details] [diff] [review]:
-----------------------------------------------------------------

::: editor/libeditor/base/nsEditor.cpp
@@ +3563,5 @@
> +bool
> +nsEditor::IsEditorRoot(nsINode* aNode)
> +{
> +  NS_ENSURE_TRUE(aNode, false);
> +  nsCOMPtr<nsINode> rootNode = GetEditorRoot();

This QI here is unneeded, please get rid of it.

@@ +3588,5 @@
> +bool
> +nsEditor::IsDescendantOfEditorRoot(nsINode* aNode)
> +{
> +  NS_ENSURE_TRUE(aNode, false);
> +  nsCOMPtr<nsIContent> root = GetEditorRoot();

Same with this one.
Attachment #619894 - Flags: review?(ehsan) → review+
Comment on attachment 619895 [details] [diff] [review]
Patch part 5, v1 -- Remove now-redundant nsHTMLEditor::IsNodeInActiveEditor

Review of attachment 619895 [details] [diff] [review]:
-----------------------------------------------------------------

Nice!
Attachment #619895 - Flags: review?(ehsan) → review+
(In reply to Ehsan Akhgari [:ehsan] from comment #25)
> This QI here is unneeded, please get rid of it.

What do you mean by QI?  The nsCOMPtr?  I'm not clear on exactly when I should use those.  Do you want just

bool
nsEditor::IsEditorRoot(nsINode* aNode)
{
  NS_ENSURE_TRUE(aNode, false);
  return aNode == GetEditorRoot();
}

instead, and likewise for IsDescendantOfEditorRoot?  (And if so, should I update the IsRoot and IsDescendantOfRoot methods I copied from?)

If that's not what you mean, what do you mean?  I don't know what calls QueryInterface under the covers.
(Note: this massively conflicted with bug 750064.  For the checkin, I'm dropping patch #3 because bug 750064 cleaned up the same methods, and the rest I merged.)
Hmm, OK I think I read the patch wrong.  There's no QI involved here, so please ignore that comment.  Sorry about that!
No problem.  Thanks!

https://hg.mozilla.org/integration/mozilla-inbound/rev/18c27249fc7f
https://hg.mozilla.org/integration/mozilla-inbound/rev/edebd9b79fe5
https://hg.mozilla.org/integration/mozilla-inbound/rev/222fc2b8378f
https://hg.mozilla.org/integration/mozilla-inbound/rev/8396c8f870d3
OS: Mac OS X → All
Hardware: x86 → All
Target Milestone: --- → mozilla15
https://hg.mozilla.org/mozilla-central/rev/18c27249fc7f
https://hg.mozilla.org/mozilla-central/rev/edebd9b79fe5
https://hg.mozilla.org/mozilla-central/rev/222fc2b8378f
https://hg.mozilla.org/mozilla-central/rev/8396c8f870d3
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [autoland-in-queue]

Comment 32

5 years ago
Autoland Patchset:
	Patches: 619889, 619890, 619892, 619894, 619895
	Branch: mozilla-central => try
Patch 619889 could not be applied to mozilla-central.
patching file editor/libeditor/base/nsEditor.cpp
Hunk #1 FAILED at 3229
Hunk #2 FAILED at 3267
Hunk #3 FAILED at 3288
Hunk #4 FAILED at 3320
Hunk #5 FAILED at 3363
Hunk #6 FAILED at 3381
Hunk #7 FAILED at 3558
7 out of 7 hunks FAILED -- saving rejects to file editor/libeditor/base/nsEditor.cpp.rej
patching file editor/libeditor/base/nsEditor.h
Hunk #1 FAILED at 533
1 out of 1 hunks FAILED -- saving rejects to file editor/libeditor/base/nsEditor.h.rej
patching file editor/libeditor/html/nsHTMLEditRules.cpp
Hunk #1 FAILED at 8395
1 out of 1 hunks FAILED -- saving rejects to file editor/libeditor/html/nsHTMLEditRules.cpp.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir

Patchset could not be applied and pushed.
Depends on: 805668
You need to log in before you can comment on or make changes to this bug.