748303 - Editor cleanup

Status: RESOLVED FIXED

(Core :: DOM: Editor, enhancement)

Description

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

If you have an empty editable div and type in "foo\nbar", IE/Opera generate
  <p>foo</p><p>bar</p>,
WebKit generates
  foo<div>bar</div>,
and Gecko generates
  foo<br>bar.
Likewise, if you have
  foo<blockquote>bar</blockquote>baz
and outdent, Gecko generates <br>s to separate the lines, IE/Opera generates <p>, WebKit <div>.  After lots of discussion, the spec requires <p>, with <div> allowable optionally using the defaultParagraphSeparator command.  Gecko should update to match the spec.

This should have no big web-compat impact, because it will just be changing to match IE, but there will probably be problems with Thunderbird and/or Composer -- maybe they'll want to set defaultParagraphSeparator = div to make sure there's no extra whitespace.

I think this is one of the most annoying browser differences in contenteditable, particularly since it hits people who aren't even using execCommand() (i.e., most editors).  Recent WebKit still defaults to div but supports defaultParagraphSeparator; if Gecko output <p> instead of <br>, the latest version of every browser could be made to output <p>, which would be a nice compat win for authors.

(richtext2 requires the non-Gecko behavior here for outdent.)

Comment 1

[(no longer active)]

Hmm, I thought we fixed this a while ago.  Note that we would probably have to keep the cmd_insertBrOnReturn command working.

There's also a pref which implements a node used by Thunderbird and Composer which lets you enter a BR the first time you press Enter, and replace it with a new paragraph element if you press Enter immediately after.  I can't seem to remember the name of the pref, and I can't find it now, but Fabien knows about it.

Comment 2

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Patch part 1, v1 -- Clean up various nsHTMLEditRules methods

This is proving harder than I thought.  But I have some cleanup patches to submit for now, at least.

Comment 3

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Patch part 2, v1 -- Remove unused extra parameter from nsHTMLEditRules::GetNodesFromPoint

Comment 4

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Patch part 2, v1 -- Allow easier use of const nsIAtom*

I could drop this patch from the series easily enough if it's not wanted.  I have it here because the next patch converts some const nsAString& to const nsIAtom*, and that requires this.  I could make the next patch change to nsIAtom* instead, but I figured I may as well keep the constness.

Comment 5

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Patch part 3, v1 -- Convert (Tag)CanContain(Tag) to const nsIAtom*

This mostly results in simpler code, because nsAutoString temporaries aren't needed.  In a few cases I needed to add an NS_GetStaticAtom() call.

Comment 6

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Comment on attachment 618247 [details] [diff] [review]
Patch part 2, v1 -- Remove unused extra parameter from nsHTMLEditRules::GetNodesFromPoint

Okay, I feel silly.  Right after submitting this, I realized I wanted the extra parameter.  :)

Comment 7

[Mozilla RelEng Bot]

Autoland Patchset:
	Patches: 618246, 618248, 618249
	Branch: mozilla-central => try
	Destination: http://hg.mozilla.org/try/pushloghtml?changeset=e097ddb45a60
Try run started, revision e097ddb45a60. To cancel or monitor the job, see: https://tbpl.mozilla.org/?tree=Try&rev=e097ddb45a60

Comment 8

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Comment on attachment 618249 [details] [diff] [review]
Patch part 3, v1 -- Convert (Tag)CanContain(Tag) to const nsIAtom*

There are crashes on the try server, probably from this patch, so I'll drop the review request and debug tomorrow.  (I thought I ran tests in editor/ locally, but maybe not.)

Comment 9

[(no longer active)]

Comment on attachment 618246 [details] [diff] [review]
Patch part 1, v1 -- Clean up various nsHTMLEditRules methods

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

::: editor/libeditor/html/nsHTMLEditRules.cpp
@@ +1597,2 @@
>      *aHandled = true;
> +    return ReturnInListItem(aSelection, listItem, node, offset);

You're changing the logic here by potentially returning an error code (and in other places in this patch too.)  Why is this OK?

Comment 10

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Comment on attachment 618246 [details] [diff] [review]
Patch part 1, v1 -- Clean up various nsHTMLEditRules methods

Oops, you're right.  I was confused by the fact that it said "res =" but then didn't do anything with res (like NS_ENSURE_SUCCESS or return it).  I don't think I was very awake when I wrote this patch series . . .

Comment 11

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Comment on attachment 618246 [details] [diff] [review]
Patch part 1, v1 -- Clean up various nsHTMLEditRules methods

I looked over the patch again and think it probably makes sense.  There are three places in the current code where a value is assigned to res but then ignored.  The return values of ReturnInListItem and ReturnInHeader are always ignored -- but this is the only place they're called, so if we really want to ignore their return value why wouldn't we return void?  Also, the return value of ReturnInParagraph is checked -- why the inconsistency?  I think ignoring the return value is just a bug.

Likewise, IsEmptyBlock has its return value assigned to res but then not checked afterward.  Maybe we really want to do this, but I doubt it.  The patch here passes tests (I think!).  But if you like, I can remove the "res =" and not check for errors in these three cases.

(Really these methods should never be returning errors anyway if there aren't serious bugs in the code.  I wish we had a better failure mode for null pointer errors than returning an error code; this kind of internal function should really have void return type.  All this NS_ENSURE_SUCCESS stuff stinks.  :(  But do we really want to fail silently if we passed a null pointer somehow or similar?)

Comment 12

[(no longer active)]

(In reply to Aryeh Gregor from comment #11)
> Comment on attachment 618246 [details] [diff] [review]
> Patch part 1, v1 -- Clean up various nsHTMLEditRules methods
> 
> I looked over the patch again and think it probably makes sense.  There are
> three places in the current code where a value is assigned to res but then
> ignored.  The return values of ReturnInListItem and ReturnInHeader are
> always ignored -- but this is the only place they're called, so if we really
> want to ignore their return value why wouldn't we return void?  Also, the
> return value of ReturnInParagraph is checked -- why the inconsistency?  I
> think ignoring the return value is just a bug.

Making them return void is fine.  But note that your patch *does* propagate those error values to the caller.  That changes behavior, and I'm not fine with that, unless we understand all of the implications.

Here's the deal.   There are a lot of places in the editor code where we do error checking which makes no sense (either because the error situation does not exist, such as a method only ever returning NS_OK, or we fail to deal with error situations which are to be expected) and we do have bugs about that.  In general any patch in editor/ which changes the error handling should come with a detailed analysis on why that is fine, otherwise we'll expose ourselves to hard to detect regressions.

> Likewise, IsEmptyBlock has its return value assigned to res but then not
> checked afterward.  Maybe we really want to do this, but I doubt it.  The
> patch here passes tests (I think!).  But if you like, I can remove the "res
> =" and not check for errors in these three cases.

Yes, I think that's what you should do (and make those functions return void as well!).

> (Really these methods should never be returning errors anyway if there
> aren't serious bugs in the code.  I wish we had a better failure mode for
> null pointer errors than returning an error code; this kind of internal
> function should really have void return type.

Sometimes there's a function which actually _can_ be called with a null pointer (whether or not that makes sense is another issue!).  But for stuff which doesn't really expect null parameters, we should use MOZ_ASSERT.

> All this NS_ENSURE_SUCCESS
> stuff stinks.  :(  But do we really want to fail silently if we passed a
> null pointer somehow or similar?)

I'm not quite sure what you mean here.

Comment 13

[(no longer active)]

Comment on attachment 618246 [details] [diff] [review]
Patch part 1, v1 -- Clean up various nsHTMLEditRules methods

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

r- per comments.

Comment 14

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Patch part 1, v2 -- Clean up various nsHTMLEditRules methods

Error-checking should be the same as before the patch now.  I didn't change any return types to void.

Comment 15

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Patch part 2, v1 -- Handle non-text/elements correctly when calling (Tag)CanContain(Tag)

When writing the next cleanup patch in this series, I noticed a bug in nsHTMLEditor::InsertNodeAtPoint.  It does, basically:

  nsAutoString tagName;
  aNode->GetNodeName(tagName);
  ToLowerCase(tagName);
  // . . .
  // Search up the parent chain to find a suitable container
  while (!CanContainTag(parent, tagName))
  {

CanContainTag and friends expect that the tag name is either an element name, or "#text".  But it turns out that it's possible for aNode in this case to be a document, in which case tagName here becomes "#document", which is treated the same as an unrecognized element, so CanContainTag returns true, despite the fact that obviously nothing should contain a document.

This situation is hit by editor/libeditor/html/tests/test_bug520189.html somehow, which is what caused the crashes mentioned in comment 8.  You can see the stack trace at <https://tbpl.mozilla.org/php/getParsedLog.php?id=11197684&tree=Try#error0> -- the fact that anything is trying to insert a document into something else might warrant a followup bug.  But this caller is incorrect anyway, even if its caller is incorrect too.  Grabbing a node's name without making sure it's a text or element node isn't correct.  If it's a PI or doctype, the name could be an identifier like "html" even though it's not an element.

I looked at the logic here, and it seems like what would have happened before is that InsertNode would eventually have been called, which would do the insertion transaction, which would fail eventually with some kind of error, which would get thrown.  With this patch, it will throw NS_ERROR_FAILURE before trying to insert the node, since it can't find any parent that can contain aNode.

Comment 16

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Patch part 4, v2 -- Convert (Tag)CanContain(Tag) to const nsIAtom*

What gives me pause about this patch is the use of NS_GetStaticAtom.  In all the cases where it's used, it shouldn't return nsnull -- that would be a bug in the caller.  If it does, TagCanContainTag will call NS_ASSERTION and return false, which doesn't occur in any of our tests.  I could do NS_NewAtom instead if you prefer.

Also, in one place I use nsAtomString to convert an nsIAtom* to a string -- should I use nsDependentAtomString instead?

Comment 17

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Autoland is confused, so here's my manual try: https://tbpl.mozilla.org/?tree=Try&rev=f6ed810c1154

Comment 18

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 619398 [details] [diff] [review]
Patch part 4, v2 -- Convert (Tag)CanContain(Tag) to const nsIAtom*

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

\o/

::: editor/libeditor/base/nsEditor.cpp
@@ +3548,5 @@
>  
>  bool 
> +nsEditor::TagCanContain(const nsIAtom* aParentTag, nsIDOMNode* aChild)
> +{
> +  if (IsTextNode(aChild)) {

I would QI to nsIContent and then check NodeType(), myself.

::: editor/libeditor/html/nsHTMLEditRules.cpp
@@ +2817,5 @@
>  nsHTMLEditRules::MoveNodeSmart(nsIDOMNode *aSource, nsIDOMNode *aDest, PRInt32 *aOffset)
>  {
>    NS_ENSURE_TRUE(aSource && aDest && aOffset, NS_ERROR_NULL_POINTER);
>  
>    nsAutoString tag;

Now unused

@@ +3022,5 @@
>      res = mHTMLEditor->GetStartNodeAndOffset(aSelection, getter_AddRefs(parent), &offset);
>      NS_ENSURE_SUCCESS(res, res);
>      
>      // make sure we can put a list here
> +    nsIAtom* listTypeAtom = NS_GetStaticAtom(*aListType);

nsCOMPtr<nsIAtom> listTypeAtom = do_GetAtom(*aListType);

::: editor/libeditor/html/nsHTMLEditor.cpp
@@ +3782,4 @@
>  {
> +  NS_ASSERTION(aParentTag, "Null aParentTag");
> +  NS_ASSERTION(aChildTag, "Null aChildTag");
> +  if (!aParentTag || !aChildTag) {

Either MOZ_ASSERT or handle this, not both, please.

Comment 19

[Mozilla RelEng Bot]

Try run for f6ed810c1154 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=f6ed810c1154
Results (out of 218 total builds):
    success: 181
    warnings: 36
    failure: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-f6ed810c1154

Comment 20

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

(In reply to Ms2ger from comment #18)
> ::: editor/libeditor/base/nsEditor.cpp
> @@ +3548,5 @@
> >  
> >  bool 
> > +nsEditor::TagCanContain(const nsIAtom* aParentTag, nsIDOMNode* aChild)
> > +{
> > +  if (IsTextNode(aChild)) {
> 
> I would QI to nsIContent and then check NodeType(), myself.

I was trying not to change the existing code structure too much.  But yes, that might make more sense.

> 
> ::: editor/libeditor/html/nsHTMLEditRules.cpp
> @@ +2817,5 @@
> >  nsHTMLEditRules::MoveNodeSmart(nsIDOMNode *aSource, nsIDOMNode *aDest, PRInt32 *aOffset)
> >  {
> >    NS_ENSURE_TRUE(aSource && aDest && aOffset, NS_ERROR_NULL_POINTER);
> >  
> >    nsAutoString tag;
> 
> Now unused

Thanks, fixed.

> @@ +3022,5 @@
> >      res = mHTMLEditor->GetStartNodeAndOffset(aSelection, getter_AddRefs(parent), &offset);
> >      NS_ENSURE_SUCCESS(res, res);
> >      
> >      // make sure we can put a list here
> > +    nsIAtom* listTypeAtom = NS_GetStaticAtom(*aListType);
> 
> nsCOMPtr<nsIAtom> listTypeAtom = do_GetAtom(*aListType);

Also in the other places, I guess?

> ::: editor/libeditor/html/nsHTMLEditor.cpp
> @@ +3782,4 @@
> >  {
> > +  NS_ASSERTION(aParentTag, "Null aParentTag");
> > +  NS_ASSERTION(aChildTag, "Null aChildTag");
> > +  if (!aParentTag || !aChildTag) {
> 
> Either MOZ_ASSERT or handle this, not both, please.

Oh, I misunderstood what MOZ_ASSERT did -- I didn't realize it only crashed in debug builds.  Yeah, that seems like a better idea, actually.

Comment 21

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

(In reply to Ms2ger from comment #18)
> I would QI to nsIContent and then check NodeType(), myself.

Note for future reference: DocumentFragments will QI successfully to dom::Element, because nsDocumentFragment inherits from nsGenericElement, so the existing code would return true for DocumentFragments in some cases.  nsINode::IsElement also returns true for DocumentFragments.  So the old code did something quite different from what it looked like.  After some fidgeting, I gave up and left it alone, because I couldn't get tests passing and it was taking far too much time that could be spent fixing actual bugs.

Comment 22

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Patch part 3, v3 -- Convert (Tag)CanContain(Tag) to const nsIAtom*

I hope this is the last version --  I spent a lot more time on this than I intended!  Updated per comment 18, except as noted in comment 21.  Try: https://tbpl.mozilla.org/?tree=Try&rev=fa8a1a0f9ffa

Comment 23

[Mozilla RelEng Bot]

Try run for fa8a1a0f9ffa is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=fa8a1a0f9ffa
Results (out of 222 total builds):
    success: 183
    warnings: 38
    failure: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-fa8a1a0f9ffa

Comment 24

[Benjamin Smedberg]

Comment on attachment 618248 [details] [diff] [review]
Patch part 2, v1 -- Allow easier use of const nsIAtom*

I really don't think we want to use the "const nsIAtom*" pattern, so I'd prefer we didn't start going down this path. nsAString and nsIAtom are pretty different in terms of structure and usage.

Comment 25

[(no longer active)]

Comment on attachment 619500 [details] [diff] [review]
Patch part 3, v3 -- Convert (Tag)CanContain(Tag) to const nsIAtom*

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

::: editor/libeditor/html/nsHTMLEditor.cpp
@@ +2474,5 @@
>          // have to find a place to put the blockquote
>          nsCOMPtr<nsIDOMNode> parent = node;
>          nsCOMPtr<nsIDOMNode> topChild = node;
>          nsCOMPtr<nsIDOMNode> tmp;
>          NS_NAMED_LITERAL_STRING(bq, "blockquote");

Please remove this variable if it's not used elsewhere in this function.

Comment 26

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #24)
> Comment on attachment 618248 [details] [diff] [review]
> Patch part 2, v1 -- Allow easier use of const nsIAtom*
> 
> I really don't think we want to use the "const nsIAtom*" pattern, so I'd
> prefer we didn't start going down this path. nsAString and nsIAtom are
> pretty different in terms of structure and usage.

Okay.  It wasn't very useful anyway due to bug 7646 -- it only worked until you needed to call ToUTF8String() or something.

Ehsan, I'll change patch part 4 (now part 3) to use nsIAtom* instead of const nsIAtom*, and keep your r+ because it's a trivial change.

(In reply to Ehsan Akhgari [:ehsan] from comment #25)
> Please remove this variable if it's not used elsewhere in this function.

It was used in one other place -- I changed it to NS_LITERAL_STRING instead.

Comment 27

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/32f636716335
https://hg.mozilla.org/integration/mozilla-inbound/rev/7b44d184f51b
https://hg.mozilla.org/integration/mozilla-inbound/rev/d2596504ce97

Note to mergers: these patches are cleanup, not actual fixes of the bug, so I'm not setting the target milestone or in-testsuite.

Comment 28

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/32f636716335
https://hg.mozilla.org/mozilla-central/rev/7b44d184f51b
https://hg.mozilla.org/mozilla-central/rev/d2596504ce97

Comment 29

[Mozilla RelEng Bot]

Try run for e097ddb45a60 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=e097ddb45a60
Results (out of 128 total builds):
    exception: 19
    success: 77
    warnings: 32
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-e097ddb45a60
 Timed out after 12 hours without completing.

Comment 30

[Mozilla RelEng Bot]

Autoland Patchset:
	Patches: 619382, 618248, 619396, 619398
	Branch: mozilla-central => try
Patch 619382 could not be applied to mozilla-central.
patching file editor/libeditor/html/nsHTMLEditRules.cpp
Hunk #1 FAILED at 1499
Hunk #2 FAILED at 6594
2 out of 2 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.

Comment 31

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Unassigning bugs from myself per: https://plus.google.com/100662365103380396132/posts/KyADU8K54uK

Comment 32

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

This bug has way too much cleanup without discussion of the original subject of the bug, so I'm going to close it.  I opened bug 1297414 to discuss the original issue.