Closed
Bug 748307
Opened 13 years ago
Closed 13 years ago
Support execCommand()s insertText, forwardDelete, insertParagraph per spec
Categories
(Core :: DOM: Editor, enhancement)
Core
DOM: Editor
Tracking
()
RESOLVED
FIXED
mozilla15
People
(Reporter: ayg, Assigned: ayg)
References
(Blocks 1 open bug)
Details
(Keywords: dev-doc-needed)
Attachments
(5 files, 1 obsolete file)
(deleted),
patch
|
ehsan.akhgari
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
ehsan.akhgari
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
ehsan.akhgari
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
ehsan.akhgari
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
ehsan.akhgari
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 3•13 years ago
|
||
Assignee | ||
Comment 4•13 years ago
|
||
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)
Assignee | ||
Comment 5•13 years ago
|
||
Attachment #625478 -
Flags: review?(ehsan)
Assignee | ||
Comment 6•13 years ago
|
||
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)
Assignee | ||
Comment 7•13 years ago
|
||
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 | ||
Updated•13 years ago
|
Assignee: ayg → nobody
Flags: in-testsuite+
Summary: Support execCommand("inserttext") → Support execCommand()s insertText, forwardDelete, insertParagraph per spec
Updated•13 years ago
|
Assignee: nobody → ayg
Comment 8•13 years ago
|
||
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•13 years ago
|
||
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
Assignee | ||
Comment 10•13 years ago
|
||
(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.
Assignee | ||
Comment 11•13 years ago
|
||
(Green try run: https://tbpl.mozilla.org/?tree=Try&rev=73e46d18eb1b)
Updated•13 years ago
|
Keywords: dev-doc-needed
Updated•13 years ago
|
Attachment #625476 -
Flags: review?(ehsan) → review+
Comment 12•13 years ago
|
||
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+
Updated•13 years ago
|
Attachment #625478 -
Flags: review?(ehsan) → review+
Comment 13•13 years ago
|
||
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+
Comment 14•13 years ago
|
||
(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•13 years ago
|
||
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-
Assignee | ||
Comment 16•13 years ago
|
||
(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.
Assignee | ||
Comment 17•13 years ago
|
||
See the changes to tests.js and data.js.
Attachment #625480 -
Attachment is obsolete: true
Attachment #626365 -
Flags: review?(ehsan)
Comment 18•13 years ago
|
||
(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.
Updated•13 years ago
|
Attachment #626365 -
Flags: review?(ehsan) → review+
Comment 19•13 years ago
|
||
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
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla15
Assignee | ||
Comment 20•13 years ago
|
||
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.
You need to log in
before you can comment on or make changes to this bug.
Description
•