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

RESOLVED FIXED in mozilla15

Status

()

Core
Editor
--
enhancement
RESOLVED FIXED
5 years ago
2 years ago

People

(Reporter: ayg, Assigned: ayg)

Tracking

(Depends on: 2 bugs, Blocks: 2 bugs, {dev-doc-needed})

Trunk
mozilla15
dev-doc-needed
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 1 obsolete attachment)

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.
Duplicate of this bug: 748306
Duplicate of this bug: 748304
Created attachment 625476 [details] [diff] [review]
Patch part 1, v1 -- Make TypedText's second argument a named enum
Assignee: nobody → ayg
Status: NEW → ASSIGNED
Attachment #625476 - Flags: review?(ehsan)
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.
Attachment #625477 - Flags: review?(ehsan)
Created attachment 625478 [details] [diff] [review]
Patch part 3, v1 -- Clean up WillDoAction
Attachment #625478 - Flags: review?(ehsan)
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.
Attachment #625479 - Flags: review?(ehsan)
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
Attachment #625480 - Flags: review?(ehsan)
Assignee: ayg → nobody
Blocks: 751843
No longer blocks: 685931
Flags: in-testsuite+
Summary: Support execCommand("inserttext") → Support execCommand()s insertText, forwardDelete, insertParagraph per spec
Blocks: 685931

Updated

5 years ago
Assignee: nobody → ayg
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 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
(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.
(Green try run: https://tbpl.mozilla.org/?tree=Try&rev=73e46d18eb1b)
Keywords: dev-doc-needed
Attachment #625476 - Flags: review?(ehsan) → review+
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.
Attachment #625477 - Flags: review?(ehsan) → review+
Attachment #625478 - Flags: review?(ehsan) → review+
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.
Attachment #625479 - Flags: review?(ehsan) → review+
(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 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.
Attachment #625480 - Flags: review?(ehsan) → review-
(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.
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.
Attachment #625480 - Attachment is obsolete: true
Attachment #626365 - Flags: review?(ehsan)
(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.
Attachment #626365 - Flags: review?(ehsan) → review+
Depends on: 758576
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
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla15

Updated

5 years ago
Depends on: 759748

Updated

5 years ago
Depends on: 761861
No longer depends on: 761861
Depends on: 761993
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.

Updated

3 years ago
Depends on: 1130651

Updated

2 years ago
Depends on: 1220696
You need to log in before you can comment on or make changes to this bug.