676401 - The queryCommandEnabled API doesn't take the active editing host into account

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[(no longer active)]

See the testcase in the URL field.

The reason why this happens is that for most commands, we just look to see if there is an editor available, in which case we go ahead and return true.  This used to be correct before Firefox 3 when we only supported designMode, but in the face of contentEditable, we should pay attention to the active editing host.  For example, the testcase should alert false, because the selection doesn't fall in the editable section.

A good way to fix this is to look at the implementations of IsCommandEnabled in editor/ and make each one respect the active editing host.  For most commands, we should check to see if the selection's anchor node and check to see if it falls in the active editing host.

Fabien, can you look into this, please?

Comment 1

[Fabien Cazenave [:kaze]]

Working on it. I’ve added an `isSelectionEditable' read-only attribute to <nsIEditor> and I’m looking for a way to test all execCommands…

Side note (bug 456798): `queryCommandSupported' is not implemented and `queryCommandEnabled' raises an exception on non-supported commands.

Comment 2

[Fabien Cazenave [:kaze]]

Attachment: testcase

This testcase checks the result of `queryCommandEnabled' on the usual execCommands.

It doesn’t test cut/copy/paste yet because enhanced privileges are required. That will be done in the unit test.

There are a few `ns*Command' that can’t be tested that way:

./libeditor/base/nsEditorCommands.cpp
  nsCutOrDeleteCommand
  nsCopyOrDeleteCommand
  nsPasteTransferableCommand
  nsSwitchTextDirectionCommand
  nsSelectionMoveCommands
  nsInsertPlaintextCommand
  nsPasteQuotationCommand

./composer/src/nsComposerCommands.cpp
  nsBaseStateUpdatingCommand
  nsPasteNoFormattingCommand
  nsRemoveListCommand
  nsMultiStateCommand
  nsAbsolutePositioningCommand
  nsDecreaseZIndexCommand
  nsIncreaseZIndexCommand
  nsInsertTagCommand

./composer/src/nsComposerDocumentCommands.cpp
  nsSetDocumentOptionsCommand
  nsSetDocumentStateCommand

Ehsan, do you have a suggestion to test these commands?

Comment 3

[Fabien Cazenave [:kaze]]

Attachment: patch proposal

Comment 4

[Fabien Cazenave [:kaze]]

Attachment: patch proposal

forgot the `SimpleTest.finish()'

Comment 5

[(no longer active)]

(In reply to Fabien Cazenave (:kazé) from comment #2)
> Created attachment 552870 [details]
> testcase
> 
> This testcase checks the result of `queryCommandEnabled' on the usual
> execCommands.
> 
> It doesn’t test cut/copy/paste yet because enhanced privileges are required.
> That will be done in the unit test.
> 
> There are a few `ns*Command' that can’t be tested that way:
> 
> ./libeditor/base/nsEditorCommands.cpp
>   nsCutOrDeleteCommand
>   nsCopyOrDeleteCommand
>   nsPasteTransferableCommand
>   nsSwitchTextDirectionCommand
>   nsSelectionMoveCommands
>   nsInsertPlaintextCommand
>   nsPasteQuotationCommand
> 
> ./composer/src/nsComposerCommands.cpp
>   nsBaseStateUpdatingCommand
>   nsPasteNoFormattingCommand
>   nsRemoveListCommand
>   nsMultiStateCommand
>   nsAbsolutePositioningCommand
>   nsDecreaseZIndexCommand
>   nsIncreaseZIndexCommand
>   nsInsertTagCommand
> 
> ./composer/src/nsComposerDocumentCommands.cpp
>   nsSetDocumentOptionsCommand
>   nsSetDocumentStateCommand
> 
> Ehsan, do you have a suggestion to test these commands?

Those commands are not exposed through execCommand IINM.  To test them, you can grab the editor's command manager manually in a chrome test and fire these commands.

Comment 6

[(no longer active)]

Comment on attachment 552911 [details] [diff] [review]
patch proposal

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

I believe this is the wrong patch!

Comment 7

[(no longer active)]

Comment on attachment 552877 [details] [diff] [review]
patch proposal

I'm reviewing this version of the patch, instead.

>+  // common execCommands

Nit: common commands

>+  SpecialPowers.setCharPref("capability.policy.policynames",                      "allowclipboard");
>+  SpecialPowers.setCharPref("capability.policy.allowclipboard.sites",             "http://mochi.test:8888");
>+  SpecialPowers.setCharPref("capability.policy.allowclipboard.Clipboard.cutcopy", "allAccess");
>+  SpecialPowers.setCharPref("capability.policy.allowclipboard.Clipboard.paste",   "allAccess");
>+  commands = ["cut", "paste", "copy"];
>+  for (i = 0; i < commands.length; i++) {
>+    IsCommandEnabled(commands[i]);
>+    document.execCommand(commands[i], false, false);
>+  }
>+
>+  // delete/undo/redo
>+  commands = ["delete", "undo", "redo"];
>+  for (i = 0; i < commands.length; i++) {
>+    IsCommandEnabled(commands[i]);
>+    document.execCommand(commands[i], false, false);
>+  }

Why do you need to actually run these commands here?

Also, you should revert your changes to the prefs using clearUserPref when you're done in this test.

r=me with those.

Comment 8

[Fabien Cazenave [:kaze]]

(In reply to Ehsan Akhgari [:ehsan] from comment #6) 
> I believe this is the wrong patch!

Ugh, sorry. You’re right. :-/

(In reply to Ehsan Akhgari [:ehsan] from comment #7)
> >+  SpecialPowers.setCharPref("capability.policy.allowclipboard.Clipboard.paste",   "allAccess");
> >+  commands = ["cut", "paste", "copy"];
> >+  for (i = 0; i < commands.length; i++) {
> >+    IsCommandEnabled(commands[i]);
> >+    document.execCommand(commands[i], false, false);
> >+  }
> >+
> >+  // delete/undo/redo
> >+  commands = ["delete", "undo", "redo"];
> >+  for (i = 0; i < commands.length; i++) {
> >+    IsCommandEnabled(commands[i]);
> >+    document.execCommand(commands[i], false, false);
> >+  }
> 
> Why do you need to actually run these commands here?

We have to execute this commands because:
 * there's nothing to undo if we haven't modified the selection first;
 * there's nothing to redo if we haven't undone something first.

I’m getting a proper patch ready ASAP.

Comment 9

[Fabien Cazenave [:kaze]]

Attachment: patch proposal

updated and rebased patch

Comment 10

[(no longer active)]

Comment on attachment 553191 [details] [diff] [review]
patch proposal

Thanks!

Comment 11

[(no longer active)]

Has this passed try?  Does it need landing?

Comment 12

[Fabien Cazenave [:kaze]]

I'm finding out that this patch creates three regressions on browserscope2 tests: RTE2-QE_PASTE_TEXT-1 (dM|body|div), i.e. the test checking `document.queryCommandEnabled("paste")'.

Sorry, I haven’t seen this regression because my patch queue was a mess. I’ll look at this tomorrow.

Comment 13

[(no longer active)]

Hmm, I may have an idea what's going on with the RTE2-QE_PASTE_TEXT-1 failure.  You should make sure that there is actually something on the clipboard before querying for the paste command.  The clipboard on Linux is asynchronous, which probably causes this test to at least fail on Linux.

To test this hypothesis, try copying something to the clipboard and then running the tests manually.  If that fixes the problem, you can use the waitForClipboard function to make sure that there is something on the clipboard before you run the richtext2 suite.

Comment 14

[Fabien Cazenave [:kaze]]

Attachment: patch proposal

You’re right! Funny to see that my clipboard is empty after several hours of work… Vim magic.

This patch modifies test_richtext2.html to ensure the clipboard isn't empty before running the test suite, as you suggested. It’ll allow to run the browserscope/richtext2 tests alone.

Pushing to Try…

Comment 15

[Fabien Cazenave [:kaze]]

TryServer results look fine: http://tbpl.allizom.org/?usebuildbot=1&tree=Try&rev=d3b94afcef05

Comment 16

[(no longer active)]

Comment on attachment 553429 [details] [diff] [review]
patch proposal

Looks great, thanks!

Comment 17

[(no longer active)]

Pushed to inbound.

Comment 18

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/63f07bd1d64f

Comment 19

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

Comment on attachment 553429 [details] [diff] [review]
patch proposal

>+    PRBool isEditable = PR_FALSE;
>+    NS_SUCCEEDED(editor->GetIsSelectionEditable(&isEditable));

That doesn't make sense. Please file a followup to either remove the NS_SUCCEEDED() or to check its result?

Comment 20

[Eric Shepherd [:sheppy]]

Documentation updated:

https://developer.mozilla.org/en/XPCOM_Interface_Reference/NsIEditor#Attributes

Also mentioned on Firefox 9 for developers.

Comment 21

[[e-mail]]

"copy" should actually be always enabled. (It isn't because of bug 692153.)