748307 - Support execCommand()s insertText, forwardDelete, insertParagraph per spec

Status: RESOLVED FIXED

(Core :: DOM: Editor, enhancement)

Description

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

In WebKit, execCommand("inserttext", false, "foo") is the same as if the user typed "foo", generally replacing the selection with "foo".  This seems useful and I specced it.

Comment 3

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

Attachment: Patch part 1, v1 -- Make TypedText's second argument a named enum

Comment 4

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

Attachment: Patch part 2, v1 -- Make WillDoAction take an nsTypedSelection

I'm working on the theory that we always want to use nsTypedSelection where possible.  Let me know if I shouldn't  be doing this.  It's a lot easier to switch to than nsINode, because it's actually an instance of nsISelection, so no wrappers needed for things that expect nsISelection.

Comment 5

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

Attachment: Patch part 3, v1 -- Clean up WillDoAction

Comment 6

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

Attachment: Patch part 4, v1 -- Clean up ExecCommand and QueryCommand*

I know you've said you don't like how NS_ENSURE_* obscures control flow, but there are a lot of places here where we return NS_ERROR_FAILURE or whatnot, and I've spent a lot of time before trying to track down where an error is coming from.  The warning printed to the console is very useful.

Comment 7

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

Attachment: Patch part 5, v1 -- Support insertText, forwardDelete, insertParagraph per spec

There are some test regressions here.  I think they're just cases where we didn't support the command, so we did nothing, and coincidentally we were supposed to do nothing in that specific case.

I'm not totally sure about adding cmd_forwardDelete.  It seems to do basically the same thing as the preexisting cmd_deleteCharForward; but on the other hand, cmd_delete seems to do basically the same thing as the preexisting cmd_deleteCharBackward.  Is the difference in nsDeleteCommand::IsCommandEnabled really meant to be there, or could that be made saner?

The biggest win from this patch is that we get much better regression-testing for typing text/hitting Enter/hitting Delete.  Previously those code paths weren't directly exposed to normal JS, so test_runtest.html/richtext2/etc. weren't testing them.  (I do want to improve test_runtest.html so it also tests using synthesizeKey where appropriate, but this is easier and works about as well.)

Try: https://tbpl.mozilla.org/?tree=Try&rev=8a8b98dba55d

Comment 8

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

Comment on attachment 625477 [details] [diff] [review]
Patch part 2, v1 -- Make WillDoAction take an nsTypedSelection

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

::: editor/libeditor/html/nsHTMLEditor.cpp
@@ +4918,5 @@
>  
>    // Protect the edit rules object from dying
>    nsCOMPtr<nsIEditRules> kungFuDeathGrip(mRules);
>  
>    nsresult res;

Move this declaration down, please

Comment 9

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

Comment on attachment 625479 [details] [diff] [review]
Patch part 4, v1 -- Clean up ExecCommand and QueryCommand*

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

::: content/html/document/src/nsHTMLDocument.cpp
@@ +3147,2 @@
>        rv = cmdParams->SetStringValue("state_attribute", value);
> +    } else if (cmdToDispatch.Equals("cmd_insertHTML")) {

EqualsLiteral should work, I hope

@@ +3343,2 @@
>    // GetCommandState like the other commands
> +  if (cmdToDispatch.Equals("cmd_getContents")) {

And here

Comment 10

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

(docshell/base/crashtests/432114-2.html is timing out here because it does insertParagraph and expects a DOMNodeRemoved event to fire.  I've changed it to do execCommand("formatBlock", false, "p") and that fixes it, but I won't bother uploading a new patch to Bugzilla.)

(In reply to Ms2ger from comment #8)
> ...

(In reply to Ms2ger from comment #9)
> ...

Thanks, done.

Comment 11

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

(Green try run: https://tbpl.mozilla.org/?tree=Try&rev=73e46d18eb1b)

Comment 12

[(no longer active)]

Comment on attachment 625477 [details] [diff] [review]
Patch part 2, v1 -- Make WillDoAction take an nsTypedSelection

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

r=me with Ms2ger's comment addressed.

Comment 13

[(no longer active)]

Comment on attachment 625479 [details] [diff] [review]
Patch part 4, v1 -- Clean up ExecCommand and QueryCommand*

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

::: content/html/document/src/nsHTMLDocument.cpp
@@ +3285,3 @@
>        nsMemory::Free(actualAlignmentType);
> +    }
> +    NS_ENSURE_SUCCESS(rv, rv);

Nit: please move this up to where |rv| is set.

Comment 14

[(no longer active)]

(In reply to Aryeh Gregor from comment #6)
> Created attachment 625479 [details] [diff] [review]
> Patch part 4, v1 -- Clean up ExecCommand and QueryCommand*
> 
> I know you've said you don't like how NS_ENSURE_* obscures control flow, but
> there are a lot of places here where we return NS_ERROR_FAILURE or whatnot,
> and I've spent a lot of time before trying to track down where an error is
> coming from.  The warning printed to the console is very useful.

Not sure who this is directed to.  If you're talking to me, I don't have anything against NS_ENSURE_*.  In fact, I use the warnings when I want to figure out what's going wrong in the editor too!

Comment 15

[(no longer active)]

Comment on attachment 625480 [details] [diff] [review]
Patch part 5, v1 -- Support insertText, forwardDelete, insertParagraph per spec

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

With this patch, what will happen if you pass an HTML string to insertText?  I don't think this protects against that case.  Also, if there are no existing tests which cover that case, we should add some.

Comment 16

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

(In reply to Ehsan Akhgari [:ehsan] from comment #13)
> Comment on attachment 625479 [details] [diff] [review]
> Patch part 4, v1 -- Clean up ExecCommand and QueryCommand*
> 
> Review of attachment 625479 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/html/document/src/nsHTMLDocument.cpp
> @@ +3285,3 @@
> >        nsMemory::Free(actualAlignmentType);
> > +    }
> > +    NS_ENSURE_SUCCESS(rv, rv);
> 
> Nit: please move this up to where |rv| is set.

I thought the idea of that code was that the Free() would occur even if there was an error.  If I moved it up and rv was a failure, we'd return from the function without calling Free(), which sounds bad.  Do you really want me to do that?

Why we're using manual memory allocation here instead of an nsAutoString or something is a different question, of course!  Should I try to switch it to that instead?

(In reply to Ehsan Akhgari [:ehsan] from comment #14)
> Not sure who this is directed to.  If you're talking to me, I don't have
> anything against NS_ENSURE_*.  In fact, I use the warnings when I want to
> figure out what's going wrong in the editor too!

I recently saw you say something like that on some newsgroup posting.  Doesn't matter.

(In reply to Ehsan Akhgari [:ehsan] from comment #15)
> With this patch, what will happen if you pass an HTML string to insertText? 
> I don't think this protects against that case.  Also, if there are no
> existing tests which cover that case, we should add some.

I just added some to the editing spec test suite; will attach a new patch shortly.  The code handles it fine -- it just dumps the text into a text node, so escaping isn't needed.

Comment 17

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

Attachment: Patch part 5, v2 -- Support insertText, forwardDelete, insertParagraph per spec (now with more tests)

See the changes to tests.js and data.js.

Comment 18

[(no longer active)]

(In reply to Aryeh Gregor from comment #16)
> (In reply to Ehsan Akhgari [:ehsan] from comment #13)
> > Comment on attachment 625479 [details] [diff] [review]
> > Patch part 4, v1 -- Clean up ExecCommand and QueryCommand*
> > 
> > Review of attachment 625479 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: content/html/document/src/nsHTMLDocument.cpp
> > @@ +3285,3 @@
> > >        nsMemory::Free(actualAlignmentType);
> > > +    }
> > > +    NS_ENSURE_SUCCESS(rv, rv);
> > 
> > Nit: please move this up to where |rv| is set.
> 
> I thought the idea of that code was that the Free() would occur even if
> there was an error.  If I moved it up and rv was a failure, we'd return from
> the function without calling Free(), which sounds bad.  Do you really want
> me to do that?

Oh, right!

> Why we're using manual memory allocation here instead of an nsAutoString or
> something is a different question, of course!  Should I try to switch it to
> that instead?

Please!  Or file a follow-up.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c5f15a9a3308
https://hg.mozilla.org/mozilla-central/rev/f45e87248f24
https://hg.mozilla.org/mozilla-central/rev/d3d137c04d72
https://hg.mozilla.org/mozilla-central/rev/d216ddd1d2ed
https://hg.mozilla.org/mozilla-central/rev/91bb23730236

Comment 20

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

Note for documentation-writers: bug 761993 plans to revert the insertParagraph change added by this bug, so if that lands, documentation for Firefox 15 should not mention any change to insertParagraph support.