Last Comment Bug 748307 - Support execCommand()s insertText, forwardDelete, insertParagraph per spec
: Support execCommand()s insertText, forwardDelete, insertParagraph per spec
Status: RESOLVED FIXED
: dev-doc-needed
Product: Core
Classification: Components
Component: Editor (show other bugs)
: Trunk
: All All
: -- enhancement (vote)
: mozilla15
Assigned To: :Aryeh Gregor (away until August 15)
:
Mentors:
: 748304 748306 (view as bug list)
Depends on: 758576 1130651 1220696 759748 761993
Blocks: richtext2 editingspectests
  Show dependency treegraph
 
Reported: 2012-04-24 05:18 PDT by :Aryeh Gregor (away until August 15)
Modified: 2015-12-25 16:29 PST (History)
3 users (show)
ayg: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch part 1, v1 -- Make TypedText's second argument a named enum (6.01 KB, patch)
2012-05-20 03:03 PDT, :Aryeh Gregor (away until August 15)
ehsan: review+
Details | Diff | Splinter Review
Patch part 2, v1 -- Make WillDoAction take an nsTypedSelection (30.88 KB, patch)
2012-05-20 03:04 PDT, :Aryeh Gregor (away until August 15)
ehsan: review+
Details | Diff | Splinter Review
Patch part 3, v1 -- Clean up WillDoAction (9.53 KB, patch)
2012-05-20 03:05 PDT, :Aryeh Gregor (away until August 15)
ehsan: review+
Details | Diff | Splinter Review
Patch part 4, v1 -- Clean up ExecCommand and QueryCommand* (12.38 KB, patch)
2012-05-20 03:07 PDT, :Aryeh Gregor (away until August 15)
ehsan: review+
Details | Diff | Splinter Review
Patch part 5, v1 -- Support insertText, forwardDelete, insertParagraph per spec (727.53 KB, patch)
2012-05-20 03:12 PDT, :Aryeh Gregor (away until August 15)
ehsan: review-
Details | Diff | Splinter Review
Patch part 5, v2 -- Support insertText, forwardDelete, insertParagraph per spec (now with more tests) (730.83 KB, patch)
2012-05-23 01:35 PDT, :Aryeh Gregor (away until August 15)
ehsan: review+
Details | Diff | Splinter Review

Description :Aryeh Gregor (away until August 15) 2012-04-24 05:18:39 PDT
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 1 :Aryeh Gregor (away until August 15) 2012-05-20 03:02:12 PDT
*** Bug 748306 has been marked as a duplicate of this bug. ***
Comment 2 :Aryeh Gregor (away until August 15) 2012-05-20 03:02:20 PDT
*** Bug 748304 has been marked as a duplicate of this bug. ***
Comment 3 :Aryeh Gregor (away until August 15) 2012-05-20 03:03:10 PDT
Created attachment 625476 [details] [diff] [review]
Patch part 1, v1 -- Make TypedText's second argument a named enum
Comment 4 :Aryeh Gregor (away until August 15) 2012-05-20 03:04:15 PDT
Created attachment 625477 [details] [diff] [review]
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 (away until August 15) 2012-05-20 03:05:22 PDT
Created attachment 625478 [details] [diff] [review]
Patch part 3, v1 -- Clean up WillDoAction
Comment 6 :Aryeh Gregor (away until August 15) 2012-05-20 03:07:00 PDT
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.
Comment 7 :Aryeh Gregor (away until August 15) 2012-05-20 03:12:40 PDT
Created attachment 625480 [details] [diff] [review]
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 2012-05-20 04:04:58 PDT
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 2012-05-20 04:08:30 PDT
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 (away until August 15) 2012-05-20 04:54:00 PDT
(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 (away until August 15) 2012-05-22 02:24:38 PDT
(Green try run: https://tbpl.mozilla.org/?tree=Try&rev=73e46d18eb1b)
Comment 12 :Ehsan Akhgari 2012-05-22 11:36:46 PDT
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 :Ehsan Akhgari 2012-05-22 11:45:27 PDT
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 :Ehsan Akhgari 2012-05-22 11:49:54 PDT
(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 :Ehsan Akhgari 2012-05-22 12:40:23 PDT
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 (away until August 15) 2012-05-23 01:29:16 PDT
(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 (away until August 15) 2012-05-23 01:35:03 PDT
Created attachment 626365 [details] [diff] [review]
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 :Ehsan Akhgari 2012-05-23 14:25:38 PDT
(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 20 :Aryeh Gregor (away until August 15) 2012-06-06 05:09:49 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.