The spec says execCommand() should return false if the command isn't enabled. This is what we should do; otherwise what does "enabled" mean? I'd like to fix this somehow in DoCommand(Params)(), so that all commands will automatically return before executing if they're not enabled. But I don't see an easy way to do that. Doing it in ExecCommand() would be easy enough, although it wouldn't fix other callers.

So more than a couple of Jesse's fuzz hits would have been prevented by this, like "run insertHTML with selection in detached node" (bug 766360). It's easy enough to fix this for callers of ExecCommand, at least.

Once this is fixed, should it be considered a bug if execCommand throws?

In principle, yes, I don't think execCommand() should ever throw, and the spec says it shouldn't. (There was some discussion about whether it should throw for unsupported commands, but we eventually concluded no.) In practice, we have tons of known situations in which it does throw -- e.g., if there's no editing host on the page -- but I'd think they should be filed as bugs if they aren't already, yeah.

So perhaps predictably, I had to change the definition of "enabled" for a few commands: * I made nsSetDocumentStateCommand always enabled. This means cmd_setDocumentModified, cmd_setDocumentUseCSS, cmd_setDocumentReadOnly, cmd_insertBrOnReturn, cmd_enableObjectResizing, and cmd_enableInlineTableEditing. Previously they were disabled if the selection wasn't editable, which doesn't make a lot of sense to me, since they should work fine in that case, right? At least styleWithCSS should be per-document according to the spec -- the rest are nonstandard, so I can't swear to anything. * I changed nsDeleteCommand (delete/forwardDelete) to just checking GetIsSelectionEditable() like most commands. Previously it would check CanCut() as well, which is clearly wrong -- that returns false if the selection is collapsed. * I also changed nsOutdentCommand to just checking GetIsSelectionEditable(). Previously it would also return false if the selection wasn't indented. This makes some sense, and matches Opera Next 12.00 alpha; but it goes against the spec, IE10 Developer Preview, Chrome 21 dev, and richtext2. I think it's better to make queryCommandEnabled() consistent rather than try to cover all the reasons a command might fail, which we can never do anyway. So most commands will now fail if the selection's anchor is not in an editable node. I think this should actually be strengthened somewhat -- the spec says """ [Most commands] are enabled if the active range is not null, its start node is either editable or an editing host, its end node is either editable or an editing host, and there is some editing host that is an inclusive ancestor of both its start node and its end node. """ http://dvcs.w3.org/hg/editing/raw-file/tip/editing.html#enabled-commands See <https://www.w3.org/Bugs/Public/show_bug.cgi?id=16094> for background on why that definition is used. But that can be for a separate bug -- the difference is relatively small. (It's hard if not impossible to persuade Gecko to create selections that span editing hosts unless you force it by modifying the endpoints with JS.) Try: https://tbpl.mozilla.org/?tree=Try&rev=209a9184995c

Ah . . . I probably should have guessed, but I didn't realize that IsCommandEnabled is actually used internally by Gecko for its own menu items. I'm getting 1300 ERROR TEST-UNEXPECTED-FAIL | /tests/browser/base/content/test/test_contextmenu.html | checking item #5 (context-delete) enabled state - got true, expected false and some similar others, and indeed the "Delete" item is incorrectly enabled on the context menu if nothing is selected. This makes sense because that item is meant to be "delete the selection", but it doesn't make sense for execCommand("delete") or execCommand("forwardDelete"). Do we want a new cmd_deleteSelection or cmd_deleteDirectionless or something that can be used by the context menu? Also, this raises the issue (which I guess I should have been thinking about but wasn't) that if authors build a richtext editor and use queryCommandEnabled() to gray out the icons, outdent will now not be grayed out in a situation where you might expect. LibreOffice 3.5.3.2 does not gray out the icon in this case, and Ms2ger reports that neither does Word 2003. So I think it's safe to match the spec/other browsers here.

Comment on attachment 635207 [details] [diff] [review] Patch v1 New patch to come with a cmd_deleteDirectionless.

So this patch is basically the same as the last, except 1) It makes cmd_delete still disabled if canCut() is false. This was actually always the case, just the code was written very confusingly so I didn't think about it. When adding cmd_forwardDelete, I copied this feature of cmd_delete without understanding why. 2) It changes the direction of cmd_delete from eForward to eNone, and makes execCommand("delete") use cmd_deleteCharBackward instead. It doesn't make sense to have both cmd_delete and cmd_deleteCharBackward do the same thing! Clearly at least some users in browser/ expect the command to behave this way -- delete the selected text, not the next char. (Which happens to work the same if you never execute it with collapsed selection.) 3) For symmetry, it gets rid of cmd_forwardDelete and uses cmd_deleteCharForward instead. I wondered when I added cmd_forwardDelete why we had redundant cmd_delete/cmd_deleteCharBackward. (2) admittedly scares me, and I'd be unsurprised if it caused some breakage. But it seems like the "right" fix -- separate "delete current selection" from "delete next char". Callers should be easily fixed to use cmd_deleteCharBackward instead, if that's what they want. A lower-risk but hackier fix would be to leave cmd_delete as ePrevious, and switch execCommand("delete") to using cmd_deleteCharBackward. This would avoid breakage in the cases where a caller ran cmd_delete without checking IsCommandEnabled() -- but in those cases we'd ideally want to fix the caller. Would you prefer that? New try: https://tbpl.mozilla.org/?tree=Try&rev=48ea17935aba

(In reply to Aryeh Gregor from comment #3) > In principle, yes, I don't think execCommand() should ever throw, and the > spec says it shouldn't. (There was some discussion about whether it should > throw for unsupported commands, but we eventually concluded no.) In > practice, we have tons of known situations in which it does throw -- e.g., > if there's no editing host on the page -- but I'd think they should be filed > as bugs if they aren't already, yeah. Agreed.

Comment on attachment 635264 [details] [diff] [review] Patch v2, hopefully avoids test regressions Review of attachment 635264 [details] [diff] [review]: ----------------------------------------------------------------- I _think_ this is fine, but it's going to be risky. I'd like to see the answer to my comment about nsEditorController.cpp before r+ing... ::: content/html/document/src/nsHTMLDocument.cpp @@ +3190,5 @@ > } > > + // Return false for disabled commands (bug 760052) > + bool enabled; > + cmdMgr->IsCommandEnabled(cmdToDispatch.get(), window, &enabled); Please initialize enabled, it's not safe to assume that IsCommandEnabled always sets it. ::: editor/libeditor/base/nsEditorCommands.cpp @@ +562,4 @@ > nsIEditor::EDirection deleteDir = nsIEditor::eNone; > + > + if (!nsCRT::strcmp("cmd_delete", aCommandName)) { > + deleteDir = nsIEditor::eNone; I assume that you have verified that passing eNone to DeleteSelection doesn't explode somewhere. :-) ::: editor/libeditor/base/nsEditorController.cpp @@ -57,5 @@ > > NS_REGISTER_ONE_COMMAND(nsSwitchTextDirectionCommand, "cmd_switchTextDirection"); > > NS_REGISTER_FIRST_COMMAND(nsDeleteCommand, "cmd_delete"); > - NS_REGISTER_NEXT_COMMAND(nsDeleteCommand, "cmd_forwardDelete"); Hmm, so what handles cmd_forwardDelete now?

Will this fix assertions for keyboard equivalents as well? For example, hitting del on the keyboard instead of calling execCommand("forwardDelete").

(In reply to Ehsan Akhgari [:ehsan] from comment #9) > ::: content/html/document/src/nsHTMLDocument.cpp > @@ +3190,5 @@ > > } > > > > + // Return false for disabled commands (bug 760052) > > + bool enabled; > > + cmdMgr->IsCommandEnabled(cmdToDispatch.get(), window, &enabled); > > Please initialize enabled, it's not safe to assume that IsCommandEnabled > always sets it. Okay, will do. Initialize to true or false? > I assume that you have verified that passing eNone to DeleteSelection > doesn't explode somewhere. :-) Um, well, not as such. I mean, tests pass, but not beyond that. Should I just pass ePrevious here instead, so that cmd_delete and cmd_deleteCharBackward only differ in when they're enabled and not in their actual effect? With the patch, cmd_delete isn't web-accessible anyway, so it seems like the more sensible thing to do. > ::: editor/libeditor/base/nsEditorController.cpp > @@ -57,5 @@ > > > > NS_REGISTER_ONE_COMMAND(nsSwitchTextDirectionCommand, "cmd_switchTextDirection"); > > > > NS_REGISTER_FIRST_COMMAND(nsDeleteCommand, "cmd_delete"); > > - NS_REGISTER_NEXT_COMMAND(nsDeleteCommand, "cmd_forwardDelete"); > > Hmm, so what handles cmd_forwardDelete now? Nothing -- I got rid of it. See gMidasCommandTable changes. execCommand("forwardDelete") now calls cmd_deleteCharForward instead of its own made-up command. (In reply to Jesse Ruderman from comment #10) > Will this fix assertions for keyboard equivalents as well? For example, > hitting del on the keyboard instead of calling execCommand("forwardDelete"). execCommand("forwardDelete") is meant to behave identically to deleting a character using the delete key. By the time it gets to actual editor actions that do things like delete nodes or whatnot, the codepaths are identical. Basically, both of them call nsEditor::DeleteSelection, so if that's in the call stack, they're almost certainly the same. Likewise execCommand("delete") and backspace, and execCommand("insertText") should behave the same as typing text too.

I added the test from bug 766360 here, and rebased some expected test results because of bug 766387 sneaking ahead of this in my patch queue. Ignoring that, here's the interdiff: diff --git a/content/html/document/src/nsHTMLDocument.cpp b/content/html/document/src/nsHTMLDocument.cpp --- a/content/html/document/src/nsHTMLDocument.cpp +++ b/content/html/document/src/nsHTMLDocument.cpp @@ -3185,17 +3185,17 @@ nsHTMLDocument::ExecCommand(const nsAStr cmdToDispatch.EqualsLiteral("cmd_insertLinkNoUI") || cmdToDispatch.EqualsLiteral("cmd_paragraphState")) && paramStr.IsEmpty()) { // Invalid value, return false return NS_OK; } // Return false for disabled commands (bug 760052) - bool enabled; + bool enabled = false; cmdMgr->IsCommandEnabled(cmdToDispatch.get(), window, &enabled); if (!enabled) { return NS_OK; } if (!isBool && paramStr.IsEmpty()) { rv = cmdMgr->DoCommand(cmdToDispatch.get(), nsnull, window); } else { diff --git a/editor/libeditor/base/nsEditorCommands.cpp b/editor/libeditor/base/nsEditorCommands.cpp --- a/editor/libeditor/base/nsEditorCommands.cpp +++ b/editor/libeditor/base/nsEditorCommands.cpp @@ -557,17 +557,20 @@ nsDeleteCommand::DoCommand(const char* a nsISupports* aCommandRefCon) { nsCOMPtr<nsIEditor> editor = do_QueryInterface(aCommandRefCon); NS_ENSURE_TRUE(editor, NS_ERROR_FAILURE); nsIEditor::EDirection deleteDir = nsIEditor::eNone; if (!nsCRT::strcmp("cmd_delete", aCommandName)) { - deleteDir = nsIEditor::eNone; + // Really this should probably be eNone, but it only makes a difference if + // the selection is collapsed, and then this command is disabled. So let's + // keep it as it always was to avoid breaking things. + deleteDir = nsIEditor::ePrevious; } else if (!nsCRT::strcmp("cmd_deleteCharForward", aCommandName)) { deleteDir = nsIEditor::eNext; } else if (!nsCRT::strcmp("cmd_deleteCharBackward", aCommandName)) { deleteDir = nsIEditor::ePrevious; } else if (!nsCRT::strcmp("cmd_deleteWordBackward", aCommandName)) { deleteDir = nsIEditor::ePreviousWord; } else if (!nsCRT::strcmp("cmd_deleteWordForward", aCommandName)) { deleteDir = nsIEditor::eNextWord; Try: https://tbpl.mozilla.org/?tree=Try&rev=1b32a64bc0ab

Comment on attachment 636140 [details] [diff] [review] Patch v3, addresses review comments and adds bug 766360 crashtest Yeah, I think using ePrevious could be safer.